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

[Eugene Loh]

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

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

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

> @@ -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;
>   			}
>   		}