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

[Keun-O Park]

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. 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.

-- Kpark

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