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

[Kris Van Hees]

On Thu, Sep 08, 2022 at 02:27:32PM -0400, Eugene Loh via DTrace-devel wrote:
> I'm curious about the status of certain earlier review comments.
> Specifically...
> 
> On 9/8/22 13:12, Kris Van Hees via DTrace-devel wrote:
> > The basic mechanics of the setopt() action are implemented in this
> > patch.  Each dynamic runtime option that can be set using setopt()
> > must provide support for it.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_cg.c      | 11 +++++++++
> >   libdtrace/dt_consume.c | 53 ++++++++++++++++++++++++++++++++++--------
> >   2 files changed, 54 insertions(+), 10 deletions(-)
> 
> There should be a test.  One candidate is removing the xfail on tst.badopt.d
> (assuming the .r file is fixed).

The series clearly already has at least one test that exercises this, but yes,
tst.badopt.d should be adjusted as part of this patch since it now passes.

But I will add a bunch of other tests also because apparently we do not really
have the typical action tests for this anyway.

Thanks for having me think this through some more.

> > diff --git 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 no value is given, we default to NULL. */
> > +	if (val == NULL)
> > +		val = dt_node_int(0);
> > +	dt_cg_store_val(pcb, val, DTRACEACT_LIBACT, NULL, DT_ACT_SETOPT);
> > +	TRACE_REGSET("setopt(): End   ");
> >   }
> 
> The point is not that we're defaulting to null but that we're emitting
> something with non-unit alignment.  The hack is sufficiently, um, nuanced
> that a clearer and more accurate explanation here would be good.

Actually, we *are* defaulting to NULL.  And we are using the fact that this
means we are putting an 4-byte int in the data record rather than a string, and
that the alignment for strings is 1 byte whereas the 4-byte int uses a 4 byte
alignment.

The fact that an alignment check is sufficient to distinguish the two is not
relevant to this code in the code generator - that is something that the
consumer code is able to use to make distinguishing between the cases easier.

> > diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> > @@ -2150,6 +2150,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> >   		     void *arg)
> >   {
> >   	dtrace_epid_t		epid;
> > +	dtrace_datadesc_t	*epd = pdat->dtpda_ddesc;
> >   	dt_spec_buf_t		tmpl;
> >   	dt_spec_buf_t		*dtsb;
> >   	int			specid;
> 
> It is premature to compute epd here.  I assume this value is incorrect,
> never used, and overwritten.

Copy and paste mistake - fixed.

> > @@ -2170,11 +2171,12 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
> >   	pdat->dtpda_data = data;
> >   	rval = dt_epid_lookup(dtp, epid, &pdat->dtpda_ddesc,
> > -			      &pdat->dtpda_pdesc);
> > +					 &pdat->dtpda_pdesc);
> >   	if (rval != 0)
> >   		return dt_set_errno(dtp, EDT_BADEPID);
> > -	if (pdat->dtpda_ddesc->dtdd_uarg != DT_ECB_DEFAULT) {
> > +	epd = pdat->dtpda_ddesc;
> > +	if (epd->dtdd_uarg != DT_ECB_DEFAULT) {
> >   		rval = dt_handle(dtp, pdat);
> >   		if (rval == DTRACE_CONSUME_NEXT)
> > @@ -2332,6 +2334,37 @@ 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);
> > +
> > +				/*
> > +				 * Two possibilities: either a string was
> > +				 * passed (alignment 1), or a uint32_t was
> > +				 * passed (alignment 4).  The latter indicates
> > +				 * a toggle option (no value).
> > +				 */
> > +				if (vrec->dtrd_alignment == 1)
> > +					val = data + vrec->dtrd_offset;
> > +				else
> > +					val = "1";
> 
> 
> Okay, though "on" or "set" might be cleaner.  Okay as is.
> 
> > +
> > +				if (dt_setopt(dtp, pdat, opt, val) != 0)
> > +					return DTRACE_WORKSTATUS_ERROR;
> > +
> > +				continue;
> > +			}
> > +			default:
> >   				continue;
> >   			}
> >   		}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel