[DTrace-devel] [PATCH 1/5] Implement setopt()

[Kris Van Hees]

On Wed, Sep 07, 2022 at 04:13:41PM +0100, Nick Alcock wrote:
> On 7 Sep 2022, Kris Van Hees spake thusly:
> 
> >> > +	if (val == NULL)
> >> > +		val = dt_node_int(0);
> >> 
> >> So this defaults VAL to 0...
> >
> > Well, no.  This ensures we have a NULL val argument node so that the rest of
> > the code can proceed without needing to deal with val as a NULL pointer.
> 
> Aha, this is a *string*. So 0 == NULL, right, ok.
> 
> >> >@@ -2332,6 +2334,31 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> >> >  				ftruncate(fileno(fp), 0);
> >> >  				fseeko(fp, 0, SEEK_SET);
> >> >  
> >> > +				continue;
> >> > +			case DT_ACT_SETOPT: {
> >> > +				caddr_t			opt = recdata;
> >> > +				caddr_t			val;
> >> > +				dtrace_recdesc_t	*vrec;
> >> > +
> >> > +				if (i == epd->dtdd_nrecs - 1)
> >> > +					return dt_set_errno(dtp, EDT_BADSETOPT);
> >> > +
> >> > +				vrec = &epd->dtdd_recs[++i];
> >> > +				if (vrec->dtrd_action != act &&
> >> > +				    vrec->dtrd_arg != rec->dtrd_arg)
> >> > +					return dt_set_errno(dtp, EDT_BADSETOPT);
> >> > +
> >> > +				if (vrec->dtrd_size > sizeof(uint64_t))
> >> > +					val = data + vrec->dtrd_offset;
> >> > +				else
> >> > +					val = "1";
> >> 
> >> ... and then this, uh, no I have absolutely no idea what this is doing.
> >> I guess future patches will put larger things into the value than a
> >> dt_node_int? (But then why nail a 1 in there rather than what the user
> >> passed in when something integral-or-smaller is passed? Why 1 at all,
> >> rather than the 0 up above? Doesn't this deserve a comment?)
> >
> > The setopt machinery expects the option name and value to be strings.  So the
> > size of the value (which is described in vrec) will be > sizeof(uint64_t) if
> > there is a value passed,
> 
> Really? "a" is < sizeof(uint64_t). So is "foo" for that matter. This
> seems like an... unreliable way to detect if a value was passed.
> 
> I feel like I'm missing something obvious :)

We are dealing with DTrace strings, remember?  And those are fixed size.
This is really the way it was done in v1 also.