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

From: Eric Blake <eblake@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Peter Krempa <pkrempa@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add
Date: Thu, 20 Aug 2020 09:41:14 -0500	[thread overview]
Message-ID: <87282a22-1263-c632-2ea1-a412b07d8519@redhat.com> (raw)
In-Reply-To: <20200820110501.GB99531@linux.fritz.box>

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.

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