Libguestfs - Feb 2022 - [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback

In several places where asynch handlers manually call the provided
nbd_completion_callback, the value of errno is indeterminate (for
example, in file-ops.c:file_asynch_read(), the previous call to
file_synch_read() already triggered exit() on error, but does not
guarantee what is left in errno on success).  As the callback should
be paying attention to the value of *error (to be fixed in the next
patch), we are better off ensuring that we pass in a pointer to a
known-zero value; at which point, it is easier to use a dummy variable
on the stack than to mess around with errno and it's magic macro
expansion into a thread-local storage location.

Note that several callsites then check if the callback returned -1,
and if so assume that the callback has caused errno to now have a sane
value to pass on to perror.  In theory, the fact that we are no longer
passing in &errno means that if the callback assigns into *error but
did not otherwise affect errno, our perror call would no longer
reflect that value.  But in practice, since the callback never
actually returned -1, nor even assigned into *error, the call to
perror is dead code; although I have chosen to defer that additional
cleanup to the next patch.
---
 copy/file-ops.c             | 17 ++++++++++-------
 copy/multi-thread-copying.c |  8 +++++---
 copy/null-ops.c             | 12 +++++++-----
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/copy/file-ops.c b/copy/file-ops.c
index 84704341..e37a5014 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace.
- * Copyright (C) 2020 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -587,10 +587,11 @@ file_asynch_read (struct rw *rw,
                   struct command *command,
                   nbd_completion_callback cb)
 {
+  int dummy = 0;
+
   file_synch_read (rw, slice_ptr (command->slice),
                    command->slice.len, command->offset);
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
+  if (cb.callback (cb.user_data, &dummy) == -1) {
     perror (rw->name);
     exit (EXIT_FAILURE);
   }
@@ -601,10 +602,11 @@ file_asynch_write (struct rw *rw,
                    struct command *command,
                    nbd_completion_callback cb)
 {
+  int dummy = 0;
+
   file_synch_write (rw, slice_ptr (command->slice),
                     command->slice.len, command->offset);
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
+  if (cb.callback (cb.user_data, &dummy) == -1) {
     perror (rw->name);
     exit (EXIT_FAILURE);
   }
@@ -614,10 +616,11 @@ static bool
 file_asynch_zero (struct rw *rw, struct command *command,
                   nbd_completion_callback cb, bool allocate)
 {
+  int dummy = 0;
+
   if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
     return false;
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
+  if (cb.callback (cb.user_data, &dummy) == -1) {
     perror (rw->name);
     exit (EXIT_FAILURE);
   }
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index b17ca598..815b8a02 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace.
- * Copyright (C) 2020 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -393,6 +393,7 @@ finished_read (void *vp, int *error)
     bool last_is_hole = false;
     uint64_t i;
     struct command *newcommand;
+    int dummy = 0;

     /* Iterate over whole blocks in the command, starting on a block
      * boundary.
@@ -475,7 +476,7 @@ finished_read (void *vp, int *error)
     /* Free the original command since it has been split into
      * subcommands and the original is no longer needed.
      */
-    free_command (command, &errno);
+    free_command (command, &dummy);
   }

   return 1; /* auto-retires the command */
@@ -502,6 +503,7 @@ fill_dst_range_with_zeroes (struct command *command)
 {
   char *data;
   size_t data_size;
+  int dummy = 0;

   if (destination_is_zero)
     goto free_and_return;
@@ -537,7 +539,7 @@ fill_dst_range_with_zeroes (struct command *command)
   free (data);

  free_and_return:
-  free_command (command, &errno);
+  free_command (command, &dummy);
 }

 static int
diff --git a/copy/null-ops.c b/copy/null-ops.c
index a38666d6..11cd5116 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace.
- * Copyright (C) 2020-2021 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -117,8 +117,9 @@ null_asynch_write (struct rw *rw,
                    struct command *command,
                    nbd_completion_callback cb)
 {
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
+  int dummy = 0;
+
+  if (cb.callback (cb.user_data, &dummy) == -1) {
     perror (rw->name);
     exit (EXIT_FAILURE);
   }
@@ -128,8 +129,9 @@ static bool
 null_asynch_zero (struct rw *rw, struct command *command,
                   nbd_completion_callback cb, bool allocate)
 {
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
+  int dummy = 0;
+
+  if (cb.callback (cb.user_data, &dummy) == -1) {
     perror (rw->name);
     exit (EXIT_FAILURE);
   }
-- 
2.34.1

On Thu, Feb 03, 2022 at 02:25:57PM -0600, Eric Blake
wrote:
> In several places where asynch handlers manually call the provided
> nbd_completion_callback, the value of errno is indeterminate (for
> example, in file-ops.c:file_asynch_read(), the previous call to
> file_synch_read() already triggered exit() on error, but does not
> guarantee what is left in errno on success).  As the callback should
> be paying attention to the value of *error (to be fixed in the next
> patch), we are better off ensuring that we pass in a pointer to a
> known-zero value; at which point, it is easier to use a dummy variable
> on the stack than to mess around with errno and it's magic macro
> expansion into a thread-local storage location.
> 
> Note that several callsites then check if the callback returned -1,
> and if so assume that the callback has caused errno to now have a sane
> value to pass on to perror.  In theory, the fact that we are no longer
> passing in &errno means that if the callback assigns into *error but
> did not otherwise affect errno, our perror call would no longer
> reflect that value.  But in practice, since the callback never
> actually returned -1, nor even assigned into *error, the call to
> perror is dead code; although I have chosen to defer that additional
> cleanup to the next patch.
And I guess another reason not to use &errno is that it could be
updated by random system calls in the same thread, perhaps even after
we have set it.  It sounds like &errno would always be wrong.
>  copy/file-ops.c             | 17 ++++++++++-------
>  copy/multi-thread-copying.c |  8 +++++---
>  copy/null-ops.c             | 12 +++++++-----
>  3 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/copy/file-ops.c b/copy/file-ops.c
> index 84704341..e37a5014 100644
> --- a/copy/file-ops.c
> +++ b/copy/file-ops.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -587,10 +587,11 @@ file_asynch_read (struct rw *rw,
>                    struct command *command,
>                    nbd_completion_callback cb)
>  {
> +  int dummy = 0;
> +
>    file_synch_read (rw, slice_ptr (command->slice),
>                     command->slice.len, command->offset);
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -601,10 +602,11 @@ file_asynch_write (struct rw *rw,
>                     struct command *command,
>                     nbd_completion_callback cb)
>  {
> +  int dummy = 0;
> +
>    file_synch_write (rw, slice_ptr (command->slice),
>                      command->slice.len, command->offset);
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -614,10 +616,11 @@ static bool
>  file_asynch_zero (struct rw *rw, struct command *command,
>                    nbd_completion_callback cb, bool allocate)
>  {
> +  int dummy = 0;
> +
>    if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
>      return false;
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
> index b17ca598..815b8a02 100644
> --- a/copy/multi-thread-copying.c
> +++ b/copy/multi-thread-copying.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -393,6 +393,7 @@ finished_read (void *vp, int *error)
>      bool last_is_hole = false;
>      uint64_t i;
>      struct command *newcommand;
> +    int dummy = 0;
> 
>      /* Iterate over whole blocks in the command, starting on a block
>       * boundary.
> @@ -475,7 +476,7 @@ finished_read (void *vp, int *error)
>      /* Free the original command since it has been split into
>       * subcommands and the original is no longer needed.
>       */
> -    free_command (command, &errno);
> +    free_command (command, &dummy);
>    }
> 
>    return 1; /* auto-retires the command */
> @@ -502,6 +503,7 @@ fill_dst_range_with_zeroes (struct command *command)
>  {
>    char *data;
>    size_t data_size;
> +  int dummy = 0;
> 
>    if (destination_is_zero)
>      goto free_and_return;
> @@ -537,7 +539,7 @@ fill_dst_range_with_zeroes (struct command *command)
>    free (data);
> 
>   free_and_return:
> -  free_command (command, &errno);
> +  free_command (command, &dummy);
>  }
> 
>  static int
> diff --git a/copy/null-ops.c b/copy/null-ops.c
> index a38666d6..11cd5116 100644
> --- a/copy/null-ops.c
> +++ b/copy/null-ops.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020-2021 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -117,8 +117,9 @@ null_asynch_write (struct rw *rw,
>                     struct command *command,
>                     nbd_completion_callback cb)
>  {
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  int dummy = 0;
> +
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -128,8 +129,9 @@ static bool
>  null_asynch_zero (struct rw *rw, struct command *command,
>                    nbd_completion_callback cb, bool allocate)
>  {
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  int dummy = 0;
> +
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> -- 
> 2.34.1
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 02/03/22 21:25, Eric Blake wrote:
> In several places where asynch handlers manually call the provided
> nbd_completion_callback, the value of errno is indeterminate (for
> example, in file-ops.c:file_asynch_read(), the previous call to
> file_synch_read() already triggered exit() on error, but does not
> guarantee what is left in errno on success).  As the callback should
> be paying attention to the value of *error (to be fixed in the next
> patch), we are better off ensuring that we pass in a pointer to a
> known-zero value; at which point, it is easier to use a dummy variable
> on the stack than to mess around with errno and it's magic macro
> expansion into a thread-local storage location.
> 
> Note that several callsites then check if the callback returned -1,
> and if so assume that the callback has caused errno to now have a sane
> value to pass on to perror.  In theory, the fact that we are no longer
> passing in &errno means that if the callback assigns into *error but
> did not otherwise affect errno, our perror call would no longer
> reflect that value.  But in practice, since the callback never
> actually returned -1, nor even assigned into *error, the call to
> perror is dead code; although I have chosen to defer that additional
> cleanup to the next patch.
> ---
>  copy/file-ops.c             | 17 ++++++++++-------
>  copy/multi-thread-copying.c |  8 +++++---
>  copy/null-ops.c             | 12 +++++++-----
>  3 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/copy/file-ops.c b/copy/file-ops.c
> index 84704341..e37a5014 100644
> --- a/copy/file-ops.c
> +++ b/copy/file-ops.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -587,10 +587,11 @@ file_asynch_read (struct rw *rw,
>                    struct command *command,
>                    nbd_completion_callback cb)
>  {
> +  int dummy = 0;
> +
>    file_synch_read (rw, slice_ptr (command->slice),
>                     command->slice.len, command->offset);
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -601,10 +602,11 @@ file_asynch_write (struct rw *rw,
>                     struct command *command,
>                     nbd_completion_callback cb)
>  {
> +  int dummy = 0;
> +
>    file_synch_write (rw, slice_ptr (command->slice),
>                      command->slice.len, command->offset);
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -614,10 +616,11 @@ static bool
>  file_asynch_zero (struct rw *rw, struct command *command,
>                    nbd_completion_callback cb, bool allocate)
>  {
> +  int dummy = 0;
> +
>    if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
>      return false;
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
> index b17ca598..815b8a02 100644
> --- a/copy/multi-thread-copying.c
> +++ b/copy/multi-thread-copying.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -393,6 +393,7 @@ finished_read (void *vp, int *error)
>      bool last_is_hole = false;
>      uint64_t i;
>      struct command *newcommand;
> +    int dummy = 0;
> 
>      /* Iterate over whole blocks in the command, starting on a block
>       * boundary.
> @@ -475,7 +476,7 @@ finished_read (void *vp, int *error)
>      /* Free the original command since it has been split into
>       * subcommands and the original is no longer needed.
>       */
> -    free_command (command, &errno);
> +    free_command (command, &dummy);
>    }
> 
>    return 1; /* auto-retires the command */
> @@ -502,6 +503,7 @@ fill_dst_range_with_zeroes (struct command *command)
>  {
>    char *data;
>    size_t data_size;
> +  int dummy = 0;
> 
>    if (destination_is_zero)
>      goto free_and_return;
> @@ -537,7 +539,7 @@ fill_dst_range_with_zeroes (struct command *command)
>    free (data);
> 
>   free_and_return:
> -  free_command (command, &errno);
> +  free_command (command, &dummy);
>  }
> 
>  static int
> diff --git a/copy/null-ops.c b/copy/null-ops.c
> index a38666d6..11cd5116 100644
> --- a/copy/null-ops.c
> +++ b/copy/null-ops.c
> @@ -1,5 +1,5 @@
>  /* NBD client library in userspace.
> - * Copyright (C) 2020-2021 Red Hat Inc.
> + * Copyright (C) 2020-2022 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -117,8 +117,9 @@ null_asynch_write (struct rw *rw,
>                     struct command *command,
>                     nbd_completion_callback cb)
>  {
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  int dummy = 0;
> +
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> @@ -128,8 +129,9 @@ static bool
>  null_asynch_zero (struct rw *rw, struct command *command,
>                    nbd_completion_callback cb, bool allocate)
>  {
> -  errno = 0;
> -  if (cb.callback (cb.user_data, &errno) == -1) {
> +  int dummy = 0;
> +
> +  if (cb.callback (cb.user_data, &dummy) == -1) {
>      perror (rw->name);
>      exit (EXIT_FAILURE);
>    }
> 
Reviewed-by: Laszlo Ersek <lersek at redhat.com>