| Message ID | 1488792891-57629-1-git-send-email-wangkefeng.wang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
+ Arnaldo Carvalho de Melo <acme@kernel.org> On 2017/3/6 17:34, Kefeng Wang wrote: > On old perf, when using perf probe -d to delete an inexistent event, > it return errno, eg, > > -bash-4.3# perf probe -d xxx || echo $? > Info: Event "*:xxx" does not exist. > Error: Failed to delete events. > 255 > > But now perf_del_probe_events() will always set ret = 0, different > from previous del_perf_probe_events(). After this, it return errno > again, eg, > > -bash-4.3# ./perf probe -d xxx || echo $? > Error: Failed to delete events. > 254 > > And it is more appropriate to return -ENOENT instead of -EPERM. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > tools/perf/builtin-probe.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > index 1fcebc3..c46b41c 100644 > --- a/tools/perf/builtin-probe.c > +++ b/tools/perf/builtin-probe.c > @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) > if (ret == -ENOENT && ret2 == -ENOENT) > pr_debug("\"%s\" does not hit any event.\n", str); > /* Note that this is silently ignored */ > - ret = 0; > + else > + ret = 0; > > error: > if (kfd >= 0) >
Hi all, any comments, thanks. On 2017/3/7 15:33, Kefeng Wang wrote: > + Arnaldo Carvalho de Melo <acme@kernel.org> > > On 2017/3/6 17:34, Kefeng Wang wrote: >> On old perf, when using perf probe -d to delete an inexistent event, >> it return errno, eg, >> >> -bash-4.3# perf probe -d xxx || echo $? >> Info: Event "*:xxx" does not exist. >> Error: Failed to delete events. >> 255 >> >> But now perf_del_probe_events() will always set ret = 0, different >> from previous del_perf_probe_events(). After this, it return errno >> again, eg, >> >> -bash-4.3# ./perf probe -d xxx || echo $? >> Error: Failed to delete events. >> 254 >> >> And it is more appropriate to return -ENOENT instead of -EPERM. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> tools/perf/builtin-probe.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c >> index 1fcebc3..c46b41c 100644 >> --- a/tools/perf/builtin-probe.c >> +++ b/tools/perf/builtin-probe.c >> @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) >> if (ret == -ENOENT && ret2 == -ENOENT) >> pr_debug("\"%s\" does not hit any event.\n", str); >> /* Note that this is silently ignored */ >> - ret = 0; >> + else >> + ret = 0; >> >> error: >> if (kfd >= 0) >> > > > . >
Em Tue, Mar 14, 2017 at 09:19:47PM +0800, Kefeng Wang escreveu: > Hi all, any comments, thanks. For 'perf probe' make sure Masami is in the CC list, adding him, Masami? - Arnaldo > On 2017/3/7 15:33, Kefeng Wang wrote: > > + Arnaldo Carvalho de Melo <acme@kernel.org> > > > > On 2017/3/6 17:34, Kefeng Wang wrote: > >> On old perf, when using perf probe -d to delete an inexistent event, > >> it return errno, eg, > >> > >> -bash-4.3# perf probe -d xxx || echo $? > >> Info: Event "*:xxx" does not exist. > >> Error: Failed to delete events. > >> 255 > >> > >> But now perf_del_probe_events() will always set ret = 0, different > >> from previous del_perf_probe_events(). After this, it return errno > >> again, eg, > >> > >> -bash-4.3# ./perf probe -d xxx || echo $? > >> Error: Failed to delete events. > >> 254 > >> > >> And it is more appropriate to return -ENOENT instead of -EPERM. > >> > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >> --- > >> tools/perf/builtin-probe.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > >> index 1fcebc3..c46b41c 100644 > >> --- a/tools/perf/builtin-probe.c > >> +++ b/tools/perf/builtin-probe.c > >> @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) > >> if (ret == -ENOENT && ret2 == -ENOENT) > >> pr_debug("\"%s\" does not hit any event.\n", str); > >> /* Note that this is silently ignored */ > >> - ret = 0; > >> + else > >> + ret = 0; > >> > >> error: > >> if (kfd >= 0) > >> > > > > > > . > >
On Tue, 14 Mar 2017 10:30:56 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > Em Tue, Mar 14, 2017 at 09:19:47PM +0800, Kefeng Wang escreveu: > > Hi all, any comments, thanks. > > For 'perf probe' make sure Masami is in the CC list, adding him, Masami? Thanks Arnaldo, Hi Kefeng, I've made the error ignored by design, since user might pass the wildcard pattern to perf probe -d just for making sure. You can check the pattern hits any event by using perf-probe --list PATTERN. Would you have any use case to check the result? Thank you, > > - Arnaldo > > > On 2017/3/7 15:33, Kefeng Wang wrote: > > > + Arnaldo Carvalho de Melo <acme@kernel.org> > > > > > > On 2017/3/6 17:34, Kefeng Wang wrote: > > >> On old perf, when using perf probe -d to delete an inexistent event, > > >> it return errno, eg, > > >> > > >> -bash-4.3# perf probe -d xxx || echo $? > > >> Info: Event "*:xxx" does not exist. > > >> Error: Failed to delete events. > > >> 255 > > >> > > >> But now perf_del_probe_events() will always set ret = 0, different > > >> from previous del_perf_probe_events(). After this, it return errno > > >> again, eg, > > >> > > >> -bash-4.3# ./perf probe -d xxx || echo $? > > >> Error: Failed to delete events. > > >> 254 > > >> > > >> And it is more appropriate to return -ENOENT instead of -EPERM. > > >> > > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > >> --- > > >> tools/perf/builtin-probe.c | 3 ++- > > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > > >> index 1fcebc3..c46b41c 100644 > > >> --- a/tools/perf/builtin-probe.c > > >> +++ b/tools/perf/builtin-probe.c > > >> @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) > > >> if (ret == -ENOENT && ret2 == -ENOENT) > > >> pr_debug("\"%s\" does not hit any event.\n", str); > > >> /* Note that this is silently ignored */ > > >> - ret = 0; > > >> + else > > >> + ret = 0; > > >> > > >> error: > > >> if (kfd >= 0) > > >> > > > > > > > > > . > > > -- Masami Hiramatsu <mhiramat@kernel.org>
On 2017/3/16 17:39, Masami Hiramatsu wrote: > On Tue, 14 Mar 2017 10:30:56 -0300 > Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > >> Em Tue, Mar 14, 2017 at 09:19:47PM +0800, Kefeng Wang escreveu: >>> Hi all, any comments, thanks. >> >> For 'perf probe' make sure Masami is in the CC list, adding him, Masami? > > Thanks Arnaldo, > > Hi Kefeng, I've made the error ignored by design, since user might pass > the wildcard pattern to perf probe -d just for making sure. > > You can check the pattern hits any event by using perf-probe --list PATTERN. > Would you have any use case to check the result? There are some perf test cases from our inner tester, they will check the returned value for each execution, and they think return errno is better to cater for users when delete inexistent events, as it was before. And the patch only returns error when does not hit any event. If such behavior, error ignored is more reasonableo< I will push them to modify the test cases. Thanks, Kefeng > > Thank you, > >> >> - Arnaldo >> >>> On 2017/3/7 15:33, Kefeng Wang wrote: >>>> + Arnaldo Carvalho de Melo <acme@kernel.org> >>>> >>>> On 2017/3/6 17:34, Kefeng Wang wrote: >>>>> On old perf, when using perf probe -d to delete an inexistent event, >>>>> it return errno, eg, >>>>> >>>>> -bash-4.3# perf probe -d xxx || echo $? >>>>> Info: Event "*:xxx" does not exist. >>>>> Error: Failed to delete events. >>>>> 255 >>>>> >>>>> But now perf_del_probe_events() will always set ret = 0, different >>>>> from previous del_perf_probe_events(). After this, it return errno >>>>> again, eg, >>>>> >>>>> -bash-4.3# ./perf probe -d xxx || echo $? >>>>> Error: Failed to delete events. >>>>> 254 >>>>> >>>>> And it is more appropriate to return -ENOENT instead of -EPERM. >>>>> >>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>>> --- >>>>> tools/perf/builtin-probe.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c >>>>> index 1fcebc3..c46b41c 100644 >>>>> --- a/tools/perf/builtin-probe.c >>>>> +++ b/tools/perf/builtin-probe.c >>>>> @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) >>>>> if (ret == -ENOENT && ret2 == -ENOENT) >>>>> pr_debug("\"%s\" does not hit any event.\n", str); >>>>> /* Note that this is silently ignored */ >>>>> - ret = 0; >>>>> + else >>>>> + ret = 0; >>>>> >>>>> error: >>>>> if (kfd >= 0) >>>>> >>>> >>>> >>>> . >>>> > >
On Thu, 16 Mar 2017 20:07:08 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > On 2017/3/16 17:39, Masami Hiramatsu wrote: > > On Tue, 14 Mar 2017 10:30:56 -0300 > > Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > > >> Em Tue, Mar 14, 2017 at 09:19:47PM +0800, Kefeng Wang escreveu: > >>> Hi all, any comments, thanks. > >> > >> For 'perf probe' make sure Masami is in the CC list, adding him, Masami? > > > > Thanks Arnaldo, > > > > Hi Kefeng, I've made the error ignored by design, since user might pass > > the wildcard pattern to perf probe -d just for making sure. > > > > You can check the pattern hits any event by using perf-probe --list PATTERN. > > Would you have any use case to check the result? > > There are some perf test cases from our inner tester, they will check > the returned value for each execution, and they think return errno is > better to cater for users when delete inexistent events, as it was before. > And the patch only returns error when does not hit any event. > > If such behavior, error ignored is more reasonableo< I will push them to > modify the test cases. Hmm, the commit dddc7ee32fa1 ("perf probe: Fix an error when deleting probes successfully") introduced this behavior. But I think this fix will not be the best solution. OK, so could you update your patch according my comment and resend it? > >>>>> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c > >>>>> index 1fcebc3..c46b41c 100644 > >>>>> --- a/tools/perf/builtin-probe.c > >>>>> +++ b/tools/perf/builtin-probe.c > >>>>> @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) > >>>>> if (ret == -ENOENT && ret2 == -ENOENT) > >>>>> pr_debug("\"%s\" does not hit any event.\n", str); This should be pr_warning if this case has an error. > >>>>> /* Note that this is silently ignored */ This comment should be removed too, since this error is not ignored anymore. > >>>>> - ret = 0; > >>>>> + else > >>>>> + ret = 0; > >>>>> With these patches, perf probe shows an error if there is no event hit for --delete. $ sudo ./perf probe -d hoge "hoge" does not hit any event. Error: Failed to delete events. Thanks, > >>>>> error: > >>>>> if (kfd >= 0) > >>>>> > >>>> > >>>> > >>>> . > >>>> > > > > > -- Masami Hiramatsu <mhiramat@kernel.org>
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c index 1fcebc3..c46b41c 100644 --- a/tools/perf/builtin-probe.c +++ b/tools/perf/builtin-probe.c @@ -444,7 +444,8 @@ static int perf_del_probe_events(struct strfilter *filter) if (ret == -ENOENT && ret2 == -ENOENT) pr_debug("\"%s\" does not hit any event.\n", str); /* Note that this is silently ignored */ - ret = 0; + else + ret = 0; error: if (kfd >= 0)
On old perf, when using perf probe -d to delete an inexistent event, it return errno, eg, -bash-4.3# perf probe -d xxx || echo $? Info: Event "*:xxx" does not exist. Error: Failed to delete events. 255 But now perf_del_probe_events() will always set ret = 0, different from previous del_perf_probe_events(). After this, it return errno again, eg, -bash-4.3# ./perf probe -d xxx || echo $? Error: Failed to delete events. 254 And it is more appropriate to return -ENOENT instead of -EPERM. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- tools/perf/builtin-probe.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 1.7.12.4