On Fri, Apr 23, 2021 at 11:27 AM Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote: > > 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. Agreed > "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". Yes > > > > 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. :)
