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

[Max Reitz <mreitz@redhat.com>]

On 2014-10-30 at 04:22, 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(+)

Sorry if there's already been a discussion about this; I did not review 
this series before this version.

You are mixing up devices and BDSs here: The variable names and the QMP 
descriptions clearly refer to devices while you are using bdrv_find() to 
look for a BDS. Luckily (?), bdrv_find() actually uses blk_by_name() and 
blk_bs(), so it really does find the BDS for a device.

So it is not really wrong because it does the right thing(TM), but I'd 
prefer an explicit call to blk_by_name() and then blk_bs(); this would 
confuse me less, at the least.

> diff --git a/blockdev.c b/blockdev.c
> index eb743a9..025ba3a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1803,6 +1803,61 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>       aio_context_release(aio_context);
>   }
>   
> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
> +                                bool has_granularity, int64_t granularity,
> +                                Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);

Yesterday I saw Markus posting 
http://wiki.qemu.org/CodeTransitions#Error_reporting which states to 
prefer error_setg() over error_set(). I don't know the reason for this, 
so I won't object to error_set(), though.

> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    if (has_granularity) {
> +        if (granularity < 512 || is_power_of_2(granularity)) {

I think you meant !is_power_of_2().

> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");
> +            return;
> +        }
> +    } else {
> +        granularity = 65536;
> +    }
> +
> +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +}
> +
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
>       const char *id = qdict_get_str(qdict, "id");
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 62b0356..a162f63 100644
> --- a/qapi/block-core.json
> +++ 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
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

Is there a reason you're creating an own object for this?

> +
> +##
> +# @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
> +#
> +# Since 2.3
> +##
> +{'command': 'block-dirty-bitmap-add',
> +  'data': 'BlockDirtyBitmapAdd' }

Why not just inline it here? I can't imagine using BlockDirtyBitmapAdd 
somewhere else, but I haven't looked into the rest of the series yet, so 
maybe it is useful to separate it.

> +
> +##
> +# @block-dirty-bitmap-remove
> +#
> +# Remove a dirty bitmap on the device
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explaining message
> +#
> +# Since 2.3
> +##
> +{'command': 'block-dirty-bitmap-remove',
> +  'data': 'BlockDirtyBitmap' }

I can understand not inlining the arguments here, though.

> +
> +##
>   # @block_set_io_throttle:
>   #
>   # Change I/O throttle limits for a block drive.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..6f7e04c 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1202,6 +1202,55 @@ Example:
>   EQMP
>   
>       {
> +        .name       = "block-dirty-bitmap-add",
> +        .args_type  = "device:B,name:s,granularity:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
> +    },
> +    {
> +        .name       = "block-dirty-bitmap-remove",
> +        .args_type  = "device:B,name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
> +    },
> +
> +SQMP
> +
> +block-dirty-bitmap-add
> +----------------

The other commands seem to use as many dashes as there are characters in 
the header. It probably doesn't matter in the end but I have heard the 
argument of OCD before.

Max

> +
> +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)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +----------------
> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-dirty-bitmap-add.
> +
> +Arguments:
> +
> +- "device": device name to remove dirty bitmap (json-string)
> +- "name": name of the dirty bitmap to remove (json-string)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "drive0",
> +                                                      "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>           .name       = "blockdev-snapshot-sync",
>           .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
>           .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,