| Message ID | 20230107034335.1154374-1-quanfafu@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/eprobe: Replace kzalloc with kmalloc | expand |
On Sat, 7 Jan 2023 11:43:35 +0800 Quanfa Fu <quanfafu@gmail.com> wrote: > Since this memory will be filled soon below, I feel that there is kzalloc() is also used as a safety measure to make sure nothing is accidentally exposed. I rather keep it for safety. Just because it doesn't need to be here doesn't mean it should be removed. There is no benefit to making this kmalloc(), as this is far from a fast path. > no need for a memory of all zeros here. 'snprintf' does not return > negative num according to ISO C99, so I feel that no judgment is > needed here. Also, it's best to remove "feelings" from change logs. Code updates are not made due to how one feels about something (at least it shouldn't be), but about having technical reasons for doing so. I do agree there's no reason to check snprintf() from returning negative, as looking at its implementation, there is no negative return. Thus, the change log should be: "No need to check for negative return value from snprintf() as the code does not return negative values." > > No functional change intended. And this does have functional changes. If the output of a compiler is different for a function, then that's a functional change. What we consider non functional changes is if functions get moved around, or possibly code in a function is moved into a helper function where the compiler *should* end up with the same assembly. Changing what malloc is called is definitely a functional change. > > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > --- > kernel/trace/trace_eprobe.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index 352b65e2b910..cd1d271a74e7 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > for (i = 0; i < argc; i++) > len += strlen(argv[i]) + 1; > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > + ep->filter_str = kmalloc(len, GFP_KERNEL); > if (!ep->filter_str) > return -ENOMEM; > > p = ep->filter_str; > for (i = 0; i < argc; i++) { > ret = snprintf(p, len, "%s ", argv[i]); I wonder if this should be a strncat() instead? > - if (ret < 0) > - goto error; > if (ret > len) { > ret = -E2BIG; > goto error; for (i = 0; i < arcc, i++) strncat(ep->filter_str, argv[i], len); I mean, before this code we have that loop already determining what len is, do we really need to check if it is going to be -E2BIG? -- Steve
Thanks a lot. Learned a lot from here. I replaced snprintf with memcpy in Patchv2 On Mon, Jan 9, 2023 at 5:22 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Sat, 7 Jan 2023 11:43:35 +0800 > Quanfa Fu <quanfafu@gmail.com> wrote: > > > Since this memory will be filled soon below, I feel that there is > > kzalloc() is also used as a safety measure to make sure nothing is > accidentally exposed. I rather keep it for safety. Just because it > doesn't need to be here doesn't mean it should be removed. There is no > benefit to making this kmalloc(), as this is far from a fast path. > > > no need for a memory of all zeros here. 'snprintf' does not return > > negative num according to ISO C99, so I feel that no judgment is > > needed here. > > Also, it's best to remove "feelings" from change logs. Code updates are > not made due to how one feels about something (at least it shouldn't > be), but about having technical reasons for doing so. I do agree > there's no reason to check snprintf() from returning negative, as > looking at its implementation, there is no negative return. Thus, the > change log should be: > > "No need to check for negative return value from snprintf() as the > code does not return negative values." > > > > > No functional change intended. > > And this does have functional changes. If the output of a compiler is > different for a function, then that's a functional change. What we > consider non functional changes is if functions get moved around, or > possibly code in a function is moved into a helper function where the > compiler *should* end up with the same assembly. > > Changing what malloc is called is definitely a functional change. > > > > > Signed-off-by: Quanfa Fu <quanfafu@gmail.com> > > --- > > kernel/trace/trace_eprobe.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > > index 352b65e2b910..cd1d271a74e7 100644 > > --- a/kernel/trace/trace_eprobe.c > > +++ b/kernel/trace/trace_eprobe.c > > @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch > > for (i = 0; i < argc; i++) > > len += strlen(argv[i]) + 1; > > > > - ep->filter_str = kzalloc(len, GFP_KERNEL); > > + ep->filter_str = kmalloc(len, GFP_KERNEL); > > if (!ep->filter_str) > > return -ENOMEM; > > > > p = ep->filter_str; > > for (i = 0; i < argc; i++) { > > ret = snprintf(p, len, "%s ", argv[i]); > > I wonder if this should be a strncat() instead? > > > - if (ret < 0) > > - goto error; > > if (ret > len) { > > ret = -E2BIG; > > goto error; > > for (i = 0; i < arcc, i++) > strncat(ep->filter_str, argv[i], len); > > I mean, before this code we have that loop already determining what len > is, do we really need to check if it is going to be -E2BIG? > > -- Steve
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c index 352b65e2b910..cd1d271a74e7 100644 --- a/kernel/trace/trace_eprobe.c +++ b/kernel/trace/trace_eprobe.c @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch for (i = 0; i < argc; i++) len += strlen(argv[i]) + 1; - ep->filter_str = kzalloc(len, GFP_KERNEL); + ep->filter_str = kmalloc(len, GFP_KERNEL); if (!ep->filter_str) return -ENOMEM; p = ep->filter_str; for (i = 0; i < argc; i++) { ret = snprintf(p, len, "%s ", argv[i]); - if (ret < 0) - goto error; if (ret > len) { ret = -E2BIG; goto error;
Since this memory will be filled soon below, I feel that there is no need for a memory of all zeros here. 'snprintf' does not return negative num according to ISO C99, so I feel that no judgment is needed here. No functional change intended. Signed-off-by: Quanfa Fu <quanfafu@gmail.com> --- kernel/trace/trace_eprobe.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)