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

[Vladimir Sementsov-Ogievskiy]

03.06.2021 19:29, Eric Blake wrote:
> 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.

Actually I see bugs here:

1. we should do =, not |=, as on reconnect info changes, so we should reset 
supported flags.

2. in-fligth requests that are in retying loops are not prepared to flags 
changing. I afraid, that some malicious server may even do some bad thing

Still, let's fix it after these series. To avoid more conflicts.

> > +    }
> > +
> > +    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>


--
Best regards,
Vladimir