[DTrace-devel] [PATCH] bpf, perf: helper functions should return -errno on failure
Kris Van Hees
kris.van.hees at oracle.com
Thu Nov 30 21:52:44 UTC 2023
On Thu, Nov 30, 2023 at 04:21:58PM -0500, Eugene Loh via DTrace-devel wrote:
> 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.
One could argue that, but depending on errno is dangerous unless you do it
in the immediate caller, because anything further along the call chain might
affect the errno value. Since we are returning a value anyway, and we know
that the valid value of these functions can only be a non-negative value, it
is reasonable to make use of the return value to pass the actual error code
rather than relying on a global variable.
Since the only safe way to use errno would be to have the actual caller of
bpf() and perf_event_open() immediately check errno if those function return
01, and then would need to pass that error code to their callers as a return
value or using some other non-errno mechanism, why not have those two
functions simply do that for us? An earlier version of this patch actually
modified all the callers, and that seemed just plain silly because it was just
code duplication. The logical place to check for the -1 value and to then
look at errno is at the lowest level, and that is what thiss patch does.
Can it get your reviewed-by, please?
> 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;
> > }
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list