Re: [DTrace-devel] [PATCH 02/14] Clean up prp/uprp variable names

[Eugene Loh <eugene.loh@oracle.com>]

On 6/4/24 14:44, Kris Van Hees wrote:

> On Tue, Jun 04, 2024 at 02:11:01PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Should prp be oprp?  Or should we use opr and upr?
> This does not belong in the commit message.  But yes, we should stick with
> prp for the overlying probe, which is consistent with the rest of the code
> base.

Yes, the comment was to solicit feedback like this.  I'll go with "prp" 
I guess, even though Nick (elsewhere) voted for "oprp".  I liked "oprp" 
because it's more explicit and the same number of letters as "uprp", but 
sticking with "prp" is simpler and arguably clear enough given the 
changes in this patch.

> Also, I think this patch should address a few other issues in this file, e.g.
> making sure comments are consistent with the code (e.g. if it mentions we are
> creating overlying and underlying probe lists, the code should also create
> tgem in that order - or the comment ought to be updated to match the code
> order).  etc...

I didn't find much else in the file.  I cleaned up some provide_probe() 
stuff (I'm guessing it's what you're referring to), but if there is 
something else egregious you're concerned about let me know.

>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>> ---
>>   libdtrace/dt_prov_uprobe.c | 55 +++++++++++++++++++++++++++-----------
>>   1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index afc1f628..cace406d 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -6,6 +6,31 @@
>>    *
>>    * The uprobe-based provider for DTrace (implementing pid and USDT providers).
>>    */
>> +/*
>> + * This file uses both overlying probes (specified by the user) as well as
>> + * underlying probes (the uprobes recognized by the kernel).  To minimize
>> + * confusion, this file should use consistent variable names:
>> + *
>> + *     dt_probe_t	*prp;   //  overlying probe
>> + *     dt_probe_t	*uprp;  // underlying probe
>> + *
>> + *             Either one might be returned by dt_probe_lookup() or
>> + *             dt_probe_insert() or added to dt_enablings[] or dt_probes[].
>> + *             Further, uprp might be returned by create_underlying().
>> + *
>> + *     dt_uprobe_t	*upp;   // uprobe associated with an underlying probe
>> + *
>> + *     list_probe_t	*pop;   //  overlying probe list
>> + *     list_probe_t	*pup;   // underlying probe list
>> + *
>> + * The provider-specific prv_data has these meanings:
>> + *
>> + *     prp->prv_data            // dt_list_t of associated underlying probes
>> + *
>> + *     uprp->prv_data           // upp (the associated uprobe)
>> + *
>> + * Finally, note that upp->probes is a dt_list_t of overlying probes.
>> + */
> This comment block is too verbose and I don't think it is really needed, if you
> are going to rename variables anyway to be consistent based on your proposal
> (as you do in this patch).  So, the comment becomes unnecessary by the patch
> itself.
>
> Even if we were to retain a comment like this, I think it should be much more
> brief.  But again, I think the patch itself accomplishes all that is needed,
> so no need to comment.
>
>>   #include <sys/types.h>
>>   #include <assert.h>
>>   #include <errno.h>
>> @@ -118,7 +143,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>   	char			mod[DTRACE_MODNAMELEN];
>>   	char			prb[DTRACE_NAMELEN];
>>   	dtrace_probedesc_t	pd;
>> -	dt_probe_t		*prp;
>> +	dt_probe_t		*uprp;
> I am OK with doing this renaming of variable name because you want some form
> of consustency throughout this file, but I don't believe it is really needed.
> This function only deals with one type of probes, identified both in the
> comment and the function name as underlying probes.  So, the prp variable that
> is used in many places in DTrace source code to denote a probe pointer should
> not cause any confusion.
>
> But if you want to change it, no problem.
>
>>   	dt_uprobe_t		*upp;
>>   	int			is_enabled = 0;
>>   
>> @@ -160,8 +185,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>   	pd.fun = psp->pps_fun;
>>   	pd.prb = prb;
>>   
>> -	prp = dt_probe_lookup(dtp, &pd);
>> -	if (prp == NULL) {
>> +	uprp = dt_probe_lookup(dtp, &pd);
>> +	if (uprp == NULL) {
>>   		dt_provider_t	*pvp;
>>   
>>   		/* Get the provider for underlying probes. */
>> @@ -182,12 +207,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>   		if (upp->tp == NULL)
>>   			goto fail;
>>   
>> -		prp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>> +		uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>>   				      upp);
>> -		if (prp == NULL)
>> +		if (uprp == NULL)
>>   			goto fail;
>>   	} else
>> -		upp = prp->prv_data;
>> +		upp = uprp->prv_data;
>>   
>>   	switch (psp->pps_type) {
>>   	case DTPPT_RETURN:
>> @@ -202,7 +227,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>>   	     */
>>   	}
>>   
>> -	return prp;
>> +	return uprp;
>>   
>>   fail:
>>   	probe_destroy(dtp, upp);
>> @@ -394,8 +419,8 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
>>   static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>   {
>>   	dt_irlist_t		*dlp = &pcb->pcb_ir;
>> -	const dt_probe_t	*prp = pcb->pcb_probe;
>> -	const dt_uprobe_t	*upp = prp->prv_data;
>> +	const dt_probe_t	*uprp = pcb->pcb_probe;
>> +	const dt_uprobe_t	*upp = uprp->prv_data;
>>   	const list_probe_t	*pop;
>>   	uint_t			lbl_exit = pcb->pcb_exitlbl;
>>   
>> @@ -508,8 +533,8 @@ copyout_val(dt_pcb_t *pcb, uint_t lbl, uint32_t val, int arg)
>>   static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
>>   {
>>   	dt_irlist_t		*dlp = &pcb->pcb_ir;
>> -	const dt_probe_t	*prp = pcb->pcb_probe;
>> -	const dt_uprobe_t	*upp = prp->prv_data;
>> +	const dt_probe_t	*uprp = pcb->pcb_probe;
>> +	const dt_uprobe_t	*upp = uprp->prv_data;
>>   	const list_probe_t	*pop;
>>   	uint_t			lbl_assign = dt_irlist_label(dlp);
>>   	uint_t			lbl_exit = pcb->pcb_exitlbl;
>> @@ -636,9 +661,9 @@ out:
>>   	return name;
>>   }
>>   
>> -static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>> +static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>>   {
>> -	dt_uprobe_t	*upp = prp->prv_data;
>> +	dt_uprobe_t	*upp = uprp->prv_data;
>>   	tp_probe_t	*tpp = upp->tp;
>>   	FILE		*f;
>>   	char		*fn;
>> @@ -733,9 +758,9 @@ out:
>>    * probes that didn't get created.  If the removal fails for some reason we are
>>    * out of luck - fortunately it is not harmful to the system as a whole.
>>    */
>> -static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>> +static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
>>   {
>> -	dt_uprobe_t	*upp = prp->prv_data;
>> +	dt_uprobe_t	*upp = uprp->prv_data;
>>   	tp_probe_t	*tpp = upp->tp;
>>   
>>   	if (!dt_tp_has_info(tpp))
>> -- 
>> 2.18.4
>>
>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel