On 10/28/21 02:34, Nir Soffer wrote:
> On Wed, Oct 27, 2021 at 8:45 PM Laszlo Ersek <[email protected]> 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
Right, something similar occurred to me as well. We don't necessarily
need to distinguish QUEUED from RUNNING -- a set like { SUBMITTED,
SUCCEEDED, FAILED } would suffice.
Thanks,
Laszlo
>
> I did not have time to review the rest of the patch.
>
> Nir
>
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
