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

[Kris Van Hees]

On Fri, Jan 15, 2021 at 09:41:18PM -0800, Eugene Loh wrote:
> 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.

Yes, although I am torn on the issue because we're really supposed to be
returning a dtrace_workstatus_t value (DTRACE_WORKSTATUS_* constants).  Then
again, DTRACE_WORKSTATUS_ERROR is defined as -1 anyway, so it does work
to simply return dt_set_errno(...).  We actually have some explicit return -1
in that function as well which is clearly not the best idea.

For consistency and compactness I guess going with the return dt_set_errno(...)
variant seems prudent, and where now return -1 is done we should replace it
with return DTRACE_WORKSTATUS_ERROR.  I think I'll add that to this patch and
mention in the commit msg that this change was made to provide more consistency.

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

Yes, although these comments are norotiously inconsistent when it comes to
what the actual error string is.

> >   	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>
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel