Re: [PATCH v3 11/19] Support USDT wildcard provider descriptions

[Eugene Loh <eugene.loh@oracle.com>]

On 10/24/24 12:38, Kris Van Hees wrote:

> On Tue, Sep 24, 2024 at 04:25:52PM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> To look for pid probes, whose pid values must be specified explicitly,
>> we can require that the provider description should end in a digit.
>>
>> For USDT probes, however, there can be wildcard descriptions.  This
>> includes a blank provider description as well as a description that
>> ends in an '*'.
> What about 'test*1234' or 'test*12*' or 'test*1*2*4'?  All should match
> 'test_prov1234', right?

Right.  "test*1234" and "test*1*2*4" already end in a digit, so were 
already included.
"test*12*" ends in an '*' and so is added by the patch.

>> So, in dt_setcontext(), expand the criteria appropriately.  And
>> modify dt_pid_create_probes() accordingly.
>>
>> This is rudimentary support.  We still need to:
>> - handle globs in dt_pid_create_probes_module()
>> - add process monitoring / inotify
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>
>> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
>> index 12104fc21..62482a70f 100644
>> --- a/libdtrace/dt_cc.c
>> +++ b/libdtrace/dt_cc.c
>> @@ -273,8 +273,9 @@ dt_setcontext(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp)
>>   	 * On an error, dt_pid_create_probes() will set the error message
>>   	 * and tag -- we just have to longjmp() out of here.
>>   	 */
>> -	if (pdp->prv && pdp->prv[0] &&
>> -	    isdigit(pdp->prv[strlen(pdp->prv) - 1]) &&
>> +	if (pdp->prv &&
>> +	    (pdp->prv[0] == '\0' || isdigit(pdp->prv[strlen(pdp->prv) - 1]) ||
>> +	             pdp->prv[strlen(pdp->prv) - 1] == '*') &&
>>   	    ((pvp = dt_provider_lookup(dtp, pdp->prv)) == NULL ||
>>   	     pvp->pv_flags & DT_PROVIDER_PID) &&
>>   	    dt_pid_create_probes((dtrace_probedesc_t *)pdp, dtp, yypcb) != 0) {
> First of all, I am not sure that combining pid and USDT probe handling here
> by means of dt_pid_create_probes() makes sense under the new scheme.  In fact,
> dt_pid_create_probes() may no longer be needed because handling them separately
> seems better.
>
> Then there are two cases:
>
>   - For pid probes, a provider name must have been supplied and it must have a
>     digit as last character.  So, for pid probes you can keep the original code
>     aside from replacing dt_pid_create_probes() with dt_pid_create_pid_probes().
>
>   - For USDT probes, there aren't any restrictions, other than the ones already
>     done in dt_pid_create_usdt_probes().  So, a call to that function should be
>     done here without any conditional.

Okay, though I don't really understand the dt_setcontext() logic. E.g., 
we test pdp->prv!=NULL.  If it could be NULL, then we should still check 
before calling dt_pid_create_usdt_probes(). Stuff like that.  But I made 
your suggested change and it passed tests...  so good enough for me.