On 12/19/18 9:48 PM, Eric Blake wrote:
> 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.
>
Yes, just an "FYI".
>>
>> Signed-off-by: John Snow <js...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
>
Yeah, I think I'll just leave it this way, so all the version
graduations are in the same patch.
Thank you!
