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

[Steven Rostedt]

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.

-- Steve
--
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/