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

[Eugene Loh]

On 06/15/2020 03:05 PM, Kris Van Hees wrote:

> 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

Obviously?  Not for me.  Specifically, the Orabug that is referenced in 
the commit message points to something that seems not to use the 
change.  Further, if there was some point to the change, I would expect 
a patch series that uses the change.  In summary, I'm left with 
"mysteriously" rather than with "obviously."

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

Okay.  I'll submit a revised patch.

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