Re: [PATCH 07/22] Supply a default probe_info()

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

On Fri, Sep 13, 2024 at 08:31:16PM -0400, Kris Van Hees wrote:
> Is this still needed after the fix we put in for dealing with argc == -1, i.e.
> commit 0e3231a268 "ident: fix unsigned vs signed comparison" ?

Ah no, you posted one that is based on the ident fix.

Comments below...

> On Thu, Aug 29, 2024 at 01:22:04AM -0400, eugene.loh@oracle.com wrote:
> > From: Eugene Loh <eugene.loh@oracle.com>
> > 
> > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > ---
> >  libdtrace/dt_probe.c        |  4 +++-
> >  libdtrace/dt_prov_cpc.c     | 11 -----------
> >  libdtrace/dt_prov_dtrace.c  | 10 ----------
> >  libdtrace/dt_prov_fbt.c     | 10 ----------
> >  libdtrace/dt_prov_profile.c | 11 -----------
> >  5 files changed, 3 insertions(+), 43 deletions(-)
> > 
> > diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> > index 0b53121a..ab90d2ed 100644
> > --- a/libdtrace/dt_probe.c
> > +++ b/libdtrace/dt_probe.c
> > @@ -892,8 +892,10 @@ dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
> >  	/* Only retrieve probe argument information once per probe. */
> >  	if (prp->argc != -1)
> >  		return 0;
> > -	if (!prp->prov->impl->probe_info)
> > +	if (!prp->prov->impl->probe_info) {
> > +		prp->argc = 0;
> >  		return 0;
> > +	}
> >  	rc = prp->prov->impl->probe_info(dtp, prp, &argc, &argv);
> >  	if (rc == -1)
> >  		return rc;

I think that it might be cleaner to do:

	if (prp->prov->impl->probe_info &&
	    prp->prov->impl->probe_info(dtp, prp, &argc, &argv) == -1)
		return -1;

because the code that follows will look at argc (initialized as 0) and argv
(initialized as NULL), so if there is no probe_info hook, those initial
values will result in prp->argc being set to 0.

> > diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> > index 08689b35..8f33cf58 100644
> > --- a/libdtrace/dt_prov_cpc.c
> > +++ b/libdtrace/dt_prov_cpc.c
> > @@ -451,16 +451,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return nattach > 0 ? 0 : -1;
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	/* cpc-provider probe arguments are not typed */
> > -	*argcp = 0;
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> >  {
> >  	cpc_probe_t	*datap = prp->prv_data;
> > @@ -504,7 +494,6 @@ dt_provimpl_t	dt_cpc = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &trampoline,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &probe_destroy,
> >  	.destroy	= &destroy,
> > diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> > index bf87cb05..a9deccee 100644
> > --- a/libdtrace/dt_prov_dtrace.c
> > +++ b/libdtrace/dt_prov_dtrace.c
> > @@ -273,15 +273,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	*argcp = 0;			/* no arguments */
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Try to clean up system resources that may have been allocated for this
> >   * probe.
> > @@ -317,7 +308,6 @@ dt_provimpl_t	dt_dtrace = {
> >  	.trampoline	= &trampoline,
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &dt_tp_probe_destroy,
> >  };
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 62c568ce..21f63ddf 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -411,15 +411,6 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return dt_tp_probe_attach(dtp, prp, bpf_fd);
> >  }
> >  
> > -static int kprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -			     int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	*argcp = 0;			/* no arguments by default */
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Try to clean up system resources that may have been allocated for this
> >   * probe.
> > @@ -469,7 +460,6 @@ dt_provimpl_t	dt_fbt_kprobe = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &kprobe_trampoline,
> >  	.attach		= &kprobe_attach,
> > -	.probe_info	= &kprobe_probe_info,
> >  	.detach		= &kprobe_detach,
> >  	.probe_destroy	= &dt_tp_probe_destroy,
> >  };
> > diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> > index bc224348..e1369ca9 100644
> > --- a/libdtrace/dt_prov_profile.c
> > +++ b/libdtrace/dt_prov_profile.c
> > @@ -299,16 +299,6 @@ static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> >  	return nattach > 0 ? 0 : -1;
> >  }
> >  
> > -static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > -		      int *argcp, dt_argdesc_t **argvp)
> > -{
> > -	/* profile-provider probe arguments are not typed */
> > -	*argcp = 0;
> > -	*argvp = NULL;
> > -
> > -	return 0;
> > -}
> > -
> >  static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
> >  {
> >  	profile_probe_t	*pp = prp->prv_data;
> > @@ -337,7 +327,6 @@ dt_provimpl_t	dt_profile = {
> >  	.load_prog	= &dt_bpf_prog_load,
> >  	.trampoline	= &trampoline,
> >  	.attach		= &attach,
> > -	.probe_info	= &probe_info,
> >  	.detach		= &detach,
> >  	.probe_destroy	= &probe_destroy,
> >  };
> > -- 
> > 2.43.5
> >