[Qemu-devel] [PATCH] Rewrite block filter snapshot authorization mechanism

[Qemu-devel] [PATCH] Rewrite block filter snapshot authorization mechanism

[Benoît Canet]

Rewrite the snapshot authorization mechanism for block filter as suggested by
Paolo.

run testsuite

and

Tested BlockBackend snapshots
Tested quorum snapshot
Tested that snapshot below or above the active qcow2 file are not authorized

Benoît Canet (1):
  block: Rewrite the snapshot authorization mechanism for block filters.

 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(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH] block: Rewrite the snapshot authorization mechanism for block filters.

[Benoît Canet]

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(-)

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.
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] block: Rewrite the snapshot authorization mechanism for block filters.

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] block: Rewrite the snapshot authorization mechanism for block filters.

[Benoît Canet]

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

Re: [Qemu-devel] [PATCH] block: Rewrite the snapshot authorization mechanism for block filters.

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Rewrite block filter snapshot authorization mechanism

[Benoît Canet]

The Monday 03 Mar 2014 à 19:11:33 (+0100), Benoît Canet wrote :
> Rewrite the snapshot authorization mechanism for block filter as suggested by
> Paolo.
> 
> run testsuite
> 
> and
> 
> Tested BlockBackend snapshots
> Tested quorum snapshot
> Tested that snapshot below or above the active qcow2 file are not authorized
> 
> Benoît Canet (1):
>   block: Rewrite the snapshot authorization mechanism for block filters.
> 
>  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(-)
> 
> -- 
> 1.8.3.2
> 

gentle ping

Re: [Qemu-devel] [PATCH] Rewrite block filter snapshot authorization mechanism

[Stefan Hajnoczi]

On Mon, Mar 03, 2014 at 07:11:33PM +0100, Benoît Canet wrote:
> Rewrite the snapshot authorization mechanism for block filter as suggested by
> Paolo.
> 
> run testsuite
> 
> and
> 
> Tested BlockBackend snapshots
> Tested quorum snapshot
> Tested that snapshot below or above the active qcow2 file are not authorized
> 
> Benoît Canet (1):
>   block: Rewrite the snapshot authorization mechanism for block filters.
> 
>  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(-)

Thanks, applied to my block tree and fixed up commit description to
mention semantic change in blkverify:
https://github.com/stefanha/qemu/commits/block

Stefan