[DTrace-devel] [PATCH] Add workstatus error conditions

[Eugene Loh]

On 01/15/2021 05:38 PM, Kris Van Hees wrote:

> The processing of perf event output buffer data can fail in a few
> different ways, all leading to returning DTRACE_WORKSTATUS_ERROR.
> In order to better troubleshoot such issues we add two new DTrace
> error codes:
>
> - EDT_DSIZE: Data record has an incrreoct size

Typo.

(Beyond that, I was going to suggest using "invalid", but given that 
"incorrect" is used in the actual error string, "incorrect" also makes 
sense here.)

> - EDT_BADEPID: Data record contains an invalid EPID
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_consume.c | 12 +++++++++---
>   libdtrace/dt_error.c   |  4 +++-
>   libdtrace/dt_impl.h    |  4 +++-
>   3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 497fdd27..46b03641 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1950,14 +1950,18 @@ dt_consume_one(dtrace_hdl_t *dtp, FILE *fp, char *buf,
>   		 * and 'data' points to the 'size' member at this point.
>   		 * (Note that 'n' may be 0.)
>   		 */
> -		if (ptr > buf + hdr->size)
> +		if (ptr > buf + hdr->size) {
> +			dt_set_errno(dtp, EDT_DSIZE);
>   			return DTRACE_WORKSTATUS_ERROR;
> +		}

Instead of adding the braces and using "dt_set_errno()/return 
DTRACE_WORKSTATUS_ERROR", just use "return dt_set_errno()".  That way, 
you conform to current (and continuing) practice within this function.  
That is, all other dt_set_errno() sites within this function use the 
"return dt_set_errno()" expression.  I realize the two expressions do 
the same thing, but the pre-existing practice is more compact and 
uniformity of practice throughout the function would be good.

>   		size = *(uint32_t *)data;
>   		data += sizeof(size);
>   		ptr += sizeof(size) + size;
> -		if (ptr != buf + hdr->size)
> +		if (ptr != buf + hdr->size) {
> +			dt_set_errno(dtp, EDT_DSIZE);
>   			return DTRACE_WORKSTATUS_ERROR;
> +		}

Ditto.

>   
>   		data += sizeof(uint32_t);		/* skip padding */
>   		size -= sizeof(uint32_t);
> @@ -1976,8 +1980,10 @@ dt_consume_one(dtrace_hdl_t *dtp, FILE *fp, char *buf,
>   
>   		rval = dt_epid_lookup(dtp, epid, &pdat->dtpda_ddesc,
>   						 &pdat->dtpda_pdesc);
> -		if (rval != 0)
> +		if (rval != 0) {
> +			dt_set_errno(dtp, EDT_BADEPID);
>   			return DTRACE_WORKSTATUS_ERROR;
> +		}

Ditto.

>   		if (flow)
>   			dt_flowindent(dtp, pdat, *last, DTRACE_EPIDNONE);
> diff --git a/libdtrace/dt_error.c b/libdtrace/dt_error.c
> index b2688402..6276d52c 100644
> --- a/libdtrace/dt_error.c
> +++ b/libdtrace/dt_error.c
> @@ -1,6 +1,6 @@
>   /*
>    * Oracle Linux DTrace.
> - * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2009, 2021, Oracle and/or its affiliates. All rights reserved.
>    * Licensed under the Universal Permissive License v 1.0 as shown at
>    * http://oss.oracle.com/licenses/upl.
>    */
> @@ -52,6 +52,8 @@ static const struct {
>   	{ EDT_DMISMATCH, "Data record list does not match statement" },
>   	{ EDT_DOFFSET,	"Data record offset exceeds buffer boundary" },
>   	{ EDT_DALIGN,	"Data record has inappropriate alignment" },
> +	{ EDT_DSIZE,	"Data record has incorrect size" },
> +	{ EDT_BADEPID,	"Invalid EPID" },
>   	{ EDT_BADOPTNAME, "Invalid option name" },
>   	{ EDT_BADOPTVAL, "Invalid value for specified option" },
>   	{ EDT_BADOPTCTX, "Option cannot be used from within a D program" },
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 4548a11d..0cc4bfab 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -1,6 +1,6 @@
>   /*
>    * Oracle Linux DTrace.
> - * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
>    * Licensed under the Universal Permissive License v 1.0 as shown at
>    * http://oss.oracle.com/licenses/upl.
>    */
> @@ -585,6 +585,8 @@ enum {
>   	EDT_DMISMATCH,		/* record list does not match statement */
>   	EDT_DOFFSET,		/* record data offset error */
>   	EDT_DALIGN,		/* record data alignment error */
> +	EDT_DSIZE,		/* record data size error */
> +	EDT_BADEPID,		/* bad enabled probe id */

bad
->
invalid
(to be consistent with both the ensuing comments and, more importantly, 
the actual string)

>   	EDT_BADOPTNAME,		/* invalid dtrace_setopt option name */
>   	EDT_BADOPTVAL,		/* invalid dtrace_setopt option value */
>   	EDT_BADOPTCTX,		/* invalid dtrace_setopt option context */

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