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

(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;

> +} > + > +/* 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".)