Received-SPF: pass (google.com: domain of linux-kernel+bounces-110128-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 21 Mar 2024 10:15:47 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Ye Bin <yebin10@huawei.com>
Cc: <mhiramat@kernel.org>, <mathieu.desnoyers@efficios.com>,
 <linux-trace-kernel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 1/5] tracing/probes: support '%pd' type for print
 struct dentry's name
Message-ID: <20240321101547.1f4e68ab@gandalf.local.home>
In-Reply-To: <20240320132924.2802187-2-yebin10@huawei.com>
References: <20240320132924.2802187-1-yebin10@huawei.com>
	<20240320132924.2802187-2-yebin10@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 20 Mar 2024 21:29:20 +0800
Ye Bin <yebin10@huawei.com> wrote:

> Support print type '%pd' for print dentry's  name.
> 

The above is not a very detailed change log. A change log should state not
only what the change is doing, but also why.

Having examples of before and after would be useful in the change log.

> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  kernel/trace/trace.c        |  2 +-
>  kernel/trace/trace_fprobe.c |  6 +++++
>  kernel/trace/trace_kprobe.c |  6 +++++
>  kernel/trace/trace_probe.c  | 50 +++++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_probe.h  |  2 ++
>  5 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b12f8384a36a..ac26b8446337 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c


> +/* @buf: *buf must be equal to NULL. Caller must to free *buf */
> +int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
> +{
> +	int i, used, ret;
> +	const int bufsize = MAX_DENTRY_ARGS_LEN;
> +	char *tmpbuf = NULL;
> +
> +	if (*buf)
> +		return -EINVAL;
> +
> +	used = 0;
> +	for (i = 0; i < argc; i++) {
> +		if (glob_match("*:%pd", argv[i])) {
> +			char *tmp;
> +			char *equal;
> +
> +			if (!tmpbuf) {
> +				tmpbuf = kmalloc(bufsize, GFP_KERNEL);
> +				if (!tmpbuf)
> +					return -ENOMEM;
> +			}
> +
> +			tmp = kstrdup(argv[i], GFP_KERNEL);
> +			if (!tmp)
> +				goto nomem;
> +
> +			equal = strchr(tmp, '=');
> +			if (equal)
> +				*equal = '\0';
> +			tmp[strlen(argv[i]) - 4] = '\0';
> +			ret = snprintf(tmpbuf + used, bufsize - used,
> +				       "%s%s+0x0(+0x%zx(%s)):string",
> +				       equal ? tmp : "", equal ? "=" : "",
> +				       offsetof(struct dentry, d_name.name),
> +				       equal ? equal + 1 : tmp);

What would be really useful is if we had a way to expose BTF here. Something like:

 "%pB:<struct>:<field>"

The "%pB" would mean to look up the struct/field offsets and types via BTF,
and create the appropriate command to find and print it.

That would be much more flexible and useful as it would allow reading any
field from any structure without having to use gdb.

-- Steve


> +			kfree(tmp);
> +			if (ret >= bufsize - used)
> +				goto nomem;
> +			argv[i] = tmpbuf + used;
> +			used += ret + 1;
> +		}
> +	}
> +
> +	*buf = tmpbuf;
> +	return 0;
> +nomem:
> +	kfree(tmpbuf);
> +	return -ENOMEM;
> +}