Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add

[Peter Krempa]

On Thu, Aug 20, 2020 at 09:41:14 -0500, Eric Blake wrote:
> On 8/20/20 6:05 AM, Kevin Wolf wrote:
> 
> > As long as we can keep the compatibility code local to qmp_nbd_*(), I
> > don't think it's too bad. In particular because it's already written.
> > 
> > Instead of adjusting libvirt to changes in the nbd-* commands, I'd
> > rather have it change over to block-export-*. I would like to see the
> > nbd-server-add/remove commands deprecated soon after we have the
> > replacements.
> 
> Makes sense to me. So deprecate nbd-server-add in 5.2, and require
> block-export in 6.1.
> 
> 
> > > > +    /*
> > > > +     * nbd-server-add doesn't complain when a read-only device should 
> > > > be
> > > > +     * exported as writable, but simply downgrades it. This is an 
> > > > error with
> > > > +     * block-export-add.
> > > 
> > > I'd be happy with either marking this deprecated now (and fixing it in two
> > > releases), or declaring it a bug in nbd-server-add now (and fixing it
> > > outright).
> > 
> > How about deprecating nbd-server-add completely?
> 
> Works for me. Keeping the warts backwards-compatible in nbd-server-add is
> more palatable if we know we are going to drop it wholesale down the road.
> 
> > > > +    /*
> > > > +     * nbd-server-add removes the export when the named BlockBackend 
> > > > used for
> > > > +     * @device goes away.
> > > > +     */
> > > > +    on_eject_blk = blk_by_name(arg->device);
> > > > +    if (on_eject_blk) {
> > > > +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> > > > +    }
> > > 
> > > Wait - is the magic export removal tied only to exporting a drive name, 
> > > and
> > > not a node name?  So as long as libvirt is using only node names whwen
> > > adding exports, a drive being unplugged won't interfere?
> > 
> > Yes, seems so. It's the existing behaviour, I'm only moving the code
> > around.
> > 
> > > Overall, the change makes sense to me, although I'd love to see if we 
> > > could
> > > go further on the writable vs. read-only issue.
> > 
> > If nbd-server-add will be going away relatively soon, it's probably not
> > worth the trouble. But if you have reasons to keep it, maybe we should
> > consider it.
> 
> No, I'm fine with the idea of getting rid of nbd-server-add, at which point
> changing it before removal is pointless.

I agree that this is a better approach. While it's technically possible
to retrofit old commands since QMP schema introspection allows granilar
detection of what's happening in this regard using a new command is IMO
better. Specifically for APPS which might not use QMP introspection to
the extent libvirt does.