[DTrace-devel] [PATCH] Fix error reporting for agg walk functioons

[Eugene Loh]

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

"functioons" in the subject line.

You change the return values 0|-1 to DTRACE_AGGWALK_NEXT|ERROR in both 
dt_print_aggs() and dt_print_agg().  So does that mean the func() return 
value in dtrace_aggregate_walk_joined() should likewise be compared to 
DTRACE_AGGWALK_ERROR rather than to -1? Clearly, the computer does not 
care, but if it's worth changing in some spots then why not in others.

On 8/12/22 00:01, Kris Van Hees via DTrace-devel wrote:
> Callback functions for aggregation walkers were not correctly returning
> DTRACE_AGGWALK_* values, causing "invalid return value from callback"
> errors to be reported rather than the actual error.  This is only seen
> when the consumer code has a bug in it (e.g. record description
> mismatches).
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_aggregate.c |  2 +-
>   libdtrace/dt_consume.c   | 20 ++++++++++----------
>   libdtrace/dt_printf.c    | 16 ++++++++++------
>   3 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
> index f079073a..559a497e 100644
> --- a/libdtrace/dt_aggregate.c
> +++ b/libdtrace/dt_aggregate.c
> @@ -1112,7 +1112,7 @@ dt_aggwalk_rval(dtrace_hdl_t *dtp, dt_ahashent_t *h, int rval)
>   		/*
>   		 * We assume that errno is already set in this case.
>   		 */
> -		return dt_set_errno(dtp, errno);
> +		return -1;
>   
>   	case DTRACE_AGGWALK_ABORT:
>   		return dt_set_errno(dtp, EDT_DIRABORT);
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 51e7c43a..973084d6 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -1722,11 +1722,11 @@ dt_print_aggs(const dtrace_aggdata_t **aggsdata, int naggvars, void *arg)
>   		}
>   
>   		if (dt_print_datum(dtp, fp, rec, addr, size, 1, 0) < 0)
> -			return -1;
> +			return DTRACE_AGGWALK_ERROR;
>   
>   		if (dt_buffered_flush(dtp, NULL, rec, aggdata,
>   				      DTRACE_BUFDATA_AGGKEY) < 0)
> -			return -1;
> +			return DTRACE_AGGWALK_ERROR;
>   	}
>   
>   	for (i = (naggvars == 1 ? 0 : 1); i < naggvars; i++) {
> @@ -1743,25 +1743,25 @@ dt_print_aggs(const dtrace_aggdata_t **aggsdata, int naggvars, void *arg)
>   
>   		if (dt_print_datum(dtp, fp, rec, addr, size, normal,
>   				   agg->dtagd_sig) < 0)
> -			return -1;
> +			return DTRACE_AGGWALK_ERROR;
>   
>   		if (dt_buffered_flush(dtp, NULL, rec, aggdata,
>   				      DTRACE_BUFDATA_AGGVAL) < 0)
> -			return -1;
> +			return DTRACE_AGGWALK_ERROR;
>   
>   		if (!pd->dtpa_allunprint)
>   			agg->dtagd_flags |= DTRACE_AGD_PRINTED;
>   	}
>   
>   	if (dt_printf(dtp, fp, "\n") < 0)
> -		return -1;
> +		return DTRACE_AGGWALK_ERROR;
>   
>   	if (dt_buffered_flush(dtp, NULL, NULL, aggdata,
>   			      DTRACE_BUFDATA_AGGFORMAT |
>   			      DTRACE_BUFDATA_AGGLAST) < 0)
> -		return -1;
> +		return DTRACE_AGGWALK_ERROR;
>   
> -	return 0;
> +	return DTRACE_AGGWALK_NEXT;
>   }
>   
>   int
> @@ -1773,7 +1773,7 @@ dt_print_agg(const dtrace_aggdata_t *aggdata, void *arg)
>   
>   	if (pd->dtpa_allunprint) {
>   		if (agg->dtagd_flags & DTRACE_AGD_PRINTED)
> -			return 0;
> +			return DTRACE_AGGWALK_NEXT;
>   	} else {
>   		/*
>   		 * If we're not printing all unprinted aggregations, then the
> @@ -1782,10 +1782,10 @@ dt_print_agg(const dtrace_aggdata_t *aggdata, void *arg)
>   		 * that we encounter.
>   		 */
>   		if (agg->dtagd_nrecs == 0)
> -			return 0;
> +			return DTRACE_AGGWALK_NEXT;
>   
>   		if (aggvarid != agg->dtagd_varid)
> -			return 0;
> +			return DTRACE_AGGWALK_NEXT;
>   	}
>   
>   	return dt_print_aggs(&aggdata, 1, arg);
> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
> index 5fccb555..4fb20b23 100644
> --- a/libdtrace/dt_printf.c
> +++ b/libdtrace/dt_printf.c
> @@ -1818,8 +1818,10 @@ dt_fprinta(const dtrace_aggdata_t *adp, void *arg)
>   		return 0;	/* id does not match */
>   
>   	if (dt_printf_format(dtp, pfw->pfw_fp, pfw->pfw_argv, rec, nrecs,
> -			     adp->dtada_data, adp->dtada_size, &adp, 1) == -1)
> -		return (pfw->pfw_err = dtp->dt_errno);
> +			     adp->dtada_data, adp->dtada_size, &adp, 1) == -1) {
> +		pfw->pfw_err = dtp->dt_errno;
> +		return DTRACE_AGGWALK_ERROR;
> +	}
>   
>   	/*
>   	 * Cast away the const to set the bit indicating that this aggregation
> @@ -1827,7 +1829,7 @@ dt_fprinta(const dtrace_aggdata_t *adp, void *arg)
>   	 */
>   	((dtrace_aggdesc_t *)agg)->dtagd_flags |= DTRACE_AGD_PRINTED;
>   
> -	return 0;
> +	return DTRACE_AGGWALK_NEXT;
>   }
>   
>   static int
> @@ -1843,8 +1845,10 @@ dt_fprintas(const dtrace_aggdata_t **aggsdata, int naggvars, void *arg)
>   
>   	if (dt_printf_format(dtp, pfw->pfw_fp, pfw->pfw_argv, rec, nrecs,
>   			     aggdata->dtada_data, aggdata->dtada_size,
> -			     aggsdata, naggvars) == -1)
> -		return (pfw->pfw_err = dtp->dt_errno);
> +			     aggsdata, naggvars) == -1) {
> +		pfw->pfw_err = dtp->dt_errno;
> +		return DTRACE_AGGWALK_ERROR;
> +	}
>   
>   	/*
>   	 * For each aggregation, indicate that it has been printed, casting
> @@ -1855,7 +1859,7 @@ dt_fprintas(const dtrace_aggdata_t **aggsdata, int naggvars, void *arg)
>   		((dtrace_aggdesc_t *)agg)->dtagd_flags |= DTRACE_AGD_PRINTED;
>   	}
>   
> -	return 0;
> +	return DTRACE_AGGWALK_NEXT;
>   }
>   
>   int