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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

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 <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'

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',
>>