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

[Eric Blake]

On Wed, Jun 09, 2021 at 08:23:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > +    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.

Oh, you raise some good points.  And it's not just bs->*flags; qemu as
server uses constant metacontext ids (base:allocation is always
context 0), but even that might not be stable across reconnect.  For
example, with my proposed patch of adding qemu:joint-allocation
metacontext, if the reason we have to reconnect is because the server
is upgrading from qemu 6.0 to 6.1 temporarily bouncing the server, and
the client was paying attention to qemu:dirty-bitmap:FOO, that context
would now have a different id.

Yeah, making this code safer across potential changes in server
information (either to fail the reconnect because the reconnected
server dropped something we were previously depending on, or
gracefully handling the downgrade, or ...) is worth leaving for a
later series while we focus on the more immediate issue of making
reconnect itself stable.

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