Messages in this thread | | | From | Quanfa Fu <> | Date | Mon, 9 Jan 2023 11:44:46 +0800 | Subject | Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc |
| |
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
| |