qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and bl


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
Date: Tue, 04 Nov 2014 10:26:18 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0

On 2014-10-30 at 04:22, Fam Zheng wrote:
The new command pair is added to manage user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.

Signed-off-by: Fam Zheng <address@hidden>
---
  blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 159 insertions(+)

Sorry if there's already been a discussion about this; I did not review this series before this version.

You are mixing up devices and BDSs here: The variable names and the QMP descriptions clearly refer to devices while you are using bdrv_find() to look for a BDS. Luckily (?), bdrv_find() actually uses blk_by_name() and blk_bs(), so it really does find the BDS for a device.

So it is not really wrong because it does the right thing(TM), but I'd prefer an explicit call to blk_by_name() and then blk_bs(); this would confuse me less, at the least.

diff --git a/blockdev.c b/blockdev.c
index eb743a9..025ba3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1803,6 +1803,61 @@ void qmp_block_set_io_throttle(const char *device, 
int64_t bps, int64_t bps_rd,
      aio_context_release(aio_context);
  }
+void qmp_block_dirty_bitmap_add(const char *device, const char *name,
+                                bool has_granularity, int64_t granularity,
+                                Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);

Yesterday I saw Markus posting http://wiki.qemu.org/CodeTransitions#Error_reporting which states to prefer error_setg() over error_set(). I don't know the reason for this, so I won't object to error_set(), though.

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

I think you meant !is_power_of_2().

+            error_setg(errp, "Granularity must be power of 2 "
+                             "and greater than 512");
+            return;
+        }
+    } else {
+        granularity = 65536;
+    }
+
+    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+}
+
+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);
+}
+
  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
  {
      const char *id = qdict_get_str(qdict, "id");
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 62b0356..a162f63 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -865,6 +865,61 @@
              '*on-target-error': 'BlockdevOnError' } }
##
+# @BlockDirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmap',
+  'data': { 'device': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+#               block-dirty-bitmap-add
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmapAdd',
+  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

Is there a reason you're creating an own object for this?

+
+##
+# @block-dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is already taken, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-add',
+  'data': 'BlockDirtyBitmapAdd' }

Why not just inline it here? I can't imagine using BlockDirtyBitmapAdd somewhere else, but I haven't looked into the rest of the series yet, so maybe it is useful to separate it.

+
+##
+# @block-dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-remove',
+  'data': 'BlockDirtyBitmap' }

I can understand not inlining the arguments here, though.

+
+##
  # @block_set_io_throttle:
  #
  # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1abd619..6f7e04c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1202,6 +1202,55 @@ Example:
  EQMP
{
+        .name       = "block-dirty-bitmap-add",
+        .args_type  = "device:B,name:s,granularity:i?",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+    },
+    {
+        .name       = "block-dirty-bitmap-remove",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+    },
+
+SQMP
+
+block-dirty-bitmap-add
+----------------

The other commands seem to use as many dashes as there are characters in the header. It probably doesn't matter in the end but I have heard the argument of OCD before.

Max

+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "device": device name to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with. (int)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
+                                                   "name": "bitmap0" } }
+<- { "return": {} }
+
+block-dirty-bitmap-remove
+----------------
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-dirty-bitmap-add.
+
+Arguments:
+
+- "device": device name to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "drive0",
+                                                      "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+    {
          .name       = "blockdev-snapshot-sync",
          .args_type  = 
"device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,




reply via email to

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