[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v5 03/11] blockdev: n-ary bitmap merge
Date: Wed, 19 Dec 2018 20:48:21 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]