Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Eric Blake <eblake@redhat.com>]

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

On 10/30/2014 04:22 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>
> ---
>  blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+)
> 

> +    if (has_granularity) {
> +        if (granularity < 512 || is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");

Assuming you fix the logic of the condition, the error message is still
wrong (you allow 512, not just greater than 512).


> +++ b/qapi/block-core.json
> @@ -865,6 +865,61 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               block-dirty-bitmap-add

Do you still need to call out the command, given that it is the only
client of this type?

> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

Is it worth using type inheritance, as in:

{ 'type': 'BlockDirtyBitmapAdd',
  'base': 'BlockDirtyBitmap',
  'data': { '*granularity': 'int' } }

> +
> +##
> +# @block-dirty-bitmap-add
> +#
> +# Create a dirty bitmap with a name on the device
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @name is already taken, GenericError with an explaining message

s/explaining message/explanation/ (or even ditch the error mentions;
fewer commands are bothering with it these days)


> +
> +block-dirty-bitmap-add
> +----------------

Pad out the --- to the length of the command name.

> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "device": device name to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with. (int)

Inconsistent on whether you use trailing '.'

> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +----------------

Pad out the ---

-- 
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: 539 bytes --]