Re: [Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

from [PATCH v6 02/10]

+void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_dirty_bitmap_make_anon(bs, bitmap);
+    bdrv_release_dirty_bitmap(bs, bitmap);
+}

from [PATCH v6 05/10]:

+void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
+                                    Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bs, bitmap);
+}
+

there is one inconsistence:

you have check

+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }

when accessing bitmap in qmp_block_dirty_bitmap_remove, but not in qmp_block_dirty_bitmap_{enable,disable}.

Also, I think it'll be better to put similar part of these three functions into one separate function to avoid duplicates, like

static BdrvDirtyBitmap *bitmap find_dirty_bitmap(const char *device, const char *name,
                                   Error **errp)
{
    BlockDriverState *bs;
    BdrvDirtyBitmap *bitmap;

    // most simple error condition earlier
    if (!name || name[0] == '\0') {
        error_setg(errp, "Bitmap name cannot be empty");
        return NULL;
    }

    bs = bdrv_find(device);
    if (!bs) {
        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
        return NULL;
    }

    bitmap = bdrv_find_dirty_bitmap(bs, name);
    if (!bitmap) {
        error_setg(errp, "Dirty bitmap not found: %s", name);
        return NULL;
    }
    
    return bitmap;
}

-- 
Best regards,
Vladimir