Re: [RFC PATCH 15/22] block/export: Move device to BlockExportOptions

From: Kevin Wolf <kwolf@redhat.com>
To: Max Reitz <mreitz@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [RFC PATCH 15/22] block/export: Move device to BlockExportOptions
Date: Mon, 17 Aug 2020 17:27:33 +0200	[thread overview]
Message-ID: <20200817152733.GQ11402@linux.fritz.box> (raw)
In-Reply-To: <a163c834-bfc8-897a-c91f-8458b50dfdb6@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

Am 17.08.2020 um 17:13 hat Max Reitz geschrieben:
> On 13.08.20 18:29, Kevin Wolf wrote:
> > Every block export needs a block node to export, so move the 'device'
> > option from BlockExportOptionsNbd to BlockExportOptions.
> > 
> > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
> > to be wrapped by a new type NbdServerAddOptions that adds 'device' back
> > because nbd-server-add doesn't use the BlockExportOptions base type.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-export.json         | 27 +++++++++++++++++++++------
> >  block/export/export.c          | 26 ++++++++++++++++++++------
> >  block/monitor/block-hmp-cmds.c |  6 +++---
> >  blockdev-nbd.c                 |  4 ++--
> >  qemu-nbd.c                     |  2 +-
> >  5 files changed, 47 insertions(+), 18 deletions(-)
> 
> (Code diff looks good, just a question on the interface:)
> 
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 4ce163411f..d68f3bf87e 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> 
> [...]
> 
> > @@ -167,6 +179,8 @@
> >  # Describes a block export, i.e. how single node should be exported on an
> >  # external interface.
> >  #
> > +# @device: The device name or node name of the node to be exported
> > +#
> 
> Wouldn’t it be better to restrict ourselves to a node name here?
> (Bluntly ignoring the fact that doing so would make this patch an
> incompatible change, which would require some reordering in this series,
> unless we decide to just ignore that intra-series incompatibility.)

We already have intra-series incompatibility, so I wouldn't mind that.

> OTOH...  What does “better” mean.  It won’t hurt anyone to keep this as
> @device.  It’s just that I feel like if we had no legacy burden and did
> this all from scratch, we would just make it @node-name.
> 
> Did you deliberately decide against @node-name?

At first I thought I could still share code between nbd-server-add and
block-export-add, but that's not the case. Then I guess the only other
reason I have is consistency with other QMP commands. I won't pretend
it's a strong one.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]