Re: [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge

[Eric Blake]

On 12/19/18 8:29 PM, John Snow wrote:
> Especially outside of transactions, it is helpful to provide
> all-or-nothing semantics for bitmap merges. This facilitates
> the coalescing of multiple bitmaps into a single target for
> the "checkpoint" interpretation when assembling bitmaps that
> represent arbitrary points in time from component bitmaps.
>
> This is an incompatible change from the preliminary version
> of the API.

but that doesn't matter because it was in the x- namespace, and we're 
about to rename it anyway.

> Signed-off-by: John Snow <address@hidden>
> ---
>   blockdev.c           | 75 ++++++++++++++++++++++++++++++--------------
>   qapi/block-core.json | 22 ++++++-------
>   2 files changed, 62 insertions(+), 35 deletions(-)

> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> +                                                    const char *target,
> +                                                    strList *bitmaps,
> +                                                    HBitmap **backup,
> +                                                    Error **errp)
>   {

> -    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
> +    for (lst = bitmaps; lst; lst = lst->next) {
> +        src = bdrv_find_dirty_bitmap(bs, lst->value);
> +        if (!src) {
> +            error_setg(errp, "Dirty bitmap '%s' not found", lst->value);
> +            dst = NULL;
> +            goto out;
> +        }
> +
> +        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            dst = NULL;
> +            goto out;
> +        }
> +    }

Appears to be a silent no-op when given "bitmaps":[] as the source.  An 
alternative would be requiring at least one source in the list, but I 
don't see it as worth changing the patch to special-case an empty list 
differently from a no-op.

> @@ -1943,23 +1943,23 @@
>   ##
>   # @x-block-dirty-bitmap-merge:
>   #
> -# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
> -#
> -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
> -# bitmap is unchanged. On error, @dst_name is unchanged.
> +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
> +# The @bitmaps dirty bitmaps are unchanged.
> +# On error, @target is unchanged.
>   #
>   # Returns: nothing on success
>   #          If @node is not a valid block device, DeviceNotFound
> -#          If @dst_name or @src_name is not found, GenericError
> -#          If bitmaps has different sizes or granularities, GenericError
> +#          If any bitmap in @bitmaps or @target is not found, GenericError
> +#          If any of the bitmaps have different sizes or granularities,
> +#              GenericError
>   #
>   # Since: 3.0

Could do s/3.0/4.0/ to match the incompatible change here, but you do it 
in the later patch where your remove the x-.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org