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

Kris Van Hees
2024-09-14T01:59:50Z

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