Libguestfs - Feb 2022 - [PATCH libnbd 3/8] copy: Extract create_command and create_buffer helpers

Creating a new command requires lot of boilerplate that makes it harder
to focus on the interesting code. Extract a helpers to create a command,
and the command slice buffer.

create_buffer is called only once, but the compiler is smart enough to
inline it, and adding it makes the code much simpler.

This change is a refactoring except fixing perror() message for calloc()
failure.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/multi-thread-copying.c | 87 +++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index 2d16d2df..855d1ba4 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -128,20 +128,22 @@ multi_thread_copying (void)
 
   free (workers);
 }
 
 static void wait_for_request_slots (uintptr_t index);
 static unsigned in_flight (uintptr_t index);
 static void poll_both_ends (uintptr_t index);
 static int finished_read (void *vp, int *error);
 static int free_command (void *vp, int *error);
 static void fill_dst_range_with_zeroes (struct command *command);
+static struct command *create_command (uint64_t offset, size_t len, bool zero,
+                                       uintptr_t index);
 
 /* There are 'threads' worker threads, each copying work ranges from
  * src to dst until there are no more work ranges.
  */
 static void *
 worker_thread (void *indexp)
 {
   uintptr_t index = (uintptr_t) indexp;
   uint64_t offset, count;
   extent_list exts = empty_vector;
@@ -150,71 +152,43 @@ worker_thread (void *indexp)
     size_t i;
 
     assert (0 < count && count <= THREAD_WORK_SIZE);
     if (extents)
       src->ops->get_extents (src, index, offset, count, &exts);
     else
       default_get_extents (src, index, offset, count, &exts);
 
     for (i = 0; i < exts.len; ++i) {
       struct command *command;
-      struct buffer *buffer;
-      char *data;
       size_t len;
 
       if (exts.ptr[i].zero) {
         /* The source is zero so we can proceed directly to skipping,
          * fast zeroing, or writing zeroes at the destination.
          */
-        command = calloc (1, sizeof *command);
-        if (command == NULL) {
-          perror ("malloc");
-          exit (EXIT_FAILURE);
-        }
-        command->offset = exts.ptr[i].offset;
-        command->slice.len = exts.ptr[i].length;
-        command->slice.base = 0;
-        command->index = index;
+        command = create_command (exts.ptr[i].offset, exts.ptr[i].length,
+                                  true, index);
         fill_dst_range_with_zeroes (command);
       }
 
       else /* data */ {
         /* As the extent might be larger than permitted for a single
          * command, we may have to split this into multiple read
          * requests.
          */
         while (exts.ptr[i].length > 0) {
           len = exts.ptr[i].length;
           if (len > request_size)
             len = request_size;
-          data = malloc (len);
-          if (data == NULL) {
-            perror ("malloc");
-            exit (EXIT_FAILURE);
-          }
-          buffer = calloc (1, sizeof *buffer);
-          if (buffer == NULL) {
-            perror ("malloc");
-            exit (EXIT_FAILURE);
-          }
-          buffer->data = data;
-          buffer->refs = 1;
-          command = calloc (1, sizeof *command);
-          if (command == NULL) {
-            perror ("malloc");
-            exit (EXIT_FAILURE);
-          }
-          command->offset = exts.ptr[i].offset;
-          command->slice.len = len;
-          command->slice.base = 0;
-          command->slice.buffer = buffer;
-          command->index = index;
+
+          command = create_command (exts.ptr[i].offset, len,
+                                    false, index);
 
           wait_for_request_slots (index);
 
           /* Begin the asynch read operation. */
           src->ops->asynch_read (src, command,
                                  (nbd_completion_callback) {
                                    .callback = finished_read,
                                    .user_data = command,
                                  });
 
@@ -331,20 +305,67 @@ poll_both_ends (uintptr_t index)
     else if ((fds[1].revents & POLLOUT) != 0)
       dst->ops->asynch_notify_write (dst, index);
     else if ((fds[1].revents & (POLLERR | POLLNVAL)) != 0) {
       errno = ENOTCONN;
       perror (dst->name);
       exit (EXIT_FAILURE);
     }
   }
 }
 
+/* Create a new buffer. */
+static struct buffer*
+create_buffer (size_t len)
+{
+  struct buffer *buffer;
+
+  buffer = calloc (1, sizeof *buffer);
+  if (buffer == NULL) {
+    perror ("calloc");
+    exit (EXIT_FAILURE);
+  }
+
+  buffer->data = malloc (len);
+  if (buffer->data == NULL) {
+    perror ("malloc");
+    exit (EXIT_FAILURE);
+  }
+
+  buffer->refs = 1;
+
+  return buffer;
+}
+
+/* Create a new command for read or zero. */
+static struct command *
+create_command (uint64_t offset, size_t len, bool zero, uintptr_t index)
+{
+  struct command *command;
+
+  command = calloc (1, sizeof *command);
+  if (command == NULL) {
+    perror ("calloc");
+    exit (EXIT_FAILURE);
+  }
+
+  command->offset = offset;
+  command->slice.len = len;
+  command->slice.base = 0;
+
+  if (!zero)
+    command->slice.buffer = create_buffer (len);
+
+  command->index = index;
+
+  return command;
+}
+
 /* Create a sub-command of an existing command.  This creates a slice
  * referencing the buffer of the existing command without copying.
  */
 static struct command *
 create_subcommand (struct command *command, uint64_t offset, size_t len,
                    bool zero)
 {
   const uint64_t end = command->offset + command->slice.len;
   struct command *newcommand;
 
-- 
2.35.1

On Sun, Feb 20, 2022 at 02:13:58PM +0200, Nir Soffer
wrote:
> Creating a new command requires lot of boilerplate that makes it harder
> to focus on the interesting code. Extract a helpers to create a command,
> and the command slice buffer.
> 
> create_buffer is called only once, but the compiler is smart enough to
> inline it, and adding it makes the code much simpler.
> 
> This change is a refactoring except fixing perror() message for calloc()
> failure.
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  copy/multi-thread-copying.c | 87 +++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 33 deletions(-)
>        if (exts.ptr[i].zero) {
>          /* The source is zero so we can proceed directly to skipping,
>           * fast zeroing, or writing zeroes at the destination.
>           */
> -        command = calloc (1, sizeof *command);
> -        if (command == NULL) {
> -          perror ("malloc");
> -          exit (EXIT_FAILURE);
> -        }
> -        command->offset = exts.ptr[i].offset;
> -        command->slice.len = exts.ptr[i].length;
> -        command->slice.base = 0;
This assignment is dead code after calloc,...
> -        command->index = index;
> +        command = create_command (exts.ptr[i].offset, exts.ptr[i].length,
> +                                  true, index);
>          fill_dst_range_with_zeroes (command);
>        }
>  
>        else /* data */ {
>          /* As the extent might be larger than permitted for a single
>           * command, we may have to split this into multiple read
>           * requests.
>           */
>          while (exts.ptr[i].length > 0) {
>            len = exts.ptr[i].length;
>            if (len > request_size)
>              len = request_size;
> -          data = malloc (len);
> -          if (data == NULL) {
> -            perror ("malloc");
> -            exit (EXIT_FAILURE);
> -          }
> -          buffer = calloc (1, sizeof *buffer);
> -          if (buffer == NULL) {
> -            perror ("malloc");
> -            exit (EXIT_FAILURE);
> -          }
> -          buffer->data = data;
> -          buffer->refs = 1;
> -          command = calloc (1, sizeof *command);
> -          if (command == NULL) {
> -            perror ("malloc");
> -            exit (EXIT_FAILURE);
> -          }
> -          command->offset = exts.ptr[i].offset;
> -          command->slice.len = len;
> -          command->slice.base = 0;
...as was this,...
> -          command->slice.buffer = buffer;
> -          command->index = index;
> +
> +          command = create_command (exts.ptr[i].offset, len,
> +                                    false, index);
>  
>            wait_for_request_slots (index);
>  
>            /* Begin the asynch read operation. */
>            src->ops->asynch_read (src, command,
>                                   (nbd_completion_callback) {
>                                     .callback = finished_read,
>                                     .user_data = command,
>                                   });
>  
> @@ -331,20 +305,67 @@ poll_both_ends (uintptr_t index)
>      else if ((fds[1].revents & POLLOUT) != 0)
>        dst->ops->asynch_notify_write (dst, index);
>      else if ((fds[1].revents & (POLLERR | POLLNVAL)) != 0) {
>        errno = ENOTCONN;
>        perror (dst->name);
>        exit (EXIT_FAILURE);
>      }
>    }
>  }
>  
> +/* Create a new buffer. */
> +static struct buffer*
> +create_buffer (size_t len)
> +{
> +  struct buffer *buffer;
> +
> +  buffer = calloc (1, sizeof *buffer);
> +  if (buffer == NULL) {
> +    perror ("calloc");
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  buffer->data = malloc (len);
> +  if (buffer->data == NULL) {
> +    perror ("malloc");
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  buffer->refs = 1;
> +
> +  return buffer;
> +}
> +
> +/* Create a new command for read or zero. */
> +static struct command *
> +create_command (uint64_t offset, size_t len, bool zero, uintptr_t index)
> +{
> +  struct command *command;
> +
> +  command = calloc (1, sizeof *command);
> +  if (command == NULL) {
> +    perror ("calloc");
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  command->offset = offset;
> +  command->slice.len = len;
> +  command->slice.base = 0;
...but keeping it here for the sake of refactoring is fine.
> +
> +  if (!zero)
> +    command->slice.buffer = create_buffer (len);
> +
> +  command->index = index;
> +
> +  return command;
> +}
ACK

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