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

[Nir Soffer]

On Mon, Feb 21, 2022 at 5:03 PM Eric Blake <eblake at redhat.com> wrote:
>
> 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

Right, I will improve this later.

>
> > +
> > +  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
>