Re: [Qemu-devel] [PATCH v6 4/5] QMP: Add support for Archipelago

[Eric Blake <eblake@redhat.com>]

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

On 06/27/2014 02:24 AM, Chrysostomos Nanakos wrote:
> Introduce new enum BlockdevOptionsArchipelago.
> 
> @volume:              #Name of the Archipelago volume image
> 
> @mport:               #'mport' is the port number on which mapperd is
>                       listening. This is optional and if not specified,
>                       QEMU will make Archipelago to use the default port.
> 
> @vport:               #'vport' is the port number on which vlmcd is
>                       listening. This is optional and if not specified,
>                       QEMU will make Archipelago to use the default port.
> 
> @segment:             #optional The name of the shared memory segment
>                       Archipelago stack is using. This is optional
>                       and if not specified, QEMU will make Archipelago
>                       use the default value, 'archipelago'.
> 
> Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr>
> ---
>  qapi/block-core.json |   39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index af6b436..55eb152 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -190,8 +190,8 @@
>  # @ro: true if the backing device was open read-only
>  #
>  # @drv: the name of the block format used to open the backing device. As of
> -#       0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
> -#       'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> +#       0.14.0 this can be: 'archipelago', 'blkdebug', 'bochs', 'cloop', 'cow',
> +#       'dmg', 'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
>  #       'host_floppy', 'http', 'https', 'nbd', 'parallels', 'qcow',
>  #       'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'

The comment is wrong (although not necessarily your fault).  It should
really look more like:

As of 0.14.0, the following are supported:...
...probably some other releases mentioned, as I'm fairly certain not all
of the list was in 0.14.0...
As of 2.2, the following are also supported: 'archipelago'

>  #

> +##
> +# @BlockdevOptionsArchipelago
> +#
> +# Driver specific block device options for Archipelago.
> +#
> +# @volume:              Name of the Archipelago volume image
> +#
> +#
> +# @mport:               #optional The port number on which mapperd is

Why two blank lines?

> +#                       listening. This is optional
> +#                       and if not specified, QEMU will make Archipelago
> +#                       use the default port.

and that port number is?

> +#
> +# @vport:               #optional The port number on which vlmcd is
> +#                       listening. This is optional
> +#                       and if not specified, QEMU will make Archipelago
> +#                       use the default port.

and that port number is?

> +#
> +# @segment:             #optional The name of the shared memory segment
> +#                       Archipelago stack is using. This is optional
> +#                       and if not specified, QEMU will make Archipelago
> +#                       use the default value, 'archipelago'.
> +# Since: 2.1

At this point, I'm not sure if your series will make 2.1.  We've already
entered hard freeze, so even though your first post was before soft
freeze, it's hard to justify taking a new feature this late in the game
that still hasn't passed review.  You will probably have to change this
to 2.2 when rebasing.

> +##
> +{ 'type': 'BlockdevOptionsArchipelago',
> +  'data': { 'volume': 'str',
> +            '*mport': 'int',
> +            '*vport': 'int',
> +            '*segment': 'str' } }
> +

Looks reasonable.

> +
>  ##
>  # @BlkdebugEvent
>  #
> @@ -1347,6 +1379,7 @@
>    'base': 'BlockdevOptionsBase',
>    'discriminator': 'driver',
>    'data': {
> +      'archipelago':'BlockdevOptionsArchipelago',

Again, not necessarily your fault, but we ought to do a better job of
documenting when new union branches are added in later releases.

>        'file':       'BlockdevOptionsFile',
>        'host_device':'BlockdevOptionsFile',
>        'host_cdrom': 'BlockdevOptionsFile',
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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