Re: [PATCH] tracepoints: prevents null probe from being added

[Mathieu Desnoyers]

* Keun-O Park (kpark3469@xxxxxxxxx) wrote:
> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
> <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
> > * Keun-O Park (kpark3469@xxxxxxxxx) wrote:
> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
> >> >> * Steven Rostedt (rostedt@xxxxxxxxxxx) wrote:
> >> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3469@xxxxxxxxx wrote:
> >> >> > > From: Sahara <keun-o.park@xxxxxxxxxxxxx>
> >> >> > >
> >> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe
> >> >> > > function.
> >> >> >
> >> >> > You actually hit this in practice, or is this just something that you
> >> >> > observe from code review?
> >> >> >
> >> >> > >  Especially on getting a null probe in remove function, it seems
> >> >> > > to be used to remove all probe functions in the entry.
> >> >> >
> >> >> > Hmm, that actually sounds like a feature.
> >> >>
> >> >> Yep. It's been a long time since I wrote this code, but the removal code
> >> >> seems to use NULL probe pointer to remove all probes for a given
> >> >> tracepoint.
> >> >>
> >> >> I'd be tempted to just validate non-NULL probe within
> >> >> tracepoint_entry_add_probe() and let other sites as is, just in case
> >> >> anyone would be using this feature.
> >> >>
> >> >> I cannot say that I have personally used this "remove all" feature much
> >> >> though.
> >> >>
> >> >
> >> > I agree. I don't see anything wrong in leaving the null probe feature in
> >> > the removal code. But updating the add code looks like a proper change.
> >> >
> >> > -- Steve
> >> >
> >> >
> >>
> >> Hello Steve & Mathieu,
> >> If we want to leave the null probe feature enabled, I think it would
> >> be better modifying the code like the following for code efficiency.
> >>
> >> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> >>         int nr_probes = 0;
> >>         struct tracepoint_func *old, *new;
> >>
> >> -       WARN_ON(!probe);
> >> +       if (WARN_ON(!probe))
> >> +               return ERR_PTR(-EINVAL);
> >>
> >>         debug_print_probes(entry);
> >>         old = entry->funcs;
> >> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent
> >>
> >>         debug_print_probes(entry);
> >>         /* (N -> M), (N > 1, M >= 0) probes */
> >> -       for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> >> -               if (!probe ||
> >> -                   (old[nr_probes].func == probe &&
> >> -                    old[nr_probes].data == data))
> >> -                       nr_del++;
> >> +       if (probe) {
> >> +               for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> >> +                       if (old[nr_probes].func == probe &&
> >> +                            old[nr_probes].data == data)
> >> +                               nr_del++;
> >> +               }
> >>         }
> >>
> >> -       if (nr_probes - nr_del == 0) {
> >> +       if (!probe || nr_probes - nr_del == 0) {
> >
> > We might want to do:
> >
> > if (probe) {
> >   ...
> > } else {
> >   nr_del = nr_probes;
> > }
> >
> > if (nr_probes - nr_del == 0) {
> >    ...
> > }
> 
> This code has a problem.
> nr_probes is initialized as zero.

yes,

> And, in order to get correct count of probes,
> we need to go through the for-loop even though probe is null.
> So with above code, nr_del will be zero. Anyhow, the code will fall
> through if-clause(nr_probes-nr_del==0).
> It looks odd to me.

Ah, I see what you mean: the nr_del = nr_probes assignment is useless,
because both nr_probes and nr_del are equal to 0. So we could go for:

        if (probe) {
                for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
                        if (old[nr_probes].func == probe &&
                             old[nr_probes].data == data)
                                nr_del++;
                }
        }

        if (nr_probes - nr_del == 0) {
                ...
        } else {
                ...
        }

Does it look better ?

Thanks,

Mathieu

> 
> -- Kpark
> 
> >
> > rather than:
> >
> > if (probe) {
> >   ...
> > }
> >
> > if (!probe || nr_probes - nr_del == 0) {
> >    ...
> > }
> >
> > Using nr_del makes the code easier to follow IMHO.
> >
> > Thanks,
> >
> > Mathieu
> >

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/