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

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add
Date: Mon, 17 Aug 2020 15:22:15 +0200	[thread overview]
Message-ID: <64b289c5-f65c-3227-6856-d0a2e57db606@redhat.com> (raw)
In-Reply-To: <20200817124951.GJ11402@linux.fritz.box>

[-- Attachment #1.1: Type: text/plain, Size: 2133 bytes --]

On 17.08.20 14:49, Kevin Wolf wrote:
> Am 17.08.2020 um 13:41 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> nbd-server-add tries to be convenient and adds two questionable
>>> features that we don't want to share in block-export-add, even for NBD
>>> exports:
>>>
>>> 1. When requesting a writable export of a read-only device, the export
>>>    is silently downgraded to read-only. This should be an error in the
>>>    context of block-export-add.
>>>
>>> 2. When using a BlockBackend name, unplugging the device from the guest
>>>    will automatically stop the NBD server, too. This may sometimes be
>>>    what you want, but it could also be very surprising. Let's keep
>>>    things explicit with block-export-add. If the user wants to stop the
>>>    export, they should tell us so.
>>>
>>> Move these things into the nbd-server-add QMP command handler so that
>>> they apply only there.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  include/block/nbd.h   |  3 ++-
>>>  block/export/export.c | 44 ++++++++++++++++++++++++++++++++++++++-----
>>>  blockdev-nbd.c        | 10 ++++------
>>>  nbd/server.c          | 19 ++++++++++++-------
>>>  qemu-nbd.c            |  3 +--
>>>  5 files changed, 58 insertions(+), 21 deletions(-)

[...]

>>> +    }
>>> +
>>> +    export = blk_exp_add(&export_opts, errp);
>>> +    if (!export) {
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * 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);
>>> +    }
>>>  }
>>
>> The longer it gets, the more I think maybe it should be in some NBD file
>> like blockdev-nbd.c after all.
> 
> Fair enough. Though I think blockdev-nbd.c in the root directory is
> something that shouldn't even exist.

Absolutely.  But unless you (or someone™ else) doesn’t do anything about
it, we may as well continue to (ab)use it. O:)

Max

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]