----- Original Message ----- > From: "Steven Rostedt" <rost...@goodmis.org> > To: "Mathieu Desnoyers" <mathieu.desnoy...@efficios.com> > Cc: linux-kernel@vger.kernel.org, "Ingo Molnar" <mi...@kernel.org>, "Frederic > Weisbecker" <fweis...@gmail.com>, > "Andrew Morton" <a...@linux-foundation.org>, "Johannes Berg" > <johannes.b...@intel.com> > 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.desnoy...@efficios.com> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/