[DTrace-devel] [PATCH 2/4] Add support for string data to the trace() action

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

Two very minor comments below.

On 6/8/21 11:37 PM, Kris Van Hees wrote:
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> @@ -1864,13 +1865,27 @@ static int
>   dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
>   	       caddr_t data, int quiet)
>   {
> +	if (dtp->dt_options[DTRACEOPT_RAWBYTES] != DTRACEOPT_UNSET)
> +		return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
> +
>   	/*
> -	 * String or any other non-numeric data items are printed as a byte
> -	 * stream.
> +	 * String data can be recognized as a non-scalar data item with
> +	 * alignment == 1.
> +	 * Any other non-scalar data items are printed as a byte stream.
>   	 */

The comment block is in opposite order from the actual code, which does 
byte stream first and then string.  E.g.,. how about:
         By-reference data items with non-unit alignment are printed as 
a byte stream.
         Otherwise, by-reference data items are printed as a string.
The example speaks of "by-reference" items since that's closer to the 
code, but the phrase "by reference" is an independent matter.

> -	if (rec->dtrd_arg == DT_NF_REF)
> -		return dt_print_bytes(dtp, fp, data, rec->dtrd_size, 33,
> -				      quiet);
> +	if (rec->dtrd_arg == DT_NF_REF) {
> +		char	*s = (char *)data;
> +
> +		if (rec->dtrd_alignment > 1)
> +			return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
> +
> +		/* We have a string.  Skip the length prefix and print it. */
> +		s = (char *)dt_vint_skip(s);
> +		if (quiet)
> +			return dt_printf(dtp, fp, "%s", s);
> +		else
> +			return dt_printf(dtp, fp, "  %-33s", s);
> +	}
>   
> diff --git a/test/unittest/actions/trace/tst.str-quiet.d b/test/unittest/actions/trace/tst.str-quiet.d
> + * ASSERTION: The trace() action accepts a string argument.
> diff --git a/test/unittest/actions/trace/tst.str.d b/test/unittest/actions/trace/tst.str.d
>    * ASSERTION: The trace() action accepts a string argument.
I know the ASSERTIONs date back to the test's inception (relatively 
recently), but these ASSERTIONs seem too weak and in any case out of 
step with (almost all of) the other tests.  Compare:

  * ASSERTION: The trace() action accepts a string argument.

versus

  * ASSERTION: The trace() action prints a unsigned 16-bit value 
correctly in
  *            non-quiet mode.
  * ASSERTION: The trace() action prints a unsigned 16-bit value 
correctly in
  *            quiet mode.

The latter are more informative and make the important point that the 
tests not only accept certain arguments but in fact print them 
correctly.  How about bringing tst.str[-quiet].d in line with the bulk 
of the subdirectory's tests?