Re: [Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model

On Wed, Oct 27, 2021 at 8:45 PM Laszlo Ersek <lersek(a)redhat.com&gt; wrote: ... > (hopefully that means nbdkit_error is thread-safe) > >> + return ! cmd->error ? 0 : -1; > > (6) Using the ! operator with the ternary operator ?: is awkward IMO; > how about just > > return cmd->error ? -1 : 0; cmd->failed is more consistent, but I think we should get rid of this field. > >> +} >> + >> +/* Add an extent to the list of extents. */ >> +static int >> +add_extent (struct nbdkit_extents *extents, >> + uint64_t *position, uint64_t next_position, bool is_hole) >> +{ >> + uint32_t type = 0; >> + const uint64_t length = next_position - *position; >> + >> + if (is_hole) { >> + type = NBDKIT_EXTENT_HOLE; >> + /* Images opened as single link might be backed by another file in the >> + chain, so the holes are not guaranteed to be zeroes. */ >> + if (!single_link) >> + type |= NBDKIT_EXTENT_ZERO; >> + } >> + >> + assert (*position <= next_position); >> + if (*position == next_position) >> + return 0; >> + >> + if (vddk_debug_extents) >> + nbdkit_debug ("adding extent type %s at [%" PRIu64 "...%" PRIu64 "]", >> + is_hole ? "hole" : "allocated data", >> + *position, next_position-1); >> + if (nbdkit_add_extent (extents, *position, length, type) == -1) >> + return -1; >> + >> + *position = next_position; >> + return 0; >> +} >> + >> +/* Asynchronous commands are completed when this function is called. */ >> +static void >> +complete_command (void *vp, VixError err) >> +{ >> + struct command *cmd = vp; >> + >> + if (err != VIX_OK) { >> + VDDK_ERROR (err, "asynchronous %s failed", command_type_string (cmd->type)); >> + cmd->error = true; >> + } >> + >> + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&cmd->mutex); >> + cmd->completed = true; >> + pthread_cond_signal (&cmd->cond); >> +} > > Seems OK. The ordering seems to match the counterpart in > send_command_and_wait(). > > (7) Suggestion: in the definition of "struct command", document that the > mutex & condvar protect the "completed" field, and that "error" is only > meaningful once "completed" is set. (From the current comments on the > fields, I wasn't initially sure whether "error" should be checked > independently of "completed".) The real issue is having 2 fields to manage the state, allowing 4 possible states, when only 2 states are valid. We can replace the fields with: enum command_state { STATE_QUEUED, STATE_RUNNING, STATE_SUCCEEDED, STATE_FAILED }; ... struct command { enum command_state state

I did not have time to review the rest of the patch. Nir