5:42 a.m.
On Mon, Feb 21, 2022 at 5:03 PM Eric Blake <eblake(a)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(a)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