[DTrace-devel] [PATCH 1/5] Change use of dtrace_probe_iter()

[Kris Van Hees]

On Mon, Jun 15, 2020 at 10:44:09AM -0700, Eugene Loh wrote:
> On 06/12/2020 04:55 PM, Kris Van Hees wrote:
> 
> > On Thu, Jun 11, 2020 at 05:29:37PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> In DTrace v1, dtrace_probe_iter() was used to iterate over probes,
> >> using dt_probe_iter() as a callback function.
> >>
> >> In the current release, dtrace_probe_iter() will be used as an external
> >> API, with dt_probe_iter() the "internal" (to libdtrace) interface.
> > I am not against this patch but I also do not see the need for it.
> 
> Need?  I suppose there is no need.  It was simply my interpretation of 
> your earlier comments.  I suppose, however, that there is also no need 
> to call the dtrace_* wrapper if one can call the internal dt_* function 
> directly.
> 
> > It is not
> > wrong for libdtrace code to use dtrace_probe_iter.  My intent was that the code
> > that uses dtrace_probe_iter would continue to use it, and only have direct
> > calls to dt_probe_iter done for cases where we need the callback function to
> > have access to the actual dt_probe_t (not exported) and not just the
> > dtrace_probedesc_t (exported).
> >
> > Maybe you should look at whether using the dt_probe_f callback variant in this
> > case would benefit the rest of the code?  E.g. you don't need to do a provider
> > lookup (by name) since there is a pointer to the provider in dt_probe_t.
> 
> That seems to be a different issue from the one in this patch. Finding a 
> use for dt_probe_f should presumably have been done before dt_probe_f 
> was ever introduced -- namely, d9590b9f17c1 "Introduce internal 
> dt_probe_iter() function".  That commit introduced all this 
> dtrace_probe_iter/dt_probe_iter complexity saying it was needed, but 
> AFAICT dt_probe_f is still not used anywhere.  That patch was supposed 
> to have been in support of the probe cleanup mechanism, but that 
> mechanism does not use probe_iter.

1. dt_probe_iter with support for dt_probe_t was introduced based on needing
   it so boviously there was a use identified prior to it being introduced;
   just because the use case isn't in the code yet doesn't mean that it does
   not exist
2. my email already points out a use case because calling dtrace_probe_iter
   (or dt_probe_iter using the original dtrace_probespec_t) and then doing a
   lookup based on the probedesc you get is silly - during the matching we are
   already working with dt_probe_t structures sowe might as well make use of
   that rather than doing a dance of probedesc -> probe -> probedesc -> probe.

> I'm okay throwing out both the proposed patch and d9590b9f.

So, um, no.  I agree that changing this from using dtrace_probe_iter to
dt_probe_iter is the right choice, but only if we use the dt_probe_t based
variant (it just makes sense).

> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >>   libdtrace/dt_probe.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> >> index b6c79354..7fa6c341 100644
> >> --- a/libdtrace/dt_probe.c
> >> +++ b/libdtrace/dt_probe.c
> >> @@ -989,14 +989,14 @@ dt_probe_info(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
> >>   	pd.id = DTRACE_IDNONE;
> >>   
> >>   	/*
> >> -	 * Call dtrace_probe_iter() to find matching probes.  Our
> >> +	 * Call dt_probe_iter() to find matching probes.  Our
> >>   	 * dt_probe_desc() callback will produce the following results:
> >>   	 *
> >> -	 * m < 0 dtrace_probe_iter() found zero matches (or failed).
> >> -	 * m > 0 dtrace_probe_iter() found more than one match.
> >> -	 * m = 0 dtrace_probe_iter() found exactly one match.
> >> +	 * m < 0 dt_probe_iter() found zero matches (or failed).
> >> +	 * m > 0 dt_probe_iter() found more than one match.
> >> +	 * m = 0 dt_probe_iter() found exactly one match.
> >>   	 */
> >> -	if ((m = dtrace_probe_iter(dtp, pdp, dt_probe_desc, &pd)) < 0)
> >> +	if ((m = dt_probe_iter(dtp, pdp, NULL, dt_probe_desc, &pd)) < 0)
> >>   		return NULL; /* dt_errno is set for us */
> >>   
> >>   	if ((pvp = dt_provider_lookup(dtp, pd.prv)) == NULL)
> >> -- 
> >> 2.18.2
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel