On Thu, Apr 15, 2021 at 12:57 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Apr 14, 2021 at 2:46 AM Florent Revest <revest@xxxxxxxxxxxx> wrote: > > > > On Wed, Apr 14, 2021 at 1:16 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <revest@xxxxxxxxxxxx> wrote: > > > > + > > > > + return err + 1; > > > > > > snprintf() already returns string length *including* terminating zero, > > > so this is wrong > > > > lib/vsprintf.c says: > > * The return value is the number of characters which would be > > * generated for the given input, excluding the trailing null, > > * as per ISO C99. > > > > Also if I look at the "no arg" test case in the selftest patch. > > "simple case" is asserted to return 12 which seems correct to me > > (includes the terminating zero only once). Am I missing something ? > > > > no, you are right, but that means that bpf_trace_printk is broken, it > doesn't do + 1 (which threw me off here), shall we fix that? Answered in the 1/6 thread > > However that makes me wonder whether it would be more appropriate to > > return the value excluding the trailing null. On one hand it makes > > sense to be coherent with other BPF helpers that include the trailing > > zero (as discussed in patch v1), on the other hand the helper is > > clearly named after the standard "snprintf" function and it's likely > > that users will assume it works the same as the std snprintf. > > > Having zero included simplifies BPF code tremendously for cases like > bpf_probe_read_str(). So no, let's stick with including zero > terminator in return size. Cool :)