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

[Kris Van Hees]

On Wed, Sep 07, 2022 at 03:50:31PM +0100, Nick Alcock wrote:
> On 7 Sep 2022, Kris Van Hees via DTrace-devel told this:
> 
> > The basic mechanics of the setopt() action are implemented in this
> > patch.  Each dynamic runtime options that can be set using setopt()
> 
> s/options/option/

Thanks.

> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >  libdtrace/dt_cg.c      | 11 ++++++++++
> >  libdtrace/dt_consume.c | 47 +++++++++++++++++++++++++++++++++---------
> >  2 files changed, 48 insertions(+), 10 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index cb51c474..ffab2443 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> >@@ -1751,6 +1751,17 @@ dt_cg_act_raise(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >  static void
> >  dt_cg_act_setopt(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >  {
> > +	dt_node_t	*opt = dnp->dn_args;
> > +	dt_node_t	*val = opt->dn_list;
> > +
> > +	TRACE_REGSET("setopt(): Begin ");
> > +	dt_cg_store_val(pcb, opt, DTRACEACT_LIBACT, NULL, DT_ACT_SETOPT);
> > +
> > +	/* If not value is given, we default to 0. */
> 
> s/not/no/

Thanks.

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

> >@@ -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, and == sizeof(uint64_t) if no value was specified
(e.g. for toggle options) since then a literal 0 is passed.

I'll add a comment.