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

[Kris Van Hees <kris.van.hees@oracle.com>]

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.

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...

> 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