qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and b


From: John Snow
Subject: Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
Date: Fri, 16 Jan 2015 11:54:20 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0



On 01/16/2015 11:51 AM, Max Reitz wrote:
On 2015-01-16 at 11:48, John Snow wrote:


On 01/16/2015 10:36 AM, Max Reitz wrote:
On 2015-01-12 at 11:30, John Snow wrote:
From: Fam Zheng <address@hidden>

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.

The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.

This patch also introduces the 'block_dirty_bitmap_lookup' helper,
which takes a device name and a dirty bitmap name and validates the
lookup, returning NULL and setting errp if there is a problem with
either field. This helper will be re-used in future patches in this
series.

The types added to block-core.json will be re-used in future patches
in this series, see:
'qapi: Add transaction support to block-dirty-bitmap-{add, enable,
disable}'

Signed-off-by: Fam Zheng <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
  block.c               |  20 ++++++++++
  block/mirror.c        |  10 +----
  blockdev.c            | 100
++++++++++++++++++++++++++++++++++++++++++++++++++
  include/block/block.h |   1 +
  qapi/block-core.json  |  55 +++++++++++++++++++++++++++
  qmp-commands.hx       |  51 +++++++++++++++++++++++++
  6 files changed, 228 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index bfeae6b..3eb77ee 100644
--- a/block.c
+++ b/block.c
@@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, int64_t sector
      }
  }
+/**
+ * Chooses a default granularity based on the existing cluster size,
+ * but clamped between [4K, 64K]. Defaults to 64K in the case that
there
+ * is no cluster size information available.
+ */
+uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+{
+    BlockDriverInfo bdi;
+    uint64_t granularity;
+
+    if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
+        granularity = MAX(4096, bdi.cluster_size);
+        granularity = MIN(65536, granularity);
+    } else {
+        granularity = 65536;
+    }
+
+    return granularity;
+}
+
  void bdrv_dirty_iter_init(BlockDriverState *bs,
                            BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
  {
diff --git a/block/mirror.c b/block/mirror.c
index d819952..fc545f1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -667,15 +667,7 @@ static void mirror_start_job(BlockDriverState
*bs, BlockDriverState *target,
      MirrorBlockJob *s;
      if (granularity == 0) {
-        /* Choose the default granularity based on the target file's
cluster
-         * size, clamped between 4k and 64k.  */
-        BlockDriverInfo bdi;
-        if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size !=
0) {
-            granularity = MAX(4096, bdi.cluster_size);
-            granularity = MIN(65536, granularity);
-        } else {
-            granularity = 65536;
-        }
+        granularity = bdrv_get_default_bitmap_granularity(target);
      }
      assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index 5651a8e..95251c7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,6 +1173,48 @@ out_aio_context:
      return NULL;
  }
+/**
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names. Returns NULL on error,
+ * including when the BDS and/or bitmap is not found.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char
*node_ref,
+                                                  const char *name,
+ BlockDriverState
**pbs,
+                                                  Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    if (!node_ref) {
+        error_setg(errp, "Node reference cannot be NULL");
+        return NULL;
+    }
+    if (!name) {
+        error_setg(errp, "Bitmap name cannot be NULL");
+        return NULL;
+    }
+
+    bs = bdrv_lookup_bs(node_ref, node_ref, errp);
+    if (!bs) {
+        error_setg(errp, "Node reference '%s' not found", node_ref);

No need to throw the (hopefully) perfectly fine Error code returned by
bdrv_lookup_bs() away.


I just wanted an error message consistent with the parameter name, in
this case. i.e., We couldn't find the "Node reference" instead of
"device" or "node name." Just trying to distinguish the fact that this
is an arbitrary reference in the error message.

I can still remove it, but I am curious to see what Markus thinks of
the names I have chosen before I monkey with the errors too much more.

Very well then, but you should clean up the error returned by
bdrv_lookup_bs() (call error_free()).

Feel free to keep my R-b whichever decision you'll make (as long as the
error returned by bdrv_lookup_bs() is not leaked).

Max


Or a new helper designed specifically for single argument "BB -or- BDS" lookups, too, that uses the name of the parameter that we eventually decide upon.

I'll clean up the leak for now.

Thanks!


+        return NULL;
+    }
+
+    /* If caller provided a BDS*, provide the result of that lookup,
too. */
+    if (pbs) {
+        *pbs = bs;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);

I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error
message looks like "foo because bar" to me. But it's up to you, dirty
bitmap names most likely don't look like error reasons so nobody should
be able to confuse it.


No, I agree.

So with the error_setg() in the fail path for bdrv_lookup_bs() removed:

Reviewed-by: Max Reitz <address@hidden>




--
—js



reply via email to

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