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.
