Re: [PATCH v3 3/3] selftests/ftrace: add test case for GRP/ only input

[Linyu Yuan]

hi Masami,

thanks for review, will update your comment in next V4.

thanks

On 6/1/2022 10:57 PM, Masami Hiramatsu (Google) wrote:
> Hi Linyu,
>
> On Tue, 31 May 2022 14:29:12 +0800
> Linyu Yuan <quic_linyyuan@xxxxxxxxxxx> wrote:
>
> > add kprobe and eprobe event test for new GRP/ only format.
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> > ---
> > v3: first add in this version
> >
> >   .../testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc  | 7 ++++++-
> >   .../testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc  | 7 ++++++-
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
> > index 60c02b4..97856df 100644
> > --- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
> > @@ -1,7 +1,7 @@
> >   #!/bin/sh
> >   # SPDX-License-Identifier: GPL-2.0
> >   # description: Generic dynamic event - add/remove eprobe events
> > -# requires: dynamic_events events/syscalls/sys_enter_openat "e[:[<group>/]<event>] <attached-group>.<attached-event> [<args>]":README
> > +# requires: dynamic_events events/syscalls/sys_enter_openat "e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]":README
> Hmm, this is too restricted. With this change, this entire test only runs after
> patching this series.
> Can you change the requires so that skip checking the first half, like
>    "<attached-group>.<attached-event> [<args>]":README
> and check whether "e[:[<group>/][<event>]]" is in the README for the new
> test code?
>   
> >   echo 0 > events/enable
> >   
> > @@ -87,4 +87,9 @@ echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
> >   ! grep -q "$EPROBE" dynamic_events
> >   ! test -d events/eprobes/$EPROBE
> >   
> > +echo "e:mygroup/ $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
> > +test -d events/mygroup
> > +echo "-:mygroup/" >> dynamic_events
> > +! test -d events/mygroup
> > +
> >   clear_trace
> > diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> > index b4da41d..6b102a7 100644
> > --- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> > @@ -1,7 +1,7 @@
> >   #!/bin/sh
> >   # SPDX-License-Identifier: GPL-2.0
> >   # description: Generic dynamic event - add/remove kprobe events
> > -# requires: dynamic_events "place: [<module>:]<symbol>":README "place (kretprobe): [<module>:]<symbol>":README
> > +# requires: dynamic_events "p[:[<group>/][<event>]] <place> [<args>]":README
> Ditto. In this case, don't touch the requires line (because it is not
> updated) and just add a README check for below new code.
>
> Thank you,
>
>
> >   
> >   echo 0 > events/enable
> >   echo > dynamic_events
> > @@ -23,4 +23,9 @@ grep -q myevent1 dynamic_events
> >   
> >   echo > dynamic_events
> >   
> > +echo "p:mygroup/ $PLACE" >> dynamic_events
> > +test -d events/mygroup
> > +echo "-:mygroup/" >> dynamic_events
> > +! test -d events/mygroup
> > +
> >   clear_trace
> > --
> > 2.7.4