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

From: Eric Blake <eblake@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 15/22] block/export: Move device to BlockExportOptions
Date: Wed, 19 Aug 2020 16:13:35 -0500	[thread overview]
Message-ID: <3eaafb76-d8f8-e407-3207-02709ffd7164@redhat.com> (raw)
In-Reply-To: <20200813162935.210070-16-kwolf@redhat.com>

On 8/13/20 11:29 AM, 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>
> ---

> +++ b/qapi/block-export.json
> @@ -62,9 +62,8 @@
>   ##
>   # @BlockExportOptionsNbd:
>   #
> -# An NBD block export.
> -#
> -# @device: The device name or node name of the node to be exported
> +# An NBD block export (options shared between nbd-server-add and the NBD branch
> +# of block-export-add).
>   #
>   # @name: Export name. If unspecified, the @device parameter is used as the
>   #        export name. (Since 2.12)
> @@ -82,8 +81,21 @@
>   # Since: 5.0
>   ##
>   { 'struct': 'BlockExportOptionsNbd',
> -  'data': {'device': 'str', '*name': 'str', '*description': 'str',
> -           '*writable': 'bool', '*bitmap': 'str' } }
> +  'data': { '*name': 'str', '*description': 'str',
> +            '*writable': 'bool', '*bitmap': 'str' } }
> +
> +##
> +# @NbdServerAddOptions:
> +#
> +# An NBD block export.
> +#
> +# @device: The device name or node name of the node to be exported
> +#
> +# Since: 5.0

5.2, now?

> +##
> +{ 'struct': 'NbdServerAddOptions',
> +  'base': 'BlockExportOptionsNbd',
> +  'data': { 'device': 'str' } }

I understand why nbd sticks with device that can name device or node, but...

>   
>   ##
>   # @nbd-server-add:
> @@ -96,7 +108,7 @@
>   # Since: 1.3.0
>   ##
>   { 'command': 'nbd-server-add',
> -  'data': 'BlockExportOptionsNbd', 'boxed': true }
> +  'data': 'NbdServerAddOptions', 'boxed': true }
>   
>   ##
>   # @NbdServerRemoveMode:
> @@ -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
> +#
>   # @writethrough: If true, caches are flushed after every write request to the
>   #                export before completion is signalled. (since: 5.2;
>   #                default: false)
> @@ -175,6 +189,7 @@
>   ##
>   { 'union': 'BlockExportOptions',
>     'base': { 'type': 'BlockExportType',
> +            'device': 'str',

for block export, I'd prefer that we mandate node name only, and naming 
it @node-name (rather than @device) feels nicer, for something that only 
new code will be using (that is, we should be encouraging the use of 
node names, especially now that libvirt has finally made that leap).

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