Message-ID: <aYeOniYh0GUvsCeW@1wt.eu>
Date: Sat, 7 Feb 2026 20:12:30 +0100
From: Willy Tarreau <w@....eu>
To: david.laight.linux@...il.com
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
        linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH v2 next 02/11] tools/nolibc/printf: Move snprintf length
 check to callback

On Fri, Feb 06, 2026 at 07:11:12PM +0000, david.laight.linux@...il.com wrote:
> From: David Laight <david.laight.linux@...il.com>
> 
> 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 <david.laight.linux@...il.com>
> ---
> 
> 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. 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.

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

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

> +		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. Here
we certainly can unconditionally write the trailing zero. 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;

  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

Powered by blists - more mailing lists