[Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap

[John Snow]

Instead of bdrv_can_store_new_bitmap, rework this as
bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
to modify the driver state because we know we ARE adding a bitmap
instead of simply being asked if we CAN store one.

As part of this symmetry, move this function next to
bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.

To cement this semantic distinction, use a bitmap object instead of the
(name, granularity) pair as this allows us to set persistence as a
condition of the "add" succeeding.

Notably, the qcow2 implementation still does not actually store the new
bitmap at add time, but merely ensures that it will be able to at store
time.

Signed-off-by: John Snow <address@hidden>
---
 block/qcow2.h                |  5 ++---
 include/block/block.h        |  2 --
 include/block/block_int.h    |  5 ++---
 include/block/dirty-bitmap.h |  3 +++
 block.c                      | 22 ---------------------
 block/dirty-bitmap.c         | 38 ++++++++++++++++++++++++++++++++++++
 block/qcow2-bitmap.c         | 24 ++++++++++++++---------
 block/qcow2.c                |  2 +-
 blockdev.c                   | 19 +++++++-----------
 9 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..95d723d3c0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-                                      const char *name,
-                                      uint32_t granularity,
+int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
+                                      BdrvDirtyBitmap *bitmap,
                                       Error **errp);
 void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                           const char *name,
diff --git a/include/block/block.h b/include/block/block.h
index f9415ed740..6d520f3b59 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, 
BlockDriverState *child,
                     Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
 
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp);
 /**
  *
  * bdrv_register_buf/bdrv_unregister_buf:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..93bbb66cd0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -537,9 +537,8 @@ struct BlockDriver {
      * field of BlockDirtyBitmap's in case of success.
      */
     int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
-    bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
-                                            const char *name,
-                                            uint32_t granularity,
+    int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
+                                            BdrvDirtyBitmap *bitmap,
                                             Error **errp);
     void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
                                                 const char *name,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8044ace63e..c37edbe05f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, 
uint32_t flags,
                             Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
+                                      BdrvDirtyBitmap *bitmap,
+                                      Error **errp);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                          const char *name,
                                          Error **errp);
diff --git a/block.c b/block.c
index e3e77feee0..6aec36b7c9 100644
--- a/block.c
+++ b/block.c
@@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
     parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp)
-{
-    BlockDriver *drv = bs->drv;
-
-    if (!drv) {
-        error_setg_errno(errp, ENOMEDIUM,
-                         "Can't store persistent bitmaps to %s",
-                         bdrv_get_device_or_node_name(bs));
-        return false;
-    }
-
-    if (!drv->bdrv_can_store_new_dirty_bitmap) {
-        error_setg_errno(errp, ENOTSUP,
-                         "Can't store persistent bitmaps to %s",
-                         bdrv_get_device_or_node_name(bs));
-        return false;
-    }
-
-    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 49646a30e6..615f8480b2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
     }
 }
 
+int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
+                                     BdrvDirtyBitmap *bitmap,
+                                     Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+
+    if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is already persistent, "
+                   "and cannot be re-added to node '%s'",
+                   bdrv_dirty_bitmap_name(bitmap),
+                   bdrv_get_device_or_node_name(bs));
+        return -EINVAL;
+    }
+
+    if (!drv) {
+        error_setg_errno(errp, ENOMEDIUM,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return -ENOMEDIUM;
+    }
+
+    if (!drv->bdrv_add_persistent_dirty_bitmap) {
+        error_setg_errno(errp, ENOTSUP,
+                         "Can't store persistent bitmaps to %s",
+                         bdrv_get_device_or_node_name(bs));
+        return -ENOTSUP;
+    }
+
+    ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
+    if (ret == 0) {
+        bdrv_dirty_bitmap_set_persistence(bitmap, true);
+    }
+
+    return ret;
+}
+
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     bdrv_dirty_bitmap_lock(bitmap);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 83aee1a42a..c3a72ca782 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
     return 0;
 }
 
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-                                      const char *name,
-                                      uint32_t granularity,
+int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
+                                      BdrvDirtyBitmap *bitmap,
                                       Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     bool found;
     Qcow2BitmapList *bm_list;
+    const char *name = bdrv_dirty_bitmap_name(bitmap);
+    uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    int ret = 0;
 
     if (s->qcow_version < 3) {
         /* Without autoclear_features, we would always have to assume
@@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState 
*bs,
          * have to drop all dirty bitmaps (defeating their purpose).
          */
         error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
+        ret = -ENOTSUP;
         goto fail;
     }
 
-    if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
+    ret = check_constraints_on_bitmap(bs, name, granularity, errp);
+    if (ret) {
         goto fail;
     }
 
     if (s->nb_bitmaps == 0) {
-        return true;
+        return 0;
     }
 
     if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
         error_setg(errp,
                    "Maximum number of persistent bitmaps is already reached");
+        ret = -EOVERFLOW;
         goto fail;
     }
 
@@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState 
*bs,
         QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
     {
         error_setg(errp, "Not enough space in the bitmap directory");
+        ret = -EOVERFLOW;
         goto fail;
     }
 
-    if (bitmap_list_load(bs, &bm_list, errp)) {
+    if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
         goto fail;
     }
 
     found = find_bitmap_by_name(bm_list, name);
     bitmap_list_free(bm_list);
     if (found) {
+        ret = -EEXIST;
         error_setg(errp, "Bitmap with the same name is already stored");
         goto fail;
     }
 
-    return true;
-
+    return 0;
 fail:
     error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
                   name, bdrv_get_device_or_node_name(bs));
-    return false;
+    return ret;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 9396d490d5..1c78eba71b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
     .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
-    .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
+    .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
     .bdrv_remove_persistent_dirty_bitmap = 
qcow2_remove_persistent_dirty_bitmap,
 };
 
diff --git a/blockdev.c b/blockdev.c
index 3f44b891eb..169a8da831 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
         disabled = false;
     }
 
-    if (persistent) {
-        aio_context = bdrv_get_aio_context(bs);
-        aio_context_acquire(aio_context);
-        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
-            goto out;
-        }
-    }
-
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
     if (bitmap == NULL) {
-        goto out;
+        return;
     }
 
     if (disabled) {
         bdrv_disable_dirty_bitmap(bitmap);
     }
 
-    bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
- out:
-    if (aio_context) {
+    if (persistent) {
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
+        if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
+            bdrv_release_dirty_bitmap(bs, bitmap);
+        }
         aio_context_release(aio_context);
     }
 }
-- 
2.20.1