[DTrace-devel] [PATCH] bpf, perf: helper functions should return -errno on failure

[Eugene Loh]

One can argue about whether these return values "make sense" or "are 
more convenient," but this is a big departure from "similar" functions, 
including what people are used to thinking of as bpf() and 
perf_event_open().  That is, these functions normally return -1 on 
error, with errno indicating the error.

On 11/29/23 16:11, Kris Van Hees via DTrace-devel wrote:
> The bpf() and perf_event_open() helper functions were returning -1 on
> failure and callers were supposed to check errno.  Since the only valid
> values are >= 0, they now return -errno on failure so callers can just
> check the return value rather than errno.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_bpf.c         | 29 ++++++++++++++++++-----------
>   libdtrace/dt_prov_rawtp.c  |  4 ++--
>   libdtrace/dt_provider_tp.c |  2 +-
>   3 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 6c9758a1..8034af48 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -31,14 +31,20 @@ int
>   perf_event_open(struct perf_event_attr *attr, pid_t pid,
>   		    int cpu, int group_fd, unsigned long flags)
>   {
> -	return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd,
> -		       flags | PERF_FLAG_FD_CLOEXEC);
> +	int	rc;
> +
> +	rc = syscall(__NR_perf_event_open, attr, pid, cpu, group_fd,
> +		     flags | PERF_FLAG_FD_CLOEXEC);
> +	return rc >= 0 ? rc : -errno;
>   }
>   
>   int
>   bpf(enum bpf_cmd cmd, union bpf_attr *attr)
>   {
> -	return syscall(__NR_bpf, cmd, attr, sizeof(union bpf_attr));
> +	int	rc;
> +
> +	rc = syscall(__NR_bpf, cmd, attr, sizeof(union bpf_attr));
> +	return rc >= 0 ? rc : -errno;
>   }
>   
>   static int
> @@ -92,7 +98,7 @@ dt_bpf_prog_load(enum bpf_prog_type prog_type, const dtrace_difo_t *dp,
>   	/* Syscall could return EAGAIN - try at most 5 times. */
>   	do {
>   		fd = bpf(BPF_PROG_LOAD, &attr);
> -	} while (fd < 0 && errno == EAGAIN && ++i < 5);
> +	} while (fd == -EAGAIN && ++i < 5);
>   
>   	return fd;
>   }
> @@ -143,7 +149,7 @@ dt_bpf_map_create_meta(enum bpf_map_type otype, const char *name,
>   
>   	ifd =  bpf(BPF_MAP_CREATE, &attr);
>   	if (ifd < 0)
> -		return -1;
> +		return ifd;
>   
>   	/* Create the map-of-maps. */
>   	memset(&attr, 0, sizeof(attr));
> @@ -976,7 +982,7 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>   {
>   	size_t				logsz;
>   	char				*log;
> -	int				rc, origerrno = 0;
> +	int				rc, origerr = 0;
>   	const dtrace_probedesc_t	*pdp = prp->desc;
>   	char				*p, *q;
>   
> @@ -996,7 +1002,7 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>   		if (rc >= 0)
>   			return rc;
>   
> -		origerrno = errno;
> +		origerr = -rc;
>   	}
>   
>   	if (dtp->dt_options[DTRACEOPT_BPFLOGSIZE] != DTRACEOPT_UNSET)
> @@ -1008,18 +1014,19 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>   	rc = dt_bpf_prog_load(prp->prov->impl->prog_type, dp, 4 | 2 | 1,
>   			      log, logsz);
>   	if (rc < 0) {
> -		char msg[64];
> +		char	msg[64];
> +		int	err = -rc;
>   
>   		snprintf(msg, sizeof(msg),
>   			 "BPF program load for '%s:%s:%s:%s' failed",
>   		         pdp->prv, pdp->mod, pdp->fun, pdp->prb);
> -		if (errno == EPERM)
> +		if (err == EPERM)
>   			return dt_bpf_lockmem_error(dtp, msg);
>   		dt_bpf_error(dtp, "%s: %s\n", msg,
> -		     strerror(origerrno ? origerrno : errno));
> +		     strerror(origerr ? origerr : err));
>   
>   		/* check whether we have an incomplete BPF log */
> -		if (errno == ENOSPC) {
> +		if (err == ENOSPC) {
>   			fprintf(stderr,
>   				"BPF verifier log is incomplete and is not reported.\n"
>   				"Set DTrace option 'bpflogsize' to some greater size for more output.\n"
> diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
> index 1ff0a311..24104dd5 100644
> --- a/libdtrace/dt_prov_rawtp.c
> +++ b/libdtrace/dt_prov_rawtp.c
> @@ -181,11 +181,11 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>   		dif.dtdo_len = 2;
>   
>   		bpf_fd = dt_bpf_prog_load(dt_rawtp.prog_type, &dif, 0, NULL, 0);
> -		if (bpf_fd == -1)
> +		if (bpf_fd < 0)
>   			continue;
>   		rtp_fd = dt_bpf_raw_tracepoint_open(prp->desc->prb, bpf_fd);
>   		close(bpf_fd);
> -		if (rtp_fd == -1)
> +		if (rtp_fd < 0)
>   			continue;
>   		close(rtp_fd);
>   		break;
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index f3e03323..23766b2a 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -75,7 +75,7 @@ dt_tp_attach(dtrace_hdl_t *dtp, tp_probe_t *tpp, int bpf_fd)
>   
>   		fd = perf_event_open(&attr, -1, 0, -1, 0);
>   		if (fd < 0)
> -			return -errno;
> +			return fd;
>   
>   		tpp->event_fd = fd;
>   	}