Re: [Qemu-devel] [PATCH v2 05/18] block/backup: Add mirror sync mode 'bitmap'

[Max Reitz]

On 03.07.19 23:55, John Snow wrote:
> We don't need or want a new sync mode for simple differences in
> semantics.  Create a new mode simply named "BITMAP" that is designed to
> make use of the new Bitmap Sync Mode field.
> 
> Because the only bitmap mode is 'conditional', this adds no new
> functionality to the backup job (yet). The old incremental backup mode
> is maintained as a syntactic sugar for sync=bitmap, mode=conditional.
> 
> Add all of the plumbing necessary to support this new instruction.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c            | 20 ++++++++++++--------
>  block/mirror.c            |  6 ++++--
>  block/replication.c       |  2 +-
>  blockdev.c                | 19 +++++++++++++++++--
>  include/block/block_int.h |  4 +++-
>  qapi/block-core.json      | 20 ++++++++++++++------
>  6 files changed, 51 insertions(+), 20 deletions(-)

[...]

> diff --git a/blockdev.c b/blockdev.c
> index cad300d526..d5b089a446 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3473,6 +3473,19 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>          goto out;
>      }
>  
> +    if (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL) {
> +        if (backup->has_bitmap_mode &&
> +            backup->bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) {
> +            error_setg(errp, "Bitmap sync mode must be 'conditional' "
> +                       "when using sync mode '%s'",
> +                       MirrorSyncMode_str(backup->sync));
> +            goto out;
> +        }
> +        backup->has_bitmap_mode = true;
> +        backup->sync = MIRROR_SYNC_MODE_BITMAP;
> +        backup->bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL;
> +    }
> +

A check for

  backup->sync == MIRROR_SYNC_MODE_BITMAP && !backup->has_bitmap_mode

or maybe even better

  backup->has_bitmap && !backup->has_bitmap_mode

is missing here.

Max

>      if (backup->has_bitmap) {
>          bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
>          if (!bmap) {

signature.asc
Description: OpenPGP digital signature