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

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

On Fri, Oct 25, 2024 at 01:56:34AM -0400, Eugene Loh wrote:
> 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.

Err, duh.  I should have seen that - should have had more coffee.

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