Re: [PATCH v6 1/3] tracing: eprobe: remove duplicate is_good_name() operation

From: Linyu Yuan
Date: Sun Jun 26 2022 - 20:33:53 EST


hi Tom,

On 6/26/2022 1:25 AM, Tom Zanussi wrote:
Hi Linyu,

On Tue, 2022-06-21 at 09:59 +0800, Linyu Yuan wrote:
traceprobe_parse_event_name() already validate SYSTEM and EVENT name,
there is no need to call is_good_name() after it.

Add trace_probe_log_set_index(1) to allow report correct error
if user input wrong SYSTEM.EVENT format.

Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
---
v2: drop v1 change as it is NACK.
    add it to remove duplicate is_good_name().
v3: move it as first patch.
v4: no change
v5: add Acked-by tag
v6: keep is_good_name() check for group and event name
    add trace_probe_log_set_index(1) to report correct error message.

 kernel/trace/trace_eprobe.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c
b/kernel/trace/trace_eprobe.c
index 7d44785..8979cb9e 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -881,13 +881,12 @@ static int __trace_eprobe_create(int argc,
const char *argv[])
        if (!is_good_name(event) || !is_good_name(group))
                goto parse_error;
+       trace_probe_log_set_index(1);
Is this something that you noticed missing in the original code and are
adding now? If so, please make this a separate patch. Or is this
something that's needed for the new 'generating event name' code? If
that's the case, please move this to the other patch.

This one should only contain the code related to the duplicate
is_good_name() removal mentioned in the subject. Or if this really
does belong here, please provide more explanation of why it's needed if
you remove the duplicate is_good_name() code.
thanks, the original code should have this setting, i will create a separate patch.

Thanks,

Tom

        sys_event = argv[1];
        ret = traceprobe_parse_event_name(&sys_event, &sys_name,
buf2,
                                          sys_event - argv[1]);
        if (ret || !sys_name)
                goto parse_error;
-       if (!is_good_name(sys_event) || !is_good_name(sys_name))
-               goto parse_error;
        mutex_lock(&event_mutex);
        event_call = find_and_get_event(sys_name, sys_event);