diff mbox

block: Rewrite the snapshot authorization mechanism for block filters.

Message ID 1393870294-19351-2-git-send-email-benoit.canet@irqsave.net
State New
Headers show

Commit Message

Benoît Canet March 3, 2014, 6:11 p.m. UTC
This patch keep the recursive way of doing things but simplify it by giving
two responsabilities to all block filters implementors.

They will need to do two things:

-Set the is_filter field of their block driver to true.

-Implement the bdrv_recurse_is_first_non_filter method of their block driver like
it is done on the Quorum block driver. (block/quorum.c)

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c                   | 47 +++++++++++++++++++++--------------------------
 block/blkverify.c         | 17 ++++++++++++++++-
 block/quorum.c            |  3 +--
 include/block/block.h     |  9 ---------
 include/block/block_int.h |  8 ++++----
 5 files changed, 42 insertions(+), 42 deletions(-)

Comments

Paolo Bonzini March 3, 2014, 6:35 p.m. UTC | #1
Il 03/03/2014 19:11, Benoît Canet ha scritto:
> diff --git a/block/blkverify.c b/block/blkverify.c
> index b98b08b..e1c3117 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -288,6 +288,20 @@ static BlockDriverAIOCB *blkverify_aio_flush(BlockDriverState *bs,
>      return bdrv_aio_flush(s->test_file, cb, opaque);
>  }
>
> +static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
> +                                                  BlockDriverState *candidate)
> +{
> +    BDRVBlkverifyState *s = bs->opaque;
> +
> +    bool perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> +
> +    if (perm) {
> +        return true;
> +    }
> +
> +    return bdrv_recurse_is_first_non_filter(s->test_file, candidate);

Thanks!  Is this a silent bugfix? :)

It is a behavior change from before, because BS_FILTER_PASS_DOWN only 
tested bs->file.

Paolo
Benoît Canet March 3, 2014, 7:13 p.m. UTC | #2
The Monday 03 Mar 2014 à 19:35:13 (+0100), Paolo Bonzini wrote :
> Il 03/03/2014 19:11, Benoît Canet ha scritto:
> >diff --git a/block/blkverify.c b/block/blkverify.c
> >index b98b08b..e1c3117 100644
> >--- a/block/blkverify.c
> >+++ b/block/blkverify.c
> >@@ -288,6 +288,20 @@ static BlockDriverAIOCB *blkverify_aio_flush(BlockDriverState *bs,
> >     return bdrv_aio_flush(s->test_file, cb, opaque);
> > }
> >
> >+static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
> >+                                                  BlockDriverState *candidate)
> >+{
> >+    BDRVBlkverifyState *s = bs->opaque;
> >+
> >+    bool perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> >+
> >+    if (perm) {
> >+        return true;
> >+    }
> >+
> >+    return bdrv_recurse_is_first_non_filter(s->test_file, candidate);
> 
> Thanks!  Is this a silent bugfix? :)
> 
> It is a behavior change from before, because BS_FILTER_PASS_DOWN
> only tested bs->file.

Hmm I did not even though it was a bug, I merely rewrote the code to adapt to
the new way of doing.

Should I mention the bugfix and repost ?

Best regards

Benoît

> 
> Paolo
Paolo Bonzini March 3, 2014, 9:26 p.m. UTC | #3
Il 03/03/2014 20:13, Benoît Canet ha scritto:
> Hmm I did not even though it was a bug, I merely rewrote the code to adapt to
> the new way of doing.
>
> Should I mention the bugfix and repost ?

Up to Kevin.

Paolo
diff mbox

Patch

diff --git a/block.c b/block.c
index 2fd5482..749835c 100644
--- a/block.c
+++ b/block.c
@@ -5369,43 +5369,37 @@  int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
     return bs->drv->bdrv_amend_options(bs, options);
 }
 
-/* Used to recurse on single child block filters.
- * Single child block filter will store their child in bs->file.
+/* This function will be called by the bdrv_recurse_is_first_non_filter method
+ * of block filter and by bdrv_is_first_non_filter.
+ * It is used to test if the given bs is the candidate or recurse more in the
+ * node graph.
  */
-bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
+bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
                                       BlockDriverState *candidate)
 {
-    if (!bs->drv) {
-        return false;
-    }
-
-    if (!bs->drv->authorizations[BS_IS_A_FILTER]) {
-        if (bs == candidate) {
-            return true;
-        } else {
-            return false;
-        }
-    }
-
-    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
+    /* return false if basic checks fails */
+    if (!bs || !bs->drv) {
         return false;
     }
 
-    if (!bs->file) {
-        return false;
+    /* the code reached a non block filter driver -> check if the bs is
+     * the same as the candidate. It's the recursion termination condition.
+     */
+    if (!bs->drv->is_filter) {
+        return bs == candidate;
     }
+    /* Down this path the driver is a block filter driver */
 
-    return bdrv_recurse_is_first_non_filter(bs->file, candidate);
-}
-
-bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
-                                      BlockDriverState *candidate)
-{
-    if (bs->drv && bs->drv->bdrv_recurse_is_first_non_filter) {
+    /* If the block filter recursion method is defined use it to recurse down
+     * the node graph.
+     */
+    if (bs->drv->bdrv_recurse_is_first_non_filter) {
         return bs->drv->bdrv_recurse_is_first_non_filter(bs, candidate);
     }
 
-    return bdrv_generic_is_first_non_filter(bs, candidate);
+    /* the driver is a block filter but don't allow to recurse -> return false
+     */
+    return false;
 }
 
 /* This function checks if the candidate is the first non filter bs down it's
@@ -5420,6 +5414,7 @@  bool bdrv_is_first_non_filter(BlockDriverState *candidate)
     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         bool perm;
 
+        /* try to recurse in this top level bs */
         perm = bdrv_recurse_is_first_non_filter(bs, candidate);
 
         /* candidate is the first non filter */
diff --git a/block/blkverify.c b/block/blkverify.c
index b98b08b..e1c3117 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -288,6 +288,20 @@  static BlockDriverAIOCB *blkverify_aio_flush(BlockDriverState *bs,
     return bdrv_aio_flush(s->test_file, cb, opaque);
 }
 
+static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
+                                                  BlockDriverState *candidate)
+{
+    BDRVBlkverifyState *s = bs->opaque;
+
+    bool perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+
+    if (perm) {
+        return true;
+    }
+
+    return bdrv_recurse_is_first_non_filter(s->test_file, candidate);
+}
+
 static BlockDriver bdrv_blkverify = {
     .format_name            = "blkverify",
     .protocol_name          = "blkverify",
@@ -302,7 +316,8 @@  static BlockDriver bdrv_blkverify = {
     .bdrv_aio_writev        = blkverify_aio_writev,
     .bdrv_aio_flush         = blkverify_aio_flush,
 
-    .authorizations         = { true, false },
+    .is_filter              = true,
+    .bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
 };
 
 static void bdrv_blkverify_init(void)
diff --git a/block/quorum.c b/block/quorum.c
index 6c28239..356ebc0 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -849,8 +849,6 @@  static BlockDriver bdrv_quorum = {
     .bdrv_file_open     = quorum_open,
     .bdrv_close         = quorum_close,
 
-    .authorizations     = { true, true },
-
     .bdrv_co_flush_to_disk = quorum_co_flush,
 
     .bdrv_getlength     = quorum_getlength,
@@ -859,6 +857,7 @@  static BlockDriver bdrv_quorum = {
     .bdrv_aio_writev    = quorum_aio_writev,
     .bdrv_invalidate_cache = quorum_invalidate_cache,
 
+    .is_filter           = true,
     .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
 };
 
diff --git a/include/block/block.h b/include/block/block.h
index 780f48b..bd34d14 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -286,15 +286,6 @@  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
 
 /* external snapshots */
-
-typedef enum {
-    BS_IS_A_FILTER,
-    BS_FILTER_PASS_DOWN,
-    BS_AUTHORIZATION_COUNT,
-} BsAuthorization;
-
-bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
-                                      BlockDriverState *candidate);
 bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
                                       BlockDriverState *candidate);
 bool bdrv_is_first_non_filter(BlockDriverState *candidate);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0bcf1c9..4fc5ea8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -76,10 +76,10 @@  struct BlockDriver {
     const char *format_name;
     int instance_size;
 
-    /* this table of boolean contains authorizations for the block operations */
-    bool authorizations[BS_AUTHORIZATION_COUNT];
-    /* for snapshots complex block filter like Quorum can implement the
-     * following recursive callback instead of BS_IS_A_FILTER.
+    /* set to true if the BlockDriver is a block filter */
+    bool is_filter;
+    /* for snapshots block filter like Quorum can implement the
+     * following recursive callback.
      * It's purpose is to recurse on the filter children while calling
      * bdrv_recurse_is_first_non_filter on them.
      * For a sample implementation look in the future Quorum block filter.