Re: [Qemu-devel] [PATCH v8 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Max Reitz]

On 2014-11-26 at 18:41, John Snow wrote:
> From: Fam Zheng <address@hidden>
>
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
>
> There are two bitmap use modes for sync=dirty-bitmap:
>
>   - reset: backup job makes a copy of bitmap and resets the original
>     one.
>   - consume: backup job makes the original anonymous (invisible to user)
>     and releases it after use.
>
> Signed-off-by: Fam Zheng <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> ---
>   block.c                   |   5 ++
>   block/backup.c            | 130 ++++++++++++++++++++++++++++++++++++++--------
>   block/mirror.c            |   4 ++
>   blockdev.c                |  18 ++++++-
>   hmp.c                     |   4 +-
>   include/block/block.h     |   1 +
>   include/block/block_int.h |   6 +++
>   qapi/block-core.json      |  30 +++++++++--
>   qmp-commands.hx           |   7 +--
>   9 files changed, 175 insertions(+), 30 deletions(-)

[snip]

> diff --git a/block/backup.c b/block/backup.c
> index 792e655..2aab68f 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -386,6 +438,36 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
> *target,
>           return;
>       }
>   
> +    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> +        if (!sync_bitmap) {
> +            error_setg(errp, "must provide a valid bitmap name for "
> +                             "\"dirty-bitmap\" sync mode");
> +            return;
> +        }
> +
> +        switch (bitmap_mode) {
> +        case BITMAP_USE_MODE_RESET:
> +            original = sync_bitmap;
> +            sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
> +            bdrv_reset_dirty_bitmap(bs, original);
> +            break;
> +        case BITMAP_USE_MODE_CONSUME:
> +            bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
> +            break;
> +        default:
> +            error_setg(errp, "Invalid BitmapUseMode (%s) given to 
> backup_start",
> +                       BitmapUseMode_lookup[bitmap_mode]);
> +            return;

Well, this is basically the opposite of what I'd done, but making it 
difficult by formatting a nice error message for the user in places 
which should never be reached instead of just aborting is not wrong, in 
my opinion (I do know that others have other opinions).

[snip]

> diff --git a/blockdev.c b/blockdev.c
> index 276a31b..1a56959 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2337,7 +2342,18 @@ void qmp_drive_backup(const char *device, const char 
> *target,
>   
>       bdrv_set_aio_context(target_bs, aio_context);
>   
> -    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> +    if (has_bitmap) {
> +        bmap = bdrv_find_dirty_bitmap(bs, bitmap);
> +        if (!bmap) {
> +            error_setg(errp, "A bitmap name was given, but bitmap '%s' could not be 
> found",
> +                       bitmap);

Well, I do know I said that I'd translate "has_bitmap was set" to "A 
bitmap name was specified", but I assumed you'd remove the redundancy 
yourself. ;-)

Since this line needs to be reworked anyway (see Fam's reply), it can be 
(imho) shortened to "Bitmap '%s' could not be found". The user should 
know that he/she has specified a bitmap, and that it did have that name; 
the only thing he/she doesn't know so far is that qemu failed to find 
that bitmap, so that's the information we need to give.

With that line somehow shortened/split to 80 characters (I'd be fine 
with keeping its content as-is, I just think it'd sound strange to the 
user):

Reviewed-by: Max Reitz <address@hidden>