Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Eric Blake <eblake@redhat.com>]

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

On 03/27/2014 03:09 AM, Fam Zheng wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -2209,6 +2209,51 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @DirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               dirty-bitmap-add

Optional, but only affects dirty-bitmap-add.  You later document...

> +# @dirty-bitmap-remove
> +#
> +# Remove a dirty bitmap on the device
> +#
> +# Setting granularity has no effect here.

...that it is silently ignored where it can't be used here, and again in
7/9 for both dirty-bitmap-disable and dirty-bitmap-enable.

I think it would be smarter to do:

{ 'type': 'DirtyBitmap',
  'data': { 'device': 'str', 'name': 'str' } }

{'command': 'dirty-bitmap-add',
  'data': { 'map': 'DirtyBitmap', '*granularity': 'int' } }

Or:

{ 'type': 'DirtyBitmap',
  'data': { 'device': 'str', 'name': 'str' } }
{ 'type': 'DirtyBitmapGranularity',
  'base': 'DirtyBitmap',
  'data': { '*granularity': 'int' } }
{'command': 'dirty-bitmap-add',
  'data': 'DirtyBitmapGranularity' }


which says that the 'DirtyBitmap' struct has no optional members, and
instead of silently ignoring an optional member in 3 commands, we
instead write the one command that takes the optional argument when we
actually care about it.

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