Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

[Mathieu Desnoyers]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx, "Ingo Molnar" <mingo@xxxxxxxxxx>, "Frederic Weisbecker" <fweisbec@xxxxxxxxx>,
> "Andrew Morton" <akpm@xxxxxxxxxxxxxxxxxxxx>, "Johannes Berg" <johannes.berg@xxxxxxxxx>
> Sent: Monday, March 10, 2014 4:19:27 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Mon, 10 Mar 2014 20:01:34 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>  
> > >  	mutex_lock(&tracepoints_mutex);
> > >  	old = tracepoint_add_probe(name, probe, data);
> > > @@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
> > > *probe, void *data)
> > >  		return PTR_ERR(old);
> > >  	}
> > >  	tracepoint_update_probes();		/* may update entry */
> > > +	entry = get_tracepoint(name);
> > > +	/* Make sure the entry was enabled */
> > > +	if (!entry || !entry->enabled)
> > > +		ret = -ENODEV;
> > 
> > Hi Steven,
> > 
> > Returning -ENODEV when the probe is still registered might come as a
> > surprise to the caller. For instance, a caller may dynamically allocate
> > name, probe, and/or data, it may want to free them when
> > tracepoint_probe_register returns an error. But this "-ENODEV" return value
> > is not really an error, and the parameters passed are still used.
> 
> It's an error when you wanted to enable a probe and the probe doesn't
> exist. There are no in tree users of this call that expect it to work
> when the probe does not exist.
> 
> > 
> > If we go down this route, we might want at the very least to add
> > documentation
> > of tracepoint_probe_register() return values and their meaning
> > in a comment on top of this function (perhaps also in the header). But
> > even if we do so, this weird return value semantic with respect to use of
> > the
> > received parameters will likely cause memory corruption at some point.
> > 
> > Thoughts ?
> 
> Send a patch to document the return values. Your module can expect this
> return value when it doesn't expect the probe to exist.
> 
> Again, it's really strange when you go to enable a probe, and there is
> no probe to enable.
> 
> Note, I was nice. I removed the logic to unregister the probe in this
> case.
> 
> Anyway, this should even help you. Before there was no way to enable a
> probe and know if it was enabled or not. That is, if it didn't exist,
> there was no feedback letting you know that.
> 
> If you expect to enable a probe that doesn't exist, then you can expect
> this return value.

I'm OK with this change. I was merely pointing out that we need to document
this return value, since its semantic differs from what is usually expected.

I'll prepare a patch soon to document the return value.

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/