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

[Chrysostomos Nanakos]

On 07/02/2014 04:58 PM, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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'

After rebasing I can add 'archipelago' in the 2.2 list. Do you agree?

> >   #
> > +##
> > +# @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?

Sorry for that, I will add the default ports.

> > +#
> > +# @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.

Any proposition for this one?

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