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

Message ID 345c3f94450a38be9408f158a211c82a657034b1.1707491940.git.fweimer@redhat.com (mailing list archive)
State Accepted
Delegated to: Adhemerval Zanella Netto
Headers
Series Build getdomainname, gethostname, syslog with fortification |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Florian Weimer Feb. 9, 2024, 3:25 p.m. UTC
  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.
---
 include/printf_buffer.h | 30 +++++++++++++++++++++++++-----
 libio/vasprintf.c       | 40 +++++++++++++++++-----------------------
 2 files changed, 42 insertions(+), 28 deletions(-)
  

Comments

Adhemerval Zanella Feb. 16, 2024, 2:04 p.m. UTC | #1
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.  */
  

Patch

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
+
+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.  */