selftests/kprobe: Do not test for GRP/ without event failures

[PATCH] selftests/kprobe: Do not test for GRP/ without event failures

Posted by Steven Rostedt 3 years, 9 months ago

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

A new feature is added where kprobes (and other probes) do not need to
explicitly state the event name when creating a probe. The event name will
come from what is being attached.

That is:

  # echo 'p:foo/ vfs_read' > kprobe_events

Will no longer error, but instead create an event:

  # cat kprobe_events
 p:foo/p_vfs_read_0 vfs_read

This should not be tested as an error case anymore. Remove it from the
selftest as now this feature "breaks" the selftest as it no longer fails
as expected.

Link: https://lore.kernel.org/all/1656296348-16111-1-git-send-email-quic_linyyuan@quicinc.com/

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc       | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index fa928b431555..7c02509c71d0 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -21,7 +21,6 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
 check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
 
 check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
-check_error 'p:foo/^ vfs_read'		# NO_EVENT_NAME
 check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
 check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
 
-- 
2.35.1

Re: [PATCH] selftests/kprobe: Do not test for GRP/ without event failures

Posted by Masami Hiramatsu (Google) 3 years, 9 months ago

On Tue, 12 Jul 2022 16:17:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> A new feature is added where kprobes (and other probes) do not need to
> explicitly state the event name when creating a probe. The event name will
> come from what is being attached.
> 
> That is:
> 
>   # echo 'p:foo/ vfs_read' > kprobe_events
> 
> Will no longer error, but instead create an event:
> 
>   # cat kprobe_events
>  p:foo/p_vfs_read_0 vfs_read
> 
> This should not be tested as an error case anymore. Remove it from the
> selftest as now this feature "breaks" the selftest as it no longer fails
> as expected.

Good catch!

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

BTW, in this case, NO_EVENT_NAME error should not happen anymore.
Let me cleanup the code.

Thank you,

> 
> Link: https://lore.kernel.org/all/1656296348-16111-1-git-send-email-quic_linyyuan@quicinc.com/
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc       | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> index fa928b431555..7c02509c71d0 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> @@ -21,7 +21,6 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
>  check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
>  
>  check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
> -check_error 'p:foo/^ vfs_read'		# NO_EVENT_NAME
>  check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
>  check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
>  
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] selftests/kprobe: Do not test for GRP/ without event failures

Posted by Masami Hiramatsu (Google) 3 years, 9 months ago

On Mon, 18 Jul 2022 11:08:53 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Tue, 12 Jul 2022 16:17:07 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > A new feature is added where kprobes (and other probes) do not need to
> > explicitly state the event name when creating a probe. The event name will
> > come from what is being attached.
> > 
> > That is:
> > 
> >   # echo 'p:foo/ vfs_read' > kprobe_events
> > 
> > Will no longer error, but instead create an event:
> > 
> >   # cat kprobe_events
> >  p:foo/p_vfs_read_0 vfs_read
> > 
> > This should not be tested as an error case anymore. Remove it from the
> > selftest as now this feature "breaks" the selftest as it no longer fails
> > as expected.
> 
> Good catch!
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> BTW, in this case, NO_EVENT_NAME error should not happen anymore.
> Let me cleanup the code.

Oops, no. There is an error case of NO_EVENT_NAME. 'p: vfs_read' will cause
this error because it expects an event name after ':'. Thus, the correct
fix is just removing "foo/":

check_error 'p:^ vfs_read'		# NO_EVENT_NAME

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH] selftests/kprobe: Update test for no event name syntax error

Posted by Masami Hiramatsu (Google) 3 years, 9 months ago

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

The commit 208003254c32 ("selftests/kprobe: Do not test for GRP/
without event failures") removed a syntax which is no more cause
a syntax error (NO_EVENT_NAME error with GRP/).
However, there are another case (NO_EVENT_NAME error without GRP/)
which causes a same error. This adds a test for that case.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc   |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index 7c02509c71d0..9e85d3019ff0 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -21,6 +21,7 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
 check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
 
 check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
+check_error 'p:^ vfs_read'		# NO_EVENT_NAME
 check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
 check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME

Re: [PATCH] selftests/kprobe: Update test for no event name syntax error

Posted by Steven Rostedt 3 years, 9 months ago

On Mon, 18 Jul 2022 16:05:10 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> The commit 208003254c32 ("selftests/kprobe: Do not test for GRP/
> without event failures") removed a syntax which is no more cause
> a syntax error (NO_EVENT_NAME error with GRP/).
> However, there are another case (NO_EVENT_NAME error without GRP/)
> which causes a same error. This adds a test for that case.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Queued. Thanks Masami!

-- Steve

> ---
>  .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc   |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> index 7c02509c71d0..9e85d3019ff0 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> @@ -21,6 +21,7 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
>  check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
>  
>  check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
> +check_error 'p:^ vfs_read'		# NO_EVENT_NAME
>  check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
>  check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
>

Re: [PATCH] selftests/kprobe: Update test for no event name syntax error

Posted by Linyu Yuan 3 years, 9 months ago

hi Masami,

On 7/18/2022 3:05 PM, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> The commit 208003254c32 ("selftests/kprobe: Do not test for GRP/
> without event failures") removed a syntax which is no more cause
> a syntax error (NO_EVENT_NAME error with GRP/).
> However, there are another case (NO_EVENT_NAME error without GRP/)
> which causes a same error. This adds a test for that case.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>   .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc   |    1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> index 7c02509c71d0..9e85d3019ff0 100644
> --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> @@ -21,6 +21,7 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
>   check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
>   
>   check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
> +check_error 'p:^ vfs_read'		# NO_EVENT_NAME

i think you fix the issue which exist from start, right ?

is there better comment than NO_EVENT_NAME  ?

>   check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
>   check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
>   
>

Re: [PATCH] selftests/kprobe: Update test for no event name syntax error

Posted by Masami Hiramatsu (Google) 3 years, 9 months ago

On Mon, 18 Jul 2022 17:36:43 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

> hi Masami,
> 
> On 7/18/2022 3:05 PM, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > The commit 208003254c32 ("selftests/kprobe: Do not test for GRP/
> > without event failures") removed a syntax which is no more cause
> > a syntax error (NO_EVENT_NAME error with GRP/).
> > However, there are another case (NO_EVENT_NAME error without GRP/)
> > which causes a same error. This adds a test for that case.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >   .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc   |    1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> > index 7c02509c71d0..9e85d3019ff0 100644
> > --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
> > @@ -21,6 +21,7 @@ check_error 'p:^/bar vfs_read'		# NO_GROUP_NAME
> >   check_error 'p:^12345678901234567890123456789012345678901234567890123456789012345/bar vfs_read'	# GROUP_TOO_LONG
> >   
> >   check_error 'p:^foo.1/bar vfs_read'	# BAD_GROUP_NAME
> > +check_error 'p:^ vfs_read'		# NO_EVENT_NAME
> 
> i think you fix the issue which exist from start, right ?

Yes, this is not a new bug but the error case which still
exists.

> 
> is there better comment than NO_EVENT_NAME  ?

These comments are corresponding to the error name, so that we can
find the logging code easily. (Not for users)

Thank you,

> 
> >   check_error 'p:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
> >   check_error 'p:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
> >   
> >


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>