Messages in this thread | | | Subject | Re: [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf | From | Rasmus Villemoes <> | Date | Fri, 23 Apr 2021 11:27:01 +0200 |
| |
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.
Rasmus
| |