bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Florent Revest <revest@chromium.org>, bpf@vger.kernel.org
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	kpsingh@kernel.org, jackmanb@google.com,
Subject: Re: [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf
Date: Fri, 23 Apr 2021 11:27:01 +0200	[thread overview]
Message-ID: <ebe46a2a-92f8-8235-ecd8-566a46e41ed5@rasmusvillemoes.dk> (raw)
In-Reply-To: <20210423011517.4069221-3-revest@chromium.org>

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.

> 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.

> 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.


  reply	other threads:[~2021-04-23  9:27 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 [this message]
2021-04-23 13:45     ` Florent Revest
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

Reply instructions:

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 \
    --in-reply-to=ebe46a2a-92f8-8235-ecd8-566a46e41ed5@rasmusvillemoes.dk \
    --to=linux@rasmusvillemoes.dk \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jackmanb@google.com \
    --cc=kpsingh@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=revest@chromium.org \


* 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).