[DTrace-devel] [PATCH v2 01/04] probe: propagate dt_probe_args_info() failures to dt_probe_info()

[Kris Van Hees]

On Thu, Nov 30, 2023 at 05:11:58PM +0000, Nick Alcock wrote:
> On 30 Nov 2023, Kris Van Hees via DTrace-devel stated:
> 
> > Failures in dt_probe_args_info() were ignored by dt_probe_info() and
> > that could lead to strange behaviour.  E.g. if lockmem issues cause
> > the logic in the raw tracepoint argument count determination to fail,
> > the probe was reported as not having any arguments.  This patch will
> > make it possible to add code to accurately report such failures
> > rather than silently ignoring them.
> 
> Good idea, but...
> 
> > diff --git a/cmd/dtrace.c b/cmd/dtrace.c
> > index e7ca9e4c..9c820686 100644
> > --- a/cmd/dtrace.c
> > +++ b/cmd/dtrace.c
> > @@ -323,6 +323,8 @@ info_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp,
> >  
> >  	if (dtrace_probe_info(dtp, pdp, &p) == 0)
> >  		print_probe_info(&p);
> > +	else
> > +		return -1;
> 
> So dtrace_probe_info()'s error returns do not emit any errors anywhere:
> they just set the dtrace_errno. On error, this doesn't emit any errors either...

Which is it meant to do...

> >  	*last = edp;
> >  	return 0;
> > @@ -417,8 +419,12 @@ list_probe(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp, void *arg)
> >  	oprintf("%5d %10s %17s %33s %s\n",
> >  		pdp->id, pdp->prv, pdp->mod, pdp->fun, pdp->prb);
> >  
> > -	if (g_verbose && dtrace_probe_info(dtp, pdp, &p) == 0)
> > -		print_probe_info(&p);
> > +	if (g_verbose) {
> > +		if (dtrace_probe_info(dtp, pdp, &p) == 0)
> > +			print_probe_info(&p);
> > +		else
> > +			return -1;
> > +	}
> 
> ... so nor does this; and then this call, which you didn't touch:

Which is also what is meant to happen.
> 
> 	case DMODE_LIST:
> [...]		if (g_cmdc == 0)
> 			dtrace_probe_iter(g_dtp, NULL, list_probe, NULL);
> 
> 		dtrace_close(g_dtp);
> 		return g_status;
> 
> ... exits early without printing the dtrace_errmsg. You might want to
> check all the dtrace_probe_iter()s that call list_probe() for error
> returns, perhaps?

Err... the code you quote above and allegedly is not handling things right
is *exactly* the code that the next patch fragment you quote below changes
to actually output the error.

> > @@ -1426,8 +1432,10 @@ main(int argc, char *argv[])
> >  		for (i = 0; i < g_cmdc; i++)
> >  			list_prog(&g_cmdv[i]);
> >  
> > -		if (g_cmdc == 0)
> > -			dtrace_probe_iter(g_dtp, NULL, list_probe, NULL);
> > +		if (g_cmdc == 0) {
> > +			if (dtrace_probe_iter(g_dtp, NULL, list_probe, NULL) < 0)
> > +				dfatal(NULL); /* dtrace_errmsg() only */
> > +		}
> 
> Like you do here :)
> 
> The changes to dt_probe.c look good.
> 
> -- 
> NULL && (void)