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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.