From: Florent Revest <firstname.lastname@example.org>
To: Rasmus Villemoes <email@example.com>
Cc: bpf <firstname.lastname@example.org>, Alexei Starovoitov <email@example.com>,
Daniel Borkmann <firstname.lastname@example.org>,
Andrii Nakryiko <email@example.com>,
KP Singh <firstname.lastname@example.org>,
Brendan Jackman <email@example.com>,
open list <firstname.lastname@example.org>,
Steven Rostedt <email@example.com>
Subject: Re: [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf
Date: Fri, 23 Apr 2021 15:45:18 +0200 [thread overview]
Message-ID: <CABRcYmJj5MTHKkOq9DT4Ju0LJmFV_8hZ+uLxwVf4bhoaL3C_aQ@mail.gmail.com> (raw)
On Fri, Apr 23, 2021 at 11:27 AM Rasmus Villemoes
> On 23/04/2021 03.15, Florent Revest wrote:
> > BPF has three formatted output helpers: bpf_trace_printk, bpf_seq_printf
> > and bpf_snprintf. Their signatures specifies that arguments are always
> > provided from the BPF world as u64s (in an array or as registers). All
> > of these helpers are currently implemented by calling functions such as
> > snprintf() whose signatures take arguments as a va_list.
> It's nitpicking, but I'd prefer to keep the details accurate as this has
> already caused enough confusion. snprintf() does not take a va_list, it
> takes a variable number of arguments.
Agreed, will fix in v2
> > To convert args from u64s to a va_list
> No, the args are not converted from u64 to a va_list, they are passed to
> said variadic function (possibly after zeroing the top half via an
> interim cast to u32) as 64-bit arguments.
> "d9c9e4db bpf: Factorize
> > bpf_trace_printk and bpf_seq_printf" introduced a bpf_printf_prepare
> > function that fills an array of arguments and an array of modifiers.
> > The BPF_CAST_FMT_ARG macro was supposed to consume these arrays and cast
> > each argument to the right size. However, the C promotion rules implies
> > that every argument is stored as a u64 in the va_list.
> "that every argument is passed as a u64".
> > To comply with the format expected by bstr_printf, certain format
> > specifiers also need to be pre-formatted: %pB and %pi6/%pi4/%pI4/%pI6.
> > Because vsnprintf subroutines for these specifiers are hard to expose,
> Indeed, as lib/vsnprintf.c reviewer I would very likely NAK that.
I imagined yes :)
> > we pre-format these arguments with calls to snprintf().
> Nothing to do with this patch, but wouldn't it be better if one just
> stored the 4 or 16 bytes of ip address in the buffer, and let
> bstr_printf do the formatting?
> The derefencing of the pointer must be done at "prepare" time, but I
> don't see the point of actually doing the textual formatting at that
> time, when the point of BINARY_PRINT is to get out of the way as fast as
> possible and punt the decimal conversion slowness to a later time.
> I also don't see why '%pB' needs to be handled specially, other than the
> fact that bin_printf doesn't handle it currently; AFAICT it should be
> just as safe as 'S' and 's' to just save the pointer and act on the
> pointer value later.
These changes would make sense to me, yes, and I tried having %pB work
like %pS and %ps yesterday, it worked like a charm for my usecase but
while reading the commit log of vsprintf.c to understand the
philosophy of this function better, I came across "841a915d20c
vsprintf: Do not have bprintf dereference pointers" that says "Since
perf and trace-cmd already can handle %p[sSfF] via saving kallsyms,
their pointers are saved and not processed during vbin_printf(). If
they were converted, it would break perf and trace-cmd, as they would
not know how to deal with the conversion.". I interpreted that as
"this args binary representation is some sort of UABI '' so I tried
not to mess around with it. But maybe I misunderstood something ?
+cc Steven who probably has context, I should have done that earlier. :)
next prev parent reply other threads:[~2021-04-23 13:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-23 1:15 [PATCH bpf-next 0/2] Implement BPF formatted output helpers with bstr_printf Florent Revest
2021-04-23 1:15 ` [PATCH bpf-next 1/2] seq_file: Add a seq_bprintf function Florent Revest
2021-04-23 1:15 ` [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf Florent Revest
2021-04-23 9:27 ` Rasmus Villemoes
2021-04-23 13:45 ` Florent Revest [this message]
2021-04-23 8:50 ` [PATCH bpf-next 0/2] Implement BPF " Rasmus Villemoes
2021-04-23 13:26 ` Florent Revest
2021-04-23 14:31 ` Rasmus Villemoes
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).