From: Sahara <[email protected]>
Somehow tracepoint_entry_add/remove_probe functions allow a null probe
function. Especially on getting a null probe in remove function, it seems
to be used to remove all probe functions in the entry.
But, the code is not handled as expected. Since the tracepoint_entry
maintains funcs array's last func as NULL in order to mark it as the end
of the array. Also NULL func is used in for-loop to check out the end of
the loop. So if there's NULL func in the entry's funcs, the for-loop
will be abruptly ended in the middle of operation.
Also checking out if probe is null in for-loop is not efficient.
Signed-off-by: Sahara <[email protected]>
---
kernel/tracepoint.c | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 0c05a45..30f427e 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -112,7 +112,10 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
int nr_probes = 0;
struct tracepoint_func *old, *new;
- WARN_ON(!probe);
+ if (unlikely(!probe)) {
+ WARN_ON(!probe);
+ return ERR_PTR(-EINVAL);
+ }
debug_print_probes(entry);
old = entry->funcs;
@@ -147,15 +150,19 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
old = entry->funcs;
+ if (unlikely(!probe)) {
+ WARN_ON(!probe);
+ return ERR_PTR(-EINVAL);
+ }
+
if (!old)
return ERR_PTR(-ENOENT);
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))
+ if (old[nr_probes].func == probe &&
+ old[nr_probes].data == data)
nr_del++;
}
@@ -173,8 +180,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
if (new == NULL)
return ERR_PTR(-ENOMEM);
for (i = 0; old[i].func; i++)
- if (probe &&
- (old[i].func != probe || old[i].data != data))
+ if (old[i].func != probe || old[i].data != data)
new[j++] = old[i];
new[nr_probes - nr_del].func = NULL;
entry->refcount = nr_probes - nr_del;
--
1.7.1
On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
> From: Sahara <[email protected]>
>
> 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.
> But, the code is not handled as expected. Since the tracepoint_entry
> maintains funcs array's last func as NULL in order to mark it as the end
> of the array. Also NULL func is used in for-loop to check out the end of
> the loop. So if there's NULL func in the entry's funcs, the for-loop
> will be abruptly ended in the middle of operation.
> Also checking out if probe is null in for-loop is not efficient.
>
> Signed-off-by: Sahara <[email protected]>
> ---
> kernel/tracepoint.c | 18 ++++++++++++------
> 1 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0c05a45..30f427e 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -112,7 +112,10 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> int nr_probes = 0;
> struct tracepoint_func *old, *new;
>
> - WARN_ON(!probe);
> + if (unlikely(!probe)) {
> + WARN_ON(!probe);
> + return ERR_PTR(-EINVAL);
> + }
Um, you want:
if (WARN_ON(!probe))
return ERR_PTR(-EINVAL);
>
> debug_print_probes(entry);
> old = entry->funcs;
> @@ -147,15 +150,19 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
>
> old = entry->funcs;
>
> + if (unlikely(!probe)) {
> + WARN_ON(!probe);
> + return ERR_PTR(-EINVAL);
> + }
Here too if it wasn't intended to allow removal of all probes from a
tracepoint.
> +
> if (!old)
> return ERR_PTR(-ENOENT);
>
> 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))
> + if (old[nr_probes].func == probe &&
> + old[nr_probes].data == data)
> nr_del++;
> }
>
> @@ -173,8 +180,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> for (i = 0; old[i].func; i++)
> - if (probe &&
> - (old[i].func != probe || old[i].data != data))
> + if (old[i].func != probe || old[i].data != data)
This makes it look like the null probe was intentional.
-- Steve
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> entry->refcount = nr_probes - nr_del;
* Steven Rostedt ([email protected]) wrote:
> On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
> > From: Sahara <[email protected]>
> >
> > 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.
Thanks,
Mathieu
>
> > But, the code is not handled as expected. Since the tracepoint_entry
> > maintains funcs array's last func as NULL in order to mark it as the end
> > of the array. Also NULL func is used in for-loop to check out the end of
> > the loop. So if there's NULL func in the entry's funcs, the for-loop
> > will be abruptly ended in the middle of operation.
> > Also checking out if probe is null in for-loop is not efficient.
> >
> > Signed-off-by: Sahara <[email protected]>
> > ---
> > kernel/tracepoint.c | 18 ++++++++++++------
> > 1 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 0c05a45..30f427e 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -112,7 +112,10 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> > int nr_probes = 0;
> > struct tracepoint_func *old, *new;
> >
> > - WARN_ON(!probe);
> > + if (unlikely(!probe)) {
> > + WARN_ON(!probe);
> > + return ERR_PTR(-EINVAL);
> > + }
>
> Um, you want:
>
> if (WARN_ON(!probe))
> return ERR_PTR(-EINVAL);
>
> >
> > debug_print_probes(entry);
> > old = entry->funcs;
> > @@ -147,15 +150,19 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
> >
> > old = entry->funcs;
> >
> > + if (unlikely(!probe)) {
> > + WARN_ON(!probe);
> > + return ERR_PTR(-EINVAL);
> > + }
>
> Here too if it wasn't intended to allow removal of all probes from a
> tracepoint.
>
> > +
> > if (!old)
> > return ERR_PTR(-ENOENT);
> >
> > 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))
> > + if (old[nr_probes].func == probe &&
> > + old[nr_probes].data == data)
> > nr_del++;
> > }
> >
> > @@ -173,8 +180,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
> > if (new == NULL)
> > return ERR_PTR(-ENOMEM);
> > for (i = 0; old[i].func; i++)
> > - if (probe &&
> > - (old[i].func != probe || old[i].data != data))
> > + if (old[i].func != probe || old[i].data != data)
>
> This makes it look like the null probe was intentional.
>
> -- Steve
>
> > new[j++] = old[i];
> > new[nr_probes - nr_del].func = NULL;
> > entry->refcount = nr_probes - nr_del;
>
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt ([email protected]) wrote:
> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
> > > From: Sahara <[email protected]>
> > >
> > > 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
On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
> On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> * Steven Rostedt ([email protected]) wrote:
>> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
>> > > From: Sahara <[email protected]>
>> > >
>> > > 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) {
/* N -> 0, (N > 1) */
entry->funcs = NULL;
entry->refcount = 0;
Because we know handing over the null probe to
tracepoint_entry_add_probe is not possible,
we don't have to check if the probe is null or not within for loop. If
the probe is null, it's just enough to add !probe in
'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
for-loop, falling through for-loop can be prevented when probe is
null.
@@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry
if (new == NULL)
return ERR_PTR(-ENOMEM);
for (i = 0; old[i].func; i++)
- if (probe &&
- (old[i].func != probe || old[i].data != data))
+ if (old[i].func != probe || old[i].data != data)
new[j++] = old[i];
new[nr_probes - nr_del].func = NULL;
entry->refcount = nr_probes - nr_del;
We don't have to check the probe here too. We know probe is always true here.
Thanks.
-- Kpark
On Thu, Mar 21, 2013 at 10:39 AM, Keun-O Park <[email protected]> wrote:
> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
>> On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>>> * Steven Rostedt ([email protected]) wrote:
>>> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
>>> > > From: Sahara <[email protected]>
>>> > >
>>> > > 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) {
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> entry->refcount = 0;
>
> Because we know handing over the null probe to
> tracepoint_entry_add_probe is not possible,
> we don't have to check if the probe is null or not within for loop. If
Hmm. I described this wrong. :-(
For code efficiency, I replaced 'checking null probe in every
iteration within for-loop' with 'checking once outside the loop'.
> the probe is null, it's just enough to add !probe in
> 'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
> for-loop, falling through for-loop can be prevented when probe is
> null.
>
> @@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> for (i = 0; old[i].func; i++)
> - if (probe &&
> - (old[i].func != probe || old[i].data != data))
> + if (old[i].func != probe || old[i].data != data)
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> entry->refcount = nr_probes - nr_del;
>
> We don't have to check the probe here too. We know probe is always true here.
> Thanks.
>
> -- Kpark
* Keun-O Park ([email protected]) wrote:
> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
> >> * Steven Rostedt ([email protected]) wrote:
> >> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
> >> > > From: Sahara <[email protected]>
> >> > >
> >> > > 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) {
...
}
rather than:
if (probe) {
...
}
if (!probe || nr_probes - nr_del == 0) {
...
}
Using nr_del makes the code easier to follow IMHO.
Thanks,
Mathieu
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> entry->refcount = 0;
>
> Because we know handing over the null probe to
> tracepoint_entry_add_probe is not possible,
> we don't have to check if the probe is null or not within for loop. If
> the probe is null, it's just enough to add !probe in
> 'if(nr_probes-nr_del==0)'. And, with additional if-clause wrapping
> for-loop, falling through for-loop can be prevented when probe is
> null.
>
> @@ -173,8 +172,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry
> if (new == NULL)
> return ERR_PTR(-ENOMEM);
> for (i = 0; old[i].func; i++)
> - if (probe &&
> - (old[i].func != probe || old[i].data != data))
> + if (old[i].func != probe || old[i].data != data)
> new[j++] = old[i];
> new[nr_probes - nr_del].func = NULL;
> entry->refcount = nr_probes - nr_del;
>
> We don't have to check the probe here too. We know probe is always true here.
> Thanks.
>
> -- Kpark
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
<[email protected]> wrote:
> * Keun-O Park ([email protected]) wrote:
>> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
>> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> >> * Steven Rostedt ([email protected]) wrote:
>> >> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
>> >> > > From: Sahara <[email protected]>
>> >> > >
>> >> > > 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
>
* Keun-O Park ([email protected]) wrote:
> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
> <[email protected]> wrote:
> > * Keun-O Park ([email protected]) wrote:
> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
> >> >> * Steven Rostedt ([email protected]) wrote:
> >> >> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
> >> >> > > From: Sahara <[email protected]>
> >> >> > >
> >> >> > > 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
On Thu, Mar 21, 2013 at 12:33 PM, Mathieu Desnoyers
<[email protected]> wrote:
> * Keun-O Park ([email protected]) wrote:
>> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
>> <[email protected]> wrote:
>> > * Keun-O Park ([email protected]) wrote:
>> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <[email protected]> wrote:
>> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> >> >> * Steven Rostedt ([email protected]) wrote:
>> >> >> > On Wed, 2013-03-20 at 12:18 +0900, [email protected] wrote:
>> >> >> > > From: Sahara <[email protected]>
>> >> >> > >
>> >> >> > > 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
Yes, it does, only if you don't think this code is hard to follow. ;-)
Thanks.
-- Kpark