Libguestfs - May 2023 - [PATCH nbdkit v3 5/6] server: debug: Fix error handling

Preserve errno even if the function fails.  We need to set
errno = err again on every exit path out of the function.

Check if close_memstream failed and go to the error path if so.  This
is most important on Windows where close_memstream is what actually
allocates the memory.  But even on Unix, close_memstream (which is an
alias for fclose) can return an error.

Move the error-handling code to the end of the function.

I added a comment above the 'fputs' explaining why exactly we're
using
memstream in the first place -- because it is more atomic than using
multiple fprintf.

See: https://listman.redhat.com/archives/libguestfs/2023-May/031456.html
Thanks: Laszlo Ersek
---
 server/debug.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 8afa0ae8b..177d9d5da 100644
--- a/server/debug.c
+++ b/server/debug.c
@@ -81,14 +81,8 @@ debug_common (bool in_server, const char *fs, va_list args)
   FILE *fp;
 
   fp = open_memstream (&str, &len);
-  if (fp == NULL) {
-  fail:
-    /* Try to emit what we can. */
-    errno = err;
-    vfprintf (stderr, fs, args);
-    fprintf (stderr, "\n");
-    return;
-  }
+  if (fp == NULL)
+    goto fail;
 
 #ifndef WIN32
   tty = isatty (fileno (stderr));
@@ -104,13 +98,23 @@ debug_common (bool in_server, const char *fs, va_list args)
   if (!in_server && tty) ansi_force_restore (fp);
 #endif
   fprintf (fp, "\n");
-  close_memstream (fp);
+  if (close_memstream (fp) == -1)
+    goto fail;
 
-  if (str)
-    fputs (str, stderr);
-  else
+  if (!str)
     goto fail;
 
+  /* Send it to stderr as atomically as possible. */
+  fputs (str, stderr);
+
+  errno = err;
+  return;
+
+ fail:
+  /* Try to emit what we can. */
+  errno = err;
+  vfprintf (stderr, fs, args);
+  fprintf (stderr, "\n");
   errno = err;
 }
 
-- 
2.39.2

On Tue, May 09, 2023 at 03:51:20PM +0100, Richard W.M. Jones
wrote:
> Preserve errno even if the function fails.  We need to set
> errno = err again on every exit path out of the function.
> 
> Check if close_memstream failed and go to the error path if so.  This
> is most important on Windows where close_memstream is what actually
> allocates the memory.  But even on Unix, close_memstream (which is an
> alias for fclose) can return an error.
> 
> Move the error-handling code to the end of the function.
> 
> I added a comment above the 'fputs' explaining why exactly
we're using
> memstream in the first place -- because it is more atomic than using
> multiple fprintf.
> 
> See: https://listman.redhat.com/archives/libguestfs/2023-May/031456.html
> Thanks: Laszlo Ersek
> ---
>  server/debug.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
Reviewed-by: Eric Blake <eblake at redhat.com>
> + fail:
> +  /* Try to emit what we can. */
> +  errno = err;
> +  vfprintf (stderr, fs, args);
> +  fprintf (stderr, "\n");
>    errno = err;
It might make more sense if the first line were written:

  errno = err; /* In case fs uses %m */

so that no one tries to "optimize" out what looks like a redundant
assignment when both are really required.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org