[Qemu-devel] [PATCH 00/10] block: Op blocker fixes

[Qemu-devel] [PATCH 00/10] block: Op blocker fixes

[Kevin Wolf]

This series fixes a few problems introduced recently with the op blocker
series. It includes mainly fix for cases where qemu would abort()
instead of doing proper error handling previously. These changes also
happen to result in more complete and correct permission checking.

Kevin Wolf (10):
  commit: Fix error handling
  mirror: Fix permission problem with 'replaces'
  mirror: Fix permissions for removing mirror_top_bs
  mirror: Fix error path for dirty bitmap creation
  block: Fix blockdev-snapshot error handling
  block: Factor out should_update_child()
  block: Factor out bdrv_replace_child_noperm()
  block: Ignore multiple children in bdrv_check_update_perm()
  block: Handle permission errors in change_parent_backing_link()
  block: Fix error handling in bdrv_replace_in_backing_chain()

 block.c                   | 182 ++++++++++++++++++++++++++++++----------------
 block/commit.c            |   2 +-
 block/mirror.c            |  35 +++++----
 blockdev.c                |   6 +-
 include/block/block.h     |   4 +-
 include/block/block_int.h |   6 +-
 6 files changed, 152 insertions(+), 83 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/10] commit: Fix error handling

[Kevin Wolf]

Apparently some kind of mismerge happened in commit 8dfba279, which
broke the error handling without any real reason by removing the
assignment of the return value to ret in a blk_insert_bs() call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 22a0a4d..e57c1cf 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -364,7 +364,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 
     /* Required permissions are already taken with block_job_add_bdrv() */
     s->top = blk_new(0, BLK_PERM_ALL);
-    blk_insert_bs(s->top, top, errp);
+    ret = blk_insert_bs(s->top, top, errp);
     if (ret < 0) {
         goto fail;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/10] commit: Fix error handling

[Philippe Mathieu-Daudé]

On 03/06/2017 01:21 PM, Kevin Wolf wrote:
> Apparently some kind of mismerge happened in commit 8dfba279, which
> broke the error handling without any real reason by removing the
> assignment of the return value to ret in a blk_insert_bs() call.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 22a0a4d..e57c1cf 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -364,7 +364,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>
>      /* Required permissions are already taken with block_job_add_bdrv() */
>      s->top = blk_new(0, BLK_PERM_ALL);
> -    blk_insert_bs(s->top, top, errp);
> +    ret = blk_insert_bs(s->top, top, errp);
>      if (ret < 0) {
>          goto fail;
>      }
>

[Qemu-devel] [PATCH 02/10] mirror: Fix permission problem with 'replaces'

[Kevin Wolf]

The 'replaces' option of drive-mirror can be used to mirror a Quorum
node to a new image and then let the target image replace one of the
Quorum children. In order for this graph modification to succeed, the
mirror job needs to lift its restrictions on the target node first
before actually replacing the child.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 57f26c3..c9185b3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -509,6 +509,13 @@ static void mirror_exit(BlockJob *job, void *opaque)
      * block_job_completed(). */
     bdrv_ref(src);
     bdrv_ref(mirror_top_bs);
+    bdrv_ref(target_bs);
+
+    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+     * inserting target_bs at s->to_replace, where we might not be able to get
+     * these permissions. */
+    blk_unref(s->target);
+    s->target = NULL;
 
     /* We don't access the source any more. Dropping any WRITE/RESIZE is
      * required before it could become a backing file of target_bs. */
@@ -555,8 +562,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
-    blk_unref(s->target);
-    s->target = NULL;
+    bdrv_unref(target_bs);
 
     /* Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/10] mirror: Fix permission problem with 'replaces'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> The 'replaces' option of drive-mirror can be used to mirror a Quorum
> node to a new image and then let the target image replace one of the
> Quorum children. In order for this graph modification to succeed, the
> mirror job needs to lift its restrictions on the target node first
> before actually replacing the child.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 03/10] mirror: Fix permissions for removing mirror_top_bs

[Kevin Wolf]

mirror_top_bs takes write permissions on its backing file, which can
make it impossible to attach that backing file node to another parent.
However, this is exactly what needs to be done in order to remove
mirror_top_bs from the backing chain. So give up the write permission
first.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9185b3..001b5f0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -566,8 +566,10 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
     /* Remove the mirror filter driver from the graph. Before this, get rid of
      * the blockers on the intermediate nodes so that the resulting state is
-     * valid. */
+     * valid. Also give up permissions on mirror_top_bs->backing, which might
+     * block the removal. */
     block_job_remove_all_bdrv(job);
+    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
     bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1234,6 +1236,7 @@ fail:
         block_job_unref(&s->common);
     }
 
+    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
     bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/10] mirror: Fix permissions for removing mirror_top_bs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> mirror_top_bs takes write permissions on its backing file, which can
> make it impossible to attach that backing file node to another parent.
> However, this is exactly what needs to be done in order to remove
> mirror_top_bs from the backing chain. So give up the write permission
> first.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 04/10] mirror: Fix error path for dirty bitmap creation

[Kevin Wolf]

mirror_top_bs must be removed from the graph again when creating the
dirty bitmap fails.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 001b5f0..f24dc51 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1197,10 +1197,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
-        g_free(s->replaces);
-        blk_unref(s->target);
-        block_job_unref(&s->common);
-        return;
+        goto fail;
     }
 
     /* Required permissions are already taken with blk_new() */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/10] mirror: Fix error path for dirty bitmap creation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> mirror_top_bs must be removed from the graph again when creating the
> dirty bitmap fails.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 001b5f0..f24dc51 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1197,10 +1197,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>  
>      s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>      if (!s->dirty_bitmap) {
> -        g_free(s->replaces);
> -        blk_unref(s->target);
> -        block_job_unref(&s->common);
> -        return;
> +        goto fail;
>      }
>  
>      /* Required permissions are already taken with blk_new() */
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 05/10] block: Fix blockdev-snapshot error handling

[Kevin Wolf]

For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
node reference at first and only checks later whether it already has a
backing file. Between those places, other errors can occur.

Therefore checking in external_snapshot_abort() whether state->new_bs
has a backing file is not sufficient to tell whether bdrv_append() was
already completed or not. Trying to undo the bdrv_append() when it
wasn't even executed is wrong.

Introduce a new boolean flag in the state to fix this.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 8eb4e84..af67ce4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1614,6 +1614,7 @@ typedef struct ExternalSnapshotState {
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
     AioContext *aio_context;
+    bool overlay_appended;
 } ExternalSnapshotState;
 
 static void external_snapshot_prepare(BlkActionState *common,
@@ -1780,6 +1781,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         error_propagate(errp, local_err);
         return;
     }
+    state->overlay_appended = true;
 }
 
 static void external_snapshot_commit(BlkActionState *common)
@@ -1803,7 +1805,7 @@ static void external_snapshot_abort(BlkActionState *common)
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
-        if (state->new_bs->backing) {
+        if (state->overlay_appended) {
             bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
         }
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/10] block: Fix blockdev-snapshot error handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
> node reference at first and only checks later whether it already has a
> backing file. Between those places, other errors can occur.
> 
> Therefore checking in external_snapshot_abort() whether state->new_bs
> has a backing file is not sufficient to tell whether bdrv_append() was
> already completed or not. Trying to undo the bdrv_append() when it
> wasn't even executed is wrong.
> 
> Introduce a new boolean flag in the state to fix this.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

By the way, how are you finding all these spots? Is it existing qemu-io
tests that are failing? And if so, would mentioning which test exposed
the problem being fixed be worth adding in the commit messages?  If not,
are there some qemu-io tests to be added?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 05/10] block: Fix blockdev-snapshot error handling

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

Am 06.03.2017 um 21:23 hat Eric Blake geschrieben:
> On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> > For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
> > node reference at first and only checks later whether it already has a
> > backing file. Between those places, other errors can occur.
> > 
> > Therefore checking in external_snapshot_abort() whether state->new_bs
> > has a backing file is not sufficient to tell whether bdrv_append() was
> > already completed or not. Trying to undo the bdrv_append() when it
> > wasn't even executed is wrong.
> > 
> > Introduce a new boolean flag in the state to fix this.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> By the way, how are you finding all these spots? Is it existing qemu-io
> tests that are failing? And if so, would mentioning which test exposed
> the problem being fixed be worth adding in the commit messages?  If not,
> are there some qemu-io tests to be added?

Most of the fixes here are based on qemu-iotests failures that only
appeared after fixing the error path in change_parent_backing_link(),
which is now later in this series. So there is no commit at which
qemu-iotests cases are actually failing, even though they helped me spot
some problems.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH 06/10] block: Factor out should_update_child()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index f293ccb..6dc02b8 100644
--- a/block.c
+++ b/block.c
@@ -2886,28 +2886,40 @@ void bdrv_close_all(void)
     assert(QTAILQ_EMPTY(&all_bdrv_states));
 }
 
+static bool should_update_child(BdrvChild *c, BlockDriverState *to)
+{
+    BdrvChild *to_c;
+
+    if (c->role->stay_at_node) {
+        return false;
+    }
+
+    if (c->role == &child_backing) {
+        /* If @from is a backing file of @to, ignore the child to avoid
+         * creating a loop. We only want to change the pointer of other
+         * parents. */
+        QLIST_FOREACH(to_c, &to->children, next) {
+            if (to_c == c) {
+                break;
+            }
+        }
+        if (to_c) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void change_parent_backing_link(BlockDriverState *from,
                                        BlockDriverState *to)
 {
-    BdrvChild *c, *next, *to_c;
+    BdrvChild *c, *next;
 
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
-        if (c->role->stay_at_node) {
+        if (!should_update_child(c, to)) {
             continue;
         }
-        if (c->role == &child_backing) {
-            /* If @from is a backing file of @to, ignore the child to avoid
-             * creating a loop. We only want to change the pointer of other
-             * parents. */
-            QLIST_FOREACH(to_c, &to->children, next) {
-                if (to_c == c) {
-                    break;
-                }
-            }
-            if (to_c) {
-                continue;
-            }
-        }
 
         bdrv_ref(to);
         /* FIXME Are we sure that bdrv_replace_child() can't run into
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/10] block: Factor out should_update_child()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 06/10] block: Factor out should_update_child()

[Philippe Mathieu-Daudé]

On Mon, Mar 6, 2017 at 5:35 PM, Eric Blake <eblake@redhat.com> wrote:
> On 03/06/2017 10:21 AM, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c | 42 +++++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

[Qemu-devel] [PATCH 07/10] block: Factor out bdrv_replace_child_noperm()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 6dc02b8..d4570c8 100644
--- a/block.c
+++ b/block.c
@@ -1713,11 +1713,10 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
     *nshared = shared;
 }
 
-static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
-                               bool check_new_perm)
+static void bdrv_replace_child_noperm(BdrvChild *child,
+                                      BlockDriverState *new_bs)
 {
     BlockDriverState *old_bs = child->bs;
-    uint64_t perm, shared_perm;
 
     if (old_bs) {
         if (old_bs->quiesce_counter && child->role->drained_end) {
@@ -1727,7 +1726,29 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
             child->role->detach(child);
         }
         QLIST_REMOVE(child, next_parent);
+    }
+
+    child->bs = new_bs;
+
+    if (new_bs) {
+        QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+        if (new_bs->quiesce_counter && child->role->drained_begin) {
+            child->role->drained_begin(child);
+        }
+
+        if (child->role->attach) {
+            child->role->attach(child);
+        }
+    }
+}
 
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
+                               bool check_new_perm)
+{
+    BlockDriverState *old_bs = child->bs;
+    uint64_t perm, shared_perm;
+
+    if (old_bs) {
         /* Update permissions for old node. This is guaranteed to succeed
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
@@ -1736,23 +1757,14 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
-    child->bs = new_bs;
+    bdrv_replace_child_noperm(child, new_bs);
 
     if (new_bs) {
-        QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
-        if (new_bs->quiesce_counter && child->role->drained_begin) {
-            child->role->drained_begin(child);
-        }
-
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         if (check_new_perm) {
             bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);
         }
         bdrv_set_perm(new_bs, perm, shared_perm);
-
-        if (child->role->attach) {
-            child->role->attach(child);
-        }
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/10] block: Factor out bdrv_replace_child_noperm()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 08/10] block: Ignore multiple children in bdrv_check_update_perm()

[Kevin Wolf]

change_parent_backing_link() will need to update multiple BdrvChild
objects at once. Checking permissions reference by reference doesn't
work because permissions need to be consistent only with all parents
moved to the new child.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 35 ++++++++++++++++++++++-------------
 include/block/block_int.h |  2 +-
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index d4570c8..a7b09d3 100644
--- a/block.c
+++ b/block.c
@@ -1398,7 +1398,8 @@ static int bdrv_fill_options(QDict **options, const char *filename,
  * or bdrv_abort_perm_update().
  */
 static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
-                           uint64_t cumulative_shared_perms, Error **errp)
+                           uint64_t cumulative_shared_perms,
+                           GSList *ignore_children, Error **errp)
 {
     BlockDriver *drv = bs->drv;
     BdrvChild *c;
@@ -1434,7 +1435,8 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
         drv->bdrv_child_perm(bs, c, c->role,
                              cumulative_perms, cumulative_shared_perms,
                              &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, cur_perm, cur_shared, errp);
+        ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
+                                    errp);
         if (ret < 0) {
             return ret;
         }
@@ -1554,15 +1556,15 @@ static char *bdrv_perm_names(uint64_t perm)
 
 /*
  * Checks whether a new reference to @bs can be added if the new user requires
- * @new_used_perm/@new_shared_perm as its permissions. If @ignore_child is set,
- * this old reference is ignored in the calculations; this allows checking
- * permission updates for an existing reference.
+ * @new_used_perm/@new_shared_perm as its permissions. If @ignore_children is
+ * set, the BdrvChild objects in this list are ignored in the calculations;
+ * this allows checking permission updates for an existing reference.
  *
  * Needs to be followed by a call to either bdrv_set_perm() or
  * bdrv_abort_perm_update(). */
 static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
-                                  BdrvChild *ignore_child, Error **errp)
+                                  GSList *ignore_children, Error **errp)
 {
     BdrvChild *c;
     uint64_t cumulative_perms = new_used_perm;
@@ -1572,7 +1574,7 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
     assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
 
     QLIST_FOREACH(c, &bs->parents, next_parent) {
-        if (c == ignore_child) {
+        if (g_slist_find(ignore_children, c)) {
             continue;
         }
 
@@ -1602,15 +1604,22 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
         cumulative_shared_perms &= c->shared_perm;
     }
 
-    return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms, errp);
+    return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms,
+                           ignore_children, errp);
 }
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
 int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          Error **errp)
+                          GSList *ignore_children, Error **errp)
 {
-    return bdrv_check_update_perm(c->bs, perm, shared, c, errp);
+    int ret;
+
+    ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c);
+    ret = bdrv_check_update_perm(c->bs, perm, shared, ignore_children, errp);
+    g_slist_free(ignore_children);
+
+    return ret;
 }
 
 void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
@@ -1635,7 +1644,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 {
     int ret;
 
-    ret = bdrv_child_check_perm(c, perm, shared, errp);
+    ret = bdrv_child_check_perm(c, perm, shared, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -1753,7 +1762,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
          * because we're just taking a parent away, so we're loosening
          * restrictions. */
         bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-        bdrv_check_perm(old_bs, perm, shared_perm, &error_abort);
+        bdrv_check_perm(old_bs, perm, shared_perm, NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
@@ -1762,7 +1771,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
     if (new_bs) {
         bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
         if (check_new_perm) {
-            bdrv_check_perm(new_bs, perm, shared_perm, &error_abort);
+            bdrv_check_perm(new_bs, perm, shared_perm, NULL, &error_abort);
         }
         bdrv_set_perm(new_bs, perm, shared_perm);
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a57c0bf..fc83f7f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -890,7 +890,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
 void bdrv_root_unref_child(BdrvChild *child);
 
 int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          Error **errp);
+                          GSList *ignore_children, Error **errp);
 void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 void bdrv_child_abort_perm_update(BdrvChild *c);
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/10] block: Ignore multiple children in bdrv_check_update_perm()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> change_parent_backing_link() will need to update multiple BdrvChild
> objects at once. Checking permissions reference by reference doesn't
> work because permissions need to be consistent only with all parents
> moved to the new child.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 35 ++++++++++++++++++++++-------------
>  include/block/block_int.h |  2 +-
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 


>  static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
>                                    uint64_t new_shared_perm,
> -                                  BdrvChild *ignore_child, Error **errp)
> +                                  GSList *ignore_children, Error **errp)
>  {
>      BdrvChild *c;
>      uint64_t cumulative_perms = new_used_perm;
> @@ -1572,7 +1574,7 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
>      assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
>  
>      QLIST_FOREACH(c, &bs->parents, next_parent) {
> -        if (c == ignore_child) {
> +        if (g_slist_find(ignore_children, c)) {

Quadratic complexity (searching one list for each element of another
list). Hopefully the combination of lists isn't too long in practice (it
might be a potential problem if someone creates 200 snapshots then tries
to commit them down to 1 image in a single operation, where
ignore_children would then be a long list of intermediate children being
iterated on multiple times, but I don't know if bs->parents can easily
be made as long).  We could always create some sort of hash table to
reduce complexity, as a followup patch, if it turns out to be a problem
in practice, but for now I'm okay with it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 09/10] block: Handle permission errors in change_parent_backing_link()

[Kevin Wolf]

Instead of just trying to change parents by parent over to reference @to
instead of @from, and abort()ing whenever the permissions don't allow
this, do proper permission checking beforehand and pass any error to the
callers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a7b09d3..a310132 100644
--- a/block.c
+++ b/block.c
@@ -2933,21 +2933,53 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
-                                       BlockDriverState *to)
+                                       BlockDriverState *to, Error **errp)
 {
     BdrvChild *c, *next;
+    GSList *list = NULL, *p;
+    uint64_t old_perm, old_shared;
+    uint64_t perm = 0, shared = BLK_PERM_ALL;
+    int ret;
+
+    /* Make sure that @from doesn't go away until we have successfully attached
+     * all of its parents to @to. */
+    bdrv_ref(from);
 
+    /* Put all parents into @list and calculate their cumulative permissions */
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         if (!should_update_child(c, to)) {
             continue;
         }
+        list = g_slist_prepend(list, c);
+        perm |= c->perm;
+        shared &= c->shared_perm;
+    }
+
+    /* Check whether the required permissions can be granted on @to, ignoring
+     * all BdrvChild in @list so that they can't block themselves. */
+    ret = bdrv_check_update_perm(to, perm, shared, list, errp);
+    if (ret < 0) {
+        bdrv_abort_perm_update(to);
+        goto out;
+    }
+
+    /* Now actually perform the change. We performed the permission check for
+     * all elements of @list at once, so set the permissions all at once at the
+     * very end. */
+    for (p = list; p != NULL; p = p->next) {
+        c = p->data;
 
         bdrv_ref(to);
-        /* FIXME Are we sure that bdrv_replace_child() can't run into
-         * &error_abort because of permissions? */
-        bdrv_replace_child(c, to, true);
+        bdrv_replace_child_noperm(c, to);
         bdrv_unref(from);
     }
+
+    bdrv_get_cumulative_perm(to, &old_perm, &old_shared);
+    bdrv_set_perm(to, old_perm | perm, old_shared | shared);
+
+out:
+    g_slist_free(list);
+    bdrv_unref(from);
 }
 
 /*
@@ -2980,7 +3012,12 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
         goto out;
     }
 
-    change_parent_backing_link(bs_top, bs_new);
+    change_parent_backing_link(bs_top, bs_new, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
+        goto out;
+    }
 
     /* bs_new is now referenced by its new parents, we don't need the
      * additional reference any more. */
@@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
 
     bdrv_ref(old);
 
-    change_parent_backing_link(old, new);
+    /* FIXME Proper error handling */
+    change_parent_backing_link(old, new, &error_abort);
 
     bdrv_unref(old);
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/10] block: Handle permission errors in change_parent_backing_link()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> Instead of just trying to change parents by parent over to reference @to
> instead of @from, and abort()ing whenever the permissions don't allow
> this, do proper permission checking beforehand and pass any error to the
> callers.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 

> +    /* Now actually perform the change. We performed the permission check for
> +     * all elements of @list at once, so set the permissions all at once at the
> +     * very end. */
> +    for (p = list; p != NULL; p = p->next) {
> +        c = p->data;
>  
>          bdrv_ref(to);
> -        /* FIXME Are we sure that bdrv_replace_child() can't run into
> -         * &error_abort because of permissions? */
> -        bdrv_replace_child(c, to, true);
> +        bdrv_replace_child_noperm(c, to);
>          bdrv_unref(from);

Looks awesome to get rid of a FIXME...

> @@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
>  
>      bdrv_ref(old);
>  
> -    change_parent_backing_link(old, new);
> +    /* FIXME Proper error handling */
> +    change_parent_backing_link(old, new, &error_abort);

...until this shows we are merely trimming it down to one less instance,
but are still fighting the overall battle.  But we'll get there in the
end :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 10/10] block: Fix error handling in bdrv_replace_in_backing_chain()

[Kevin Wolf]

When adding an Error parameter, bdrv_replace_in_backing_chain() would
become nothing more than a wrapper around change_parent_backing_link().
So make the latter public, renamed as bdrv_replace_node(), and remove
bdrv_replace_in_backing_chain().

Most of the callers just remove a node from the graph that they just
inserted, so they can use &error_abort, but completion of a mirror job
with 'replaces' set can actually fail.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 25 ++++++-------------------
 block/mirror.c            | 15 +++++++++------
 blockdev.c                |  2 +-
 include/block/block.h     |  4 ++--
 include/block/block_int.h |  4 ++--
 5 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index a310132..dd9ded8 100644
--- a/block.c
+++ b/block.c
@@ -2932,8 +2932,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
     return true;
 }
 
-static void change_parent_backing_link(BlockDriverState *from,
-                                       BlockDriverState *to, Error **errp)
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                       Error **errp)
 {
     BdrvChild *c, *next;
     GSList *list = NULL, *p;
@@ -2941,6 +2941,9 @@ static void change_parent_backing_link(BlockDriverState *from,
     uint64_t perm = 0, shared = BLK_PERM_ALL;
     int ret;
 
+    assert(!atomic_read(&from->in_flight));
+    assert(!atomic_read(&to->in_flight));
+
     /* Make sure that @from doesn't go away until we have successfully attached
      * all of its parents to @to. */
     bdrv_ref(from);
@@ -3003,16 +3006,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
 {
     Error *local_err = NULL;
 
-    assert(!atomic_read(&bs_top->in_flight));
-    assert(!atomic_read(&bs_new->in_flight));
-
     bdrv_set_backing_hd(bs_new, bs_top, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }
 
-    change_parent_backing_link(bs_top, bs_new, &local_err);
+    bdrv_replace_node(bs_top, bs_new, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         bdrv_set_backing_hd(bs_new, NULL, &error_abort);
@@ -3025,19 +3025,6 @@ out:
     bdrv_unref(bs_new);
 }
 
-void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
-{
-    assert(!bdrv_requests_pending(old));
-    assert(!bdrv_requests_pending(new));
-
-    bdrv_ref(old);
-
-    /* FIXME Proper error handling */
-    change_parent_backing_link(old, new, &error_abort);
-
-    bdrv_unref(old);
-}
-
 static void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->job);
diff --git a/block/mirror.c b/block/mirror.c
index f24dc51..a5d30ee 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -550,8 +550,12 @@ static void mirror_exit(BlockJob *job, void *opaque)
         /* The mirror job has no requests in flight any more, but we need to
          * drain potential other users of the BDS before changing the graph. */
         bdrv_drained_begin(target_bs);
-        bdrv_replace_in_backing_chain(to_replace, target_bs);
+        bdrv_replace_node(to_replace, target_bs, &local_err);
         bdrv_drained_end(target_bs);
+        if (local_err) {
+            error_report_err(local_err);
+            data->ret = -EPERM;
+        }
     }
     if (s->to_replace) {
         bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
@@ -570,12 +574,11 @@ static void mirror_exit(BlockJob *job, void *opaque)
      * block the removal. */
     block_job_remove_all_bdrv(job);
     bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
-     * bdrv_replace_in_backing_chain() calls), so switch the BB back so the
-     * cleanup does the right thing. We don't need any permissions any more
-     * now. */
+     * bdrv_replace_node() calls), so switch the BB back so the cleanup does
+     * the right thing. We don't need any permissions any more now. */
     blk_remove_bs(job->blk);
     blk_set_perm(job->blk, 0, BLK_PERM_ALL, &error_abort);
     blk_insert_bs(job->blk, mirror_top_bs, &error_abort);
@@ -1234,7 +1237,7 @@ fail:
     }
 
     bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-    bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index af67ce4..f1f49bd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1806,7 +1806,7 @@ static void external_snapshot_abort(BlkActionState *common)
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
         if (state->overlay_appended) {
-            bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
+            bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
         }
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index c7c4a3a..5149260 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -238,8 +238,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 BlockDriverState *bdrv_new(void);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                  Error **errp);
-void bdrv_replace_in_backing_chain(BlockDriverState *old,
-                                   BlockDriverState *new);
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                       Error **errp);
 
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fc83f7f..6c699ac 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -441,8 +441,8 @@ typedef struct BdrvAioNotifier {
 } BdrvAioNotifier;
 
 struct BdrvChildRole {
-    /* If true, bdrv_replace_in_backing_chain() doesn't change the node this
-     * BdrvChild points to. */
+    /* If true, bdrv_replace_node() doesn't change the node this BdrvChild
+     * points to. */
     bool stay_at_node;
 
     void (*inherit_options)(int *child_flags, QDict *child_options,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/10] block: Fix error handling in bdrv_replace_in_backing_chain()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> When adding an Error parameter, bdrv_replace_in_backing_chain() would
> become nothing more than a wrapper around change_parent_backing_link().
> So make the latter public, renamed as bdrv_replace_node(), and remove
> bdrv_replace_in_backing_chain().
> 
> Most of the callers just remove a node from the graph that they just
> inserted, so they can use &error_abort, but completion of a mirror job
> with 'replaces' set can actually fail.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 25 ++++++-------------------
>  block/mirror.c            | 15 +++++++++------
>  blockdev.c                |  2 +-
>  include/block/block.h     |  4 ++--
>  include/block/block_int.h |  4 ++--
>  5 files changed, 20 insertions(+), 30 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 00/10] block: Op blocker fixes

[Fam Zheng]

On Mon, 03/06 17:21, Kevin Wolf wrote:
> This series fixes a few problems introduced recently with the op blocker
> series. It includes mainly fix for cases where qemu would abort()
> instead of doing proper error handling previously. These changes also
> happen to result in more complete and correct permission checking.
> 
> Kevin Wolf (10):
>   commit: Fix error handling
>   mirror: Fix permission problem with 'replaces'
>   mirror: Fix permissions for removing mirror_top_bs
>   mirror: Fix error path for dirty bitmap creation
>   block: Fix blockdev-snapshot error handling
>   block: Factor out should_update_child()
>   block: Factor out bdrv_replace_child_noperm()
>   block: Ignore multiple children in bdrv_check_update_perm()
>   block: Handle permission errors in change_parent_backing_link()
>   block: Fix error handling in bdrv_replace_in_backing_chain()
> 
>  block.c                   | 182 ++++++++++++++++++++++++++++++----------------
>  block/commit.c            |   2 +-
>  block/mirror.c            |  35 +++++----
>  blockdev.c                |   6 +-
>  include/block/block.h     |   4 +-
>  include/block/block_int.h |   6 +-
>  6 files changed, 152 insertions(+), 83 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 00/10] block: Op blocker fixes

[Kevin Wolf]

Am 06.03.2017 um 17:21 hat Kevin Wolf geschrieben:
> This series fixes a few problems introduced recently with the op blocker
> series. It includes mainly fix for cases where qemu would abort()
> instead of doing proper error handling previously. These changes also
> happen to result in more complete and correct permission checking.

Applied to the block branch.

Kevin