From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EB88428FFE7 for ; Sat, 7 Feb 2026 23:28:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.50 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770506910; cv=none; b=m3M34XT8IQJcqIKSsvHcpzLeJSCsiipKXzoBVdjuTs4qgnJ7EfjjklZFm+MexP3MI3F1ZKugLxwKg0eBckhGKzRy6QWz2pTlVCVdhnv7pn3+HTxSbiiTeU0c4KLlXAzEyCRYAfNPVtHzjGlEDidW2pfBLfThoLyarY+KTN4Obyo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770506910; c=relaxed/simple; bh=CZXH+xww/aVOEJ7hLYQZcVkpWXEGg6kJLfCpCdcM00o=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=IMGXT9DcaK0UO+A9C6F6sVD/ii3ipVJvMwUBFeKdXwvTSDoVMKYYQmNsU8FMIQP3u0irVlyttm6rvl2ZdimsW+IfVoxLsGfOG7PH+OYDZXA0Ezkrn2DImGYly9N6o4tbz1WeqjmXAnbCxCTFD3FQ3EXQjmks2PBOHId16XJ9Hpg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=RHcPq9OE; arc=none smtp.client-ip=209.85.221.50 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="RHcPq9OE" Received: by mail-wr1-f50.google.com with SMTP id ffacd0b85a97d-436e8758b91so182197f8f.0 for ; Sat, 07 Feb 2026 15:28:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1770506908; x=1771111708; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=be4gohK9MWcRaXPi8Y6JbkqxJ0HtvzJ+GpXMNuuwe5w=; b=RHcPq9OEp/h7LptLuscQM+19BeRmoaLkQoiWQAEdYzTUZS8Aa3XbE9vAQBegDklnVs Lm6Nvq0HuxkshNC22NWuEhCPsuCWAmMlRzBmfCZmt3a1Pj12OqhFf+h/6j3H07YCiAkL 4/ibVYpD5HLXi+A+loKaB9a9kf/3bk1x9GjIFS8s+ooFBhc05QQZmzZAH9+AX4X32A6a TZSGLhR+vxuxoA7I3L1O/T/KxOtv/iXq2oH0BbNGBoStjyHN0KKn7e0LXP9uxzzEJj0E OuNYqrdVKd2QgdkIlYxnkoR/6ESg6D4MD/uL2b7B5orQ6ikv5RmBpTuxDoSBj9supJlN SUFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770506908; x=1771111708; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=be4gohK9MWcRaXPi8Y6JbkqxJ0HtvzJ+GpXMNuuwe5w=; b=wFOKg9J2CsC1vIPttuJnLo4Rh3JFPF+Inb/VGnD4Qi0bvbxO881phcqjQHPrO+MKrW 0jNMDVnyC4nrKk/khJSQfWMi/nUSmdd9dVMkW1TNIGZKh9UpcfCoiwMoUzqgRo2Gis/g WT+Aq9mDY2qw0tnkKSa5cKKOEQyeVuLRH6dlnjdDAUsP2wShL6W9fmvj4a3eWKJOmsc3 JWH5HDgsnzLL/0/64WHtnyAGj6T+uWJdoToUHLbvxuP1hnOoaGQHr7XZczxJUXLZibzp Bg5u/7YC4Mvs2s3HchNiqignOU4fjGLCfBZwjnDIhElgHJV4QdQ0WzmYSgwfh1tkNFbA b7mQ== X-Forwarded-Encrypted: i=1; AJvYcCXnIb/oz4r2PDMzlhqfddtVq+I/uQSdMJjwQqtR9Cg2S5oefr6+k5L2R65L+02TZ15jpag4tcUMBgk3N7g=@vger.kernel.org X-Gm-Message-State: AOJu0YzXv23r5RkbCP7MmaMF53DhfV5Djwqxuzgf6/a0Bssb4wnyVbsC peWOmXc556qbY9QAJkbRMMFH37wnoc1jdVzvGRi/iKWN0yuHCV3gcgE8 X-Gm-Gg: AZuq6aKqC61iXv/+BQCrpbUtoXnGDo+PGRwRT0cCiIvRJGdspM2G6egsl89cpDwLk7v Knoofgas6GqCCEfn5HFGfo1OzvoctXu3nLGj7AwCz5sWTOh9Kd/taXytC10cLIM8iw0gp5AC2jx mHqFOJ0EbMOIHdPBIVeWtEy+g1OwlXMfEBZrOmHvBo4Guo/LvabhO8aa7M4TGK2ICC223DYoEPi WV/1NNwy90+Q7T67QPjECwu8fdNf07SGc+LYs8/9zL4IWbHlRfjDsJFlwB8+3jZgSC2srenM298 MJg9bURdwWU3QV/R1xdoPD2kqXtKbY1umtLFavCmSGH/7kNewuNb5ewarlSKj/lGHuirXI+isnN S8cDYiiXkgBWtcHsHvU32VHOEQQ1aqx1IbTO0MNdNIpPAYu2nxFOzvCLxSws3NzKMq0St/V1YtA IJiP9AkK3RtwuXvp7CKmSQrbBGeg0xSgZt3XVtCo+4HrFR0bofVUYD X-Received: by 2002:a05:6000:2881:b0:431:a0:7dea with SMTP id ffacd0b85a97d-436293b394bmr10711283f8f.40.1770506907876; Sat, 07 Feb 2026 15:28:27 -0800 (PST) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4362974b230sm17147320f8f.36.2026.02.07.15.28.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 07 Feb 2026 15:28:26 -0800 (PST) Date: Sat, 7 Feb 2026 23:28:25 +0000 From: David Laight To: Willy Tarreau Cc: Thomas =?UTF-8?B?V2Vpw59zY2h1aA==?= , linux-kernel@vger.kernel.org, Cheng Li Subject: Re: [PATCH v2 next 02/11] tools/nolibc/printf: Move snprintf length check to callback Message-ID: <20260207232825.56b77d38@pumpkin> In-Reply-To: References: <20260206191121.3602-1-david.laight.linux@gmail.com> <20260206191121.3602-3-david.laight.linux@gmail.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Sat, 7 Feb 2026 20:12:30 +0100 Willy Tarreau wrote: > On Fri, Feb 06, 2026 at 07:11:12PM +0000, david.laight.linux@gmail.com wrote: > > From: David Laight > > > > Move output truncation to the snprintf() callback. > > This simplifies the main code and ensures the truncation will be > > correct when left-alignment is added. > > > > Add a zero length callback to 'finalise' the buffer rather than > > doing it in snprintf() itself. > > > > Signed-off-by: David Laight > > --- > > > > Changes for v2: > > - Formally patch 1 > > - Add comments about the final callback. > > > > tools/include/nolibc/stdio.h | 68 ++++++++++++++++++++++-------------- > > 1 file changed, 41 insertions(+), 27 deletions(-) > > > > diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h > > index f162cc697a73..36733ecd4261 100644 > > --- a/tools/include/nolibc/stdio.h > > +++ b/tools/include/nolibc/stdio.h > > @@ -245,15 +245,15 @@ char *fgets(char *s, int size, FILE *stream) > > * - %s > > * - unknown modifiers are ignored. > > */ > > -typedef int (*__nolibc_printf_cb)(intptr_t state, const char *buf, size_t size); > > +typedef int (*__nolibc_printf_cb)(void *state, const char *buf, size_t size); > > > > -static __attribute__((unused, format(printf, 4, 0))) > > -int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char *fmt, va_list args) > > +static __attribute__((unused, format(printf, 3, 0))) > > +int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list args) > > { > > char escape, lpref, ch; > > unsigned long long v; > > unsigned int written, width; > > - size_t len, ofs, w; > > + size_t len, ofs; > > char tmpbuf[21]; > > const char *outstr; > > > > @@ -355,17 +355,13 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char > > outstr = fmt; > > len = ofs - 1; > > flush_str: > > - if (n) { > > - w = len < n ? len : n; > > - n -= w; > > - while (width-- > w) { > > - if (cb(state, " ", 1) != 0) > > - return -1; > > - written += 1; > > - } > > - if (cb(state, outstr, w) != 0) > > + while (width-- > len) { > > + if (cb(state, " ", 1) != 0) > > return -1; > > + written += 1; > > } > > + if (cb(state, outstr, len) != 0) > > + return -1; > > > > written += len; > > do_escape: > > @@ -378,18 +374,23 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char > > > > /* literal char, just queue it */ > > } > > + > > + /* Flush/terminate any buffer. */ > > + if (cb(state, NULL, 0) != 0) > > + return -1; > > + > > I suspect this hunk is in fact part of the next patch which adds > buffering, because I don't see what there is to flush here without > any buffer. Ok, it could be 'terminate' here, and I could change the line again in the next patch. If the next patch goes in (it can be missed out) then maybe a different comment would be better. Or something more 'wooly' line 'finalise the output'. > If really needed, then I think that it's about time to > start adding a comment about the __nolibc_printf() function to explain > how it's supposed to work, because callback-driven code is unreadable, > there are hidden expectations everywhere that are super hard to guess > or verify. Something has to add the terminating '\0'. The callback function has the rest of the logic for writing to the buffer. So it saves replicating the length test (as was done before). It also lets vsnprintf() directly return the value returned from __nolibc_fprintf_cb(). The '\0' could be added after every memcpy(), but that has hard code paths when the buffer length is 0 or 1. It also helps if the compiler decides to inline vsnprintf. But yes, some of the motivation does come from the following patch. There you really do want all the write system calls to be in one function. > > > return written; > > } > > > > -static int __nolibc_fprintf_cb(intptr_t state, const char *buf, size_t size) > > +static int __nolibc_fprintf_cb(void *stream, const char *buf, size_t size) > > I must confess I'm not a big fan of the void* here. I've seen that you're > having one state for snprintf() and another one for fprintf(), maybe they > could be efficiently merged into a common printf_state ? Note that I'm not > vetoing this, I just want to be convinced that it's the best choice, and > neither the code, comments nor commit messages for now suggest so. 'void *' has to be better than intptr_t. A common state wouldn't work - all the fields are different (before and after the next patch). At least here all the functions are together and it is a straight callback. The usual problem with registering callback for later is the extreme difficulty in safely de-registering them. > > > { > > - return _fwrite(buf, size, (FILE *)state); > > + return size ? _fwrite(buf, size, stream) : 0; > > } > > > > static __attribute__((unused, format(printf, 2, 0))) > > int vfprintf(FILE *stream, const char *fmt, va_list args) > > { > > - return __nolibc_printf(__nolibc_fprintf_cb, (intptr_t)stream, SIZE_MAX, fmt, args); > > + return __nolibc_printf(__nolibc_fprintf_cb, stream, fmt, args); > > } > > > > static __attribute__((unused, format(printf, 1, 0))) > > @@ -447,26 +448,39 @@ int dprintf(int fd, const char *fmt, ...) > > return ret; > > } > > > > -static int __nolibc_sprintf_cb(intptr_t _state, const char *buf, size_t size) > > +struct __nolibc_sprintf_cb_state { > > + char *buf; > > + size_t size; > > +}; > > + > > +static int __nolibc_sprintf_cb(void *v_state, const char *buf, size_t size) > > { > > - char **state = (char **)_state; > > + struct __nolibc_sprintf_cb_state *state = v_state; > > + char *tgt; > > > > - memcpy(*state, buf, size); > > - *state += size; > > + if (size >= state->size) { > > + if (state->size <= 1) > > + return 0; > > I failed to understand that one. Don't we want to at least write the > trailing zero when there's one byte left ? A short comment explaining > that case would help. state->size is normally greater than zero. So when size if zero the first condition is false, and the *tgt = 0 line is executed later. However it is valid to call vsnprintf(NULL, 0, fmt, args). That is the only way state->size can be zero - in that case the code needs to return without writing to the buffer. > > > + size = state->size - 1; > > + } > > + tgt = state->buf; > > + if (size) { > > + state->size -= size; > > + state->buf = tgt + size; > > + memcpy(tgt, buf, size); > > + } else { > > + /* In particular from cb(NULL, 0) at the end of __nolibc_printf(). */ > > + *tgt = '\0'; > > + } > > Usually, "if/else" constructs result in larger code due to jumps. Size doesn't usually matter, what makes a big difference is the branches being mispredicted - I think that is ~20 clocks on my zen-5 (getting close the the P4-netburst values). I think the nolibc is optimised for size, but only within reason. That if/else does help document what is going on. > Here we certainly can unconditionally write the trailing zero. Writing the zero then overwriting it with 'no bytes' is too subtle (even for me). I'm sure you'd have wanted a big comment :-) > Bingo, we're > saving 9 bytes on x86_64 by moving it above. And even 17 bytes by dropping > the test on size and updating the state after the memcpy: > > if (size >= state->size) { > if (state->size <= 1) > return 0; > size = state->size - 1; > } > *state->buf = '\0'; > memcpy(state->buf, buf, size); > state->buf += size; > state->size -= size; That starts relying on the compiler assuming that the memcpy() can't be writing to state->buf and state->size. I'm not certain it can assume that in the callback function (With or without 'strict aliasing'). So the explicit local for tgt helps a lot - and does no harm. In the past I've done: x->buf += size; memcpy(x->buf - size, src, size) knowing that the compiler will just use the old value. Note that something has to write the '\0' for: snprintf(buf, 1, fmt, args); and snprintf(buf, len, ""); but not for snprintf(buf, 0, fmt, args); so just writing the the byte following the memcpy() isn't enough. David > > snprintf-patch1:000000000000003e t __nolibc_sprintf_cb > snprintf-patch1-alt1:0000000000000033 t __nolibc_sprintf_cb > snprintf-patch1-alt2:000000000000002d t __nolibc_sprintf_cb > > (not tested but worth a try). > > > return 0; > > } > > > > static __attribute__((unused, format(printf, 3, 0))) > > int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) > > { > > - char *state = buf; > > - int ret; > > + struct __nolibc_sprintf_cb_state state = { .buf = buf, .size = size }; > > > > - ret = __nolibc_printf(__nolibc_sprintf_cb, (intptr_t)&state, size, fmt, args); > > - if (ret < 0) > > - return ret; > > - buf[(size_t)ret < size ? (size_t)ret : size - 1] = '\0'; > > - return ret; > > + return __nolibc_printf(__nolibc_sprintf_cb, &state, fmt, args); > > } > > > > static __attribute__((unused, format(printf, 3, 4))) > > Willy