qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/5] ebpf: add formal error reporting to all APIs


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 3/5] ebpf: add formal error reporting to all APIs
Date: Wed, 7 Aug 2024 09:39:57 +0200
User-agent: Mozilla Thunderbird

On 7/8/24 09:37, Philippe Mathieu-Daudé wrote:
On 6/8/24 17:20, Daniel P. Berrangé wrote:
On Tue, Aug 06, 2024 at 05:11:55PM +0200, Philippe Mathieu-Daudé wrote:
On 6/8/24 16:56, Daniel P. Berrangé wrote:
The eBPF code is currently reporting error messages through trace
events. Trace events are fine for debugging, but they are not to be
considered the primary error reporting mechanism, as their output
is inaccessible to callers.

This adds an "Error **errp" parameter to all methods which have
important error scenarios to report to the caller.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
   ebpf/ebpf_rss.c     | 59 ++++++++++++++++++++++++++++++++++++---------
   ebpf/ebpf_rss.h     | 10 +++++---
   hw/net/virtio-net.c |  7 +++---
   3 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
index aa7170d997..59854c8b51 100644
--- a/ebpf/ebpf_rss.c
+++ b/ebpf/ebpf_rss.c
@@ -47,13 +47,14 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
       return ctx != NULL && (ctx->obj != NULL || ctx->program_fd != -1);
   }
-static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
+static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx, Error **errp)
   {
       ctx->mmap_configuration = mmap(NULL, qemu_real_host_page_size(),
                                      PROT_READ | PROT_WRITE, MAP_SHARED,
                                      ctx->map_configuration, 0);
       if (ctx->mmap_configuration == MAP_FAILED) {
           trace_ebpf_error("eBPF RSS", "can not mmap eBPF configuration array");
+        error_setg(errp, "Unable to map eBPF configuration array");
           return false;
       }
       ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
@@ -61,6 +62,7 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
                                      ctx->map_toeplitz_key, 0);
       if (ctx->mmap_toeplitz_key == MAP_FAILED) {
           trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz key");
+        error_setg(errp, "Unable to map eBPF toeplitz array");
           goto toeplitz_fail;
       }
       ctx->mmap_indirections_table = mmap(NULL, qemu_real_host_page_size(),
@@ -68,6 +70,7 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
                                      ctx->map_indirections_table, 0);
       if (ctx->mmap_indirections_table == MAP_FAILED) {
           trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection table");
+        error_setg(errp, "Unable to map eBPF indirection array");

Aren't these trace_ebpf_error() calls redundant now?

Yes & no. Errors propagated up the call stack don't get included in
any trace output, and so if a caller doesn't log them anywhere they
can thus be invisible to someone just looking at trace output.

I could remove them all from the eBPF code though, and put a single
trace event in the hw/net/virtio-net.c file instead ? Bit of a bike
shed colouring exercise to decide which is best though.

No problem, I'm fine with this patch.

Note from experience (although pre-existing in this patch), trace
events can be very verbose, and a what makes them powerful is we
can filter particular ones. The following pattern isn't practical
to filter:

   trace_foo_error(const char *error_msg);

I guess I meant:

    trace_foo_error(const char *const error_msg);

(where the string is known at build time).

While a bit tedious to add, having a single trace event per error
is way more useful.

Regards,

Phil.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]