Re: [PATCH 07/11] libio: Extract __printf_buffer_asprintf_init from asprintf implementation

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 09/02/24 12:25, Florian Weimer wrote:
> And __printf_buffer_asprintf_free as well.  This will allow to reuse
> the asprintf implementation for more general buffer handling, similar
> to open_memstream, but without the mandatory heap allocation.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  include/printf_buffer.h | 30 +++++++++++++++++++++++++-----
>  libio/vasprintf.c       | 40 +++++++++++++++++-----------------------
>  2 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index fb0b42178e..cee5afb581 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -317,6 +317,31 @@ void __printf_buffer_snprintf_init (struct __printf_buffer_snprintf *,
>  int __printf_buffer_snprintf_done (struct __printf_buffer_snprintf *)
>    attribute_hidden;
>  
> +/* Size of the initial on-stack buffer for asprintf.  It should be
> +   large enough to copy almost all asprintf usages with just a single
> +   (final, correctly sized) heap allocation.  */
> +#define PRINTF_BUFFER_SIZE_ASPRINTF 200

On my system the journactl message has a mean size of 124 and a median
of so I guess a size of 200 should cover most of the usage. 

> +
> +struct __printf_buffer_asprintf
> +{
> +  /* base.write_base points either to a heap-allocated buffer, or to
> +     the direct array below.  */
> +  struct __printf_buffer base;
> +
> +  /* Initial allocation.  */
> +  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
> +};
> +
> +/* Sets up BUF for writing via __printf_buffer.  */
> +void __printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
> +  attribute_hidden;
> +
> +/* Deallocates any allocated memory in *BUF.  (This is not the usual
> +   done routine for asprintf because that has to preserve allocation.)
> +   Always returns -1 to indicate an error.  */
> +int __printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
> +  attribute_hidden;
> +
>  /* Flush function implementations follow.  They are called from
>     __printf_buffer_flush.  Generic code should not call these flush
>     functions directly.  Some modes have inline implementations.  */
> @@ -363,11 +388,6 @@ void __wprintf_buffer_flush_to_file (struct __wprintf_buffer_to_file *)
>  /* Temporary buffer used during floating point digit translation.  */
>  #define PRINTF_BUFFER_SIZE_DIGITS 64
>  
> -/* Size of the initial on-stack buffer for asprintf.  It should be
> -   large enough to copy almost all asprintf usages with just a single
> -   (final, correctly sized) heap allocation.  */
> -#define PRINTF_BUFFER_SIZE_ASPRINTF 200
> -
>  /* This should cover most of the packet-oriented file descriptors,
>     where boundaries between writes could be visible to readers.  But
>     it is still small enough not to cause too many stack overflow issues.  */
> diff --git a/libio/vasprintf.c b/libio/vasprintf.c
> index edcfab2323..f8413eedfe 100644
> --- a/libio/vasprintf.c
> +++ b/libio/vasprintf.c
> @@ -34,18 +34,6 @@
>  #include <string.h>
>  #include <printf_buffer.h>
>  
> -struct __printf_buffer_asprintf
> -{
> -  /* base.write_base points either to a heap-allocated buffer, or to
> -     the direct array below.  */
> -  struct __printf_buffer base;
> -
> -  /* Initial allocation.  200 should be large enough to copy almost
> -     all asprintf usages with just a single (final, correctly sized)
> -     heap allocation.  */
> -  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
> -};
> -
>  void
>  __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>  {
> @@ -90,23 +78,32 @@ __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>    buf->base.write_end = new_buffer + new_size;
>  }
>  
> +void
> +__printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
> +{
> +  __printf_buffer_init (&buf->base, buf->direct, array_length (buf->direct),
> +			__printf_buffer_mode_asprintf);
> +}
> +
> +int
> +__printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
> +{
> +  if (buf->base.write_base != buf->direct)
> +    free (buf->base.write_base);
> +  return -1;
> +}
>  
>  int
>  __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>  		      unsigned int mode_flags)
>  {
>    struct __printf_buffer_asprintf buf;
> -  __printf_buffer_init (&buf.base, buf.direct, array_length (buf.direct),
> -			__printf_buffer_mode_asprintf);
> +  __printf_buffer_asprintf_init (&buf);
>  
>    __vprintf_buffer (&buf.base, format, args, mode_flags);
>    int done = __printf_buffer_done (&buf.base);
>    if (done < 0)
> -    {
> -      if (buf.base.write_base != buf.direct)
> -	free (buf.base.write_base);
> -      return done;
> -    }
> +    return __printf_buffer_asprintf_free (&buf);
>  
>    /* Transfer to the final buffer.  */
>    char *result;
> @@ -122,10 +119,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>      {
>        result = realloc (buf.base.write_base, size + 1);
>        if (result == NULL)
> -	{
> -	  free (buf.base.write_base);
> -	  return -1;
> -	}
> +	return __printf_buffer_asprintf_free (&buf);
>      }
>  
>    /* Add NUL termination.  */