[DTrace-devel] [PATCH v4 14/25] libcommon: move uprobes creation into the common library

[Nick Alcock]

On 15 Oct 2022, Kris Van Hees outgrape:

> Another comment or two below...
>
> On Sat, Oct 08, 2022 at 02:28:41AM -0400, Kris Van Hees wrote:
>> > > > +#define TRACEFS		"/sys/kernel/debug/tracing/"
>
> This belongs in a header file.
> TRACEFS is already defined in libdtrace/dt_provider.h but that does not really
> help this case.  Most uses of TRACEFS are in libdtrace.
>
> I would suggest adding TRACEFS, KPROBE_EVENTS, and UPROBE_EVENTS to
> include/port.h or putting them in a new include/tracefs.h.

A new tracefs.h is better I think: port.h feels like too much of a
grab-bag already :)

I tried adding them to uprobes.h but that's really not something we want
dt_provider.h to include!

EVENTSFS should move too. (Done.)

>> > > So with no encoding needed, and no differentiator needed, you can get rid of
>> > > this.
>> 
>> Ugh, I stand corrected.  I of course overlooked the case of the module name
>> being a filename (albeit with various limitations), which is a case that does
>> need encoding of course.  Bah.  Provider name and function name should still be
>> regular identifiers as far as I can see, and probe name is an identifier with
>> - allowed (although it seems that . might be allowed as well even though I doubt
>> it actually works).
>> 
>> So, perhaps some encoding is still going to be needed but I would urge you to be
>> quite conservative about it because size limitations on these strings can really
>> get in our way.

For now I've stuck with __XX even though it's a bit space-wasteful,
because it is at least simple. There is at least much *less* encoding
needed than before (no need to encode four :'s per name).

It's easily changeable later on anyway -- and may well not survive the
transition to the daemon writing out a state vector (we could just pass
in an ID of the relevant piece of persistent state).

>> > > > +char *
>> > > > +uprobe_create_named(dev_t dev, ino_t ino, uint64_t addr, const char *spec, int isret,
>> > > > +		    const char *prv, const char *mod, const char *fun,
>> > > > +		    const char *prb)
>> > > > +{
>> > > > +	int fd;
>> > > > +	int rc;
>> > > > +	char *name, *args = "";
>> > > > +
>> > > > +	if (asprintf(&name, "dt_pid/dt_%s%llx_%llx_%lx",
>> > > > +		isret ? "ret_" : "",
>
> Not sure why I didn't see this earlier, but I really think that the dt_pid
> prefix should be a macro so that it can be used here and in the libdtrace
> provider code without being a hardcoded string.  Or perhaps whatever code in
> libdtrace that needs to parse or recognize this string should make use of
> functions in this code rather than there being some hardcoded formats that
> needs to match between the two?

(Fixed by your code introducing uprobe_name() to generate this stuff. Of
course the dt_pid.c machinery still needed to know the format separately
because it specifically wants non-retprobes and is parsing the args too.
Splitting that out would just make it all unreadable, I think.)