[DTrace-devel] [PATCH 16/16] PID provider implementation

[Eugene Loh]

On 3/27/21 2:25 AM, Kris Van Hees wrote:

> On Fri, Mar 26, 2021 at 11:30:48PM -0400, Eugene Loh wrote:
>> On 3/19/21 12:55 AM, Kris Van Hees wrote:
>>> diff --git a/libdtrace/dt_prov_pid.c b/libdtrace/dt_prov_pid.c
>>> +	/*
>>> +	 * First check whether this pid probe already exists.  If so, there is
>>> +	 * nothing left to do.
>>> +	 */
>> I would defer that comment for later.  Right now, just say we're
>> constructing the name of the process-specific provider.
> Um, isn't that obvious?  I don't think that warrants a comment.
I don't know if the question is rhetorical.  In case it is not, what I'm 
trying to say is that you start by setting up prv and prb, variables 
that will be used over the lifetime of the function.  In my mind, it's 
fine not to add the comment about what we're doing here;  my main point 
was just to delay the comment about "check whether pid probe exists" 
until after prv and prb have been set up, steps that are not specific to 
that check.
>>> +	snprintf(prv, sizeof(prv), "pid%d", psp->pps_pid);
>>> +
>> And here just say we are constructing the name of the probe, whether for
>> the uprobe or the process-specific probe.
> Um, isn't that obvious?  I don't think that warrants a comment.
Again, fine by me.  My main suggestion was just to defer the "check 
probe exists" comment.
>>> +	 * module is the inode number (in hex), the function name is as
>>> +	 * specified for the pid probe, and the probe name is "entry",
>>> +	 * "return", or the offset into the function (in hex).
>>> +	 */
>>> +	snprintf(mod, sizeof(mod), "%lx", psp->pps_ino);
>>> +
>>> +	/*
>>> +	 * Try to lookup the main (real) probe.  Since multiple pid probes may
>>> +	 * all map onto the same underlying main (real) probe, we may already
>>> +	 * have one in the system.
>> Again "uprobe" rather than "main (real)" probe.
> I'll think of an alternative nomenclature, but it won't be uprobe.
Okay.  Refer to "inode"?  Not a great suggestion, but at least it points 
to this issue of why DTrace's process-specific probes need all these 
machinations.
>>> +dt_provimpl_t	dt_pid_sub = {
>>> +	.name		= prvname,
>>> +	.prog_type	= BPF_PROG_TYPE_KPROBE,
>>> +	.populate	= &populate,
>>> +	.provide_pid	= &provide_pid,
>>> +	.enable		= &enable,
>>> +	.trampoline	= &trampoline,
>>> +};
>> I would think that
>>               dt_pid_sub.name
>>               dt_pid_sub.populate
>> could/should be NULL.
> A provider implementation name shouldn't be NULL.
Why is that?  Put another way, how could one distinguish whether the 
correct name is prvname (same name for all process-specific providers?), 
NULL, or "foo"?  So far as I can tell, the name is simply never used.  
The only provider names that matter are those for providers hardwired 
into dt_providers[] in dt_open.c.  Since dt_pid_proc (nee dt_pid_sub) is 
not among them, the role of this name is clearer if no name is assigned 
rather than assigning a name that means nothing.

PS  Just noticed libdtrace/dt_provider.h also needs a Copyright-year 
refresh.