Re: [PATCH v3 19/33] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake()

[Eric Blake]

On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be reused in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 99 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 57 insertions(+), 42 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5e63caaf4b..03ffe95231 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -318,6 +318,50 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
>      return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
>  }
>  
> +/*
> + * Check s->info updated by negotiation process.

The parameter name is bs, not s; so this comment is a bit confusing...

> + * Update @bs correspondingly to new options.
> + */
> +static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVNBDState *s = (BDRVNBDState *)bs->opaque;

...until here.  Maybe rewrite the entire comment as:

Update @bs with information learned during a completed negotiation
process.  Return failure if the server's advertised options are
incompatible with the client's needs.

> +    int ret;
> +
> +    if (s->x_dirty_bitmap) {
> +        if (!s->info.base_allocation) {
> +            error_setg(errp, "requested x-dirty-bitmap %s not found",
> +                       s->x_dirty_bitmap);
> +            return -EINVAL;
> +        }
> +        if (strcmp(s->x_dirty_bitmap, "qemu:allocation-depth") == 0) {
> +            s->alloc_depth = true;
> +        }
> +    }
> +
> +    if (s->info.flags & NBD_FLAG_READ_ONLY) {
> +        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    if (s->info.flags & NBD_FLAG_SEND_FUA) {
> +        bs->supported_write_flags = BDRV_REQ_FUA;
> +        bs->supported_zero_flags |= BDRV_REQ_FUA;

Code motion, so it is correct, but it looks odd to use = for one
assignment and |= for the other.  Using |= in both places would be
more consistent.

> +    }
> +
> +    if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) {
> +        bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP;
> +        if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) {
> +            bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK;
> +        }
> +    }
> +
> +    trace_nbd_client_handshake_success(s->export);
> +
> +    return 0;
> +}
> +
>  static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>  {
>      int ret;
> @@ -1579,49 +1623,13 @@ static int nbd_client_handshake(BlockDriverState *bs, 
> Error **errp)

As updating the comment doesn't affect code correctness,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org