[Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Kevin Wolf]

bdrv_reopen() can switch nodes between read-only and read-write modes.
This has implications for the required permissions on their child nodes.
For example, a qcow2 node requests write permissions on bs->file only if
it is writable itself.

This means that during bdrv_reopen(), the permissions need to be
recalculated in order to prevent failures where the bs->file
permissions don't match its actual read-only state (e.g. bs->file is a
read-write node, but the permission still enforces read-only access).

Kevin Wolf (6):
  qemu-io: Reset qemuio_blk permissions before each command
  block: Add reopen_queue to bdrv_child_perm()
  block: Add reopen queue to bdrv_check_perm()
  block: Base permissions on rw state after reopen
  block: reopen: Queue children after their parents
  block: Fix permissions after bdrv_reopen()

 include/block/block.h      |   2 +-
 include/block/block_int.h  |   7 ++
 block.c                    | 191 +++++++++++++++++++++++++++++++++------------
 block/commit.c             |   1 +
 block/mirror.c             |   1 +
 block/replication.c        |   1 +
 block/vvfat.c              |   1 +
 qemu-io.c                  |  13 +++
 tests/qemu-iotests/187.out |   2 +-
 9 files changed, 169 insertions(+), 50 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Kevin Wolf]

qemu-io provides a 'reopen' command that allows switching from writable
to read-only access. We need to make sure that we don't try to keep
write permissions to a BlockBackend that becomes read-only, otherwise
things are going to fail.

command() already makes sure to request any additional permissions that
a qemu-io command requires, so just resetting the permissions to values
that are safe for read-only images is enough to fix this.

As a side effect, this makes the output of qemu-iotests case 187 more
consistent.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c                  | 13 +++++++++++++
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index 265445ad89..9e031f0f8e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -366,6 +366,12 @@ static void command_loop(void)
     char *input;
 
     for (i = 0; !done && i < ncmdline; i++) {
+        /* Make sure that we start each command with clean permissions and only
+         * add whatever the specific cmdinfo_t describes */
+        if (qemuio_blk) {
+            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
+                         &error_abort);
+        }
         done = qemuio_command(qemuio_blk, cmdline[i]);
     }
     if (cmdline) {
@@ -391,6 +397,13 @@ static void command_loop(void)
         if (input == NULL) {
             break;
         }
+
+        /* Make sure that we start each command with clean permissions and only
+         * add whatever the specific cmdinfo_t describes */
+        if (qemuio_blk) {
+            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
+                         &error_abort);
+        }
         done = qemuio_command(qemuio_blk, input);
         g_free(input);
 
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
index 68fb944cd5..30b987f71f 100644
--- a/tests/qemu-iotests/187.out
+++ b/tests/qemu-iotests/187.out
@@ -12,7 +12,7 @@ Start from read-write
 
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-write failed: Operation not permitted
+Block node is read-only
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> qemu-io provides a 'reopen' command that allows switching from writable
> to read-only access. We need to make sure that we don't try to keep
> write permissions to a BlockBackend that becomes read-only, otherwise
> things are going to fail.
> 
> command() already makes sure to request any additional permissions that
> a qemu-io command requires, so just resetting the permissions to values
> that are safe for read-only images is enough to fix this.
> 
> As a side effect, this makes the output of qemu-iotests case 187 more
> consistent.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io.c                  | 13 +++++++++++++
>  tests/qemu-iotests/187.out |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Fam Zheng]

On Fri, 09/15 12:10, Kevin Wolf wrote:
> qemu-io provides a 'reopen' command that allows switching from writable
> to read-only access. We need to make sure that we don't try to keep
> write permissions to a BlockBackend that becomes read-only, otherwise
> things are going to fail.
> 
> command() already makes sure to request any additional permissions that
> a qemu-io command requires, so just resetting the permissions to values
> that are safe for read-only images is enough to fix this.
> 
> As a side effect, this makes the output of qemu-iotests case 187 more
> consistent.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io.c                  | 13 +++++++++++++
>  tests/qemu-iotests/187.out |  2 +-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 265445ad89..9e031f0f8e 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -366,6 +366,12 @@ static void command_loop(void)
>      char *input;
>  
>      for (i = 0; !done && i < ncmdline; i++) {
> +        /* Make sure that we start each command with clean permissions and only
> +         * add whatever the specific cmdinfo_t describes */
> +        if (qemuio_blk) {
> +            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
> +                         &error_abort);
> +        }
>          done = qemuio_command(qemuio_blk, cmdline[i]);
>      }
>      if (cmdline) {
> @@ -391,6 +397,13 @@ static void command_loop(void)
>          if (input == NULL) {
>              break;
>          }
> +
> +        /* Make sure that we start each command with clean permissions and only
> +         * add whatever the specific cmdinfo_t describes */
> +        if (qemuio_blk) {
> +            blk_set_perm(qemuio_blk, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL,
> +                         &error_abort);
> +        }
>          done = qemuio_command(qemuio_blk, input);
>          g_free(input);
>  
> diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
> index 68fb944cd5..30b987f71f 100644
> --- a/tests/qemu-iotests/187.out
> +++ b/tests/qemu-iotests/187.out
> @@ -12,7 +12,7 @@ Start from read-write
>  
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -write failed: Operation not permitted
> +Block node is read-only
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> -- 
> 2.13.5
> 

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

Re: [Qemu-devel] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Kevin Wolf]

Am 15.09.2017 um 12:10 hat Kevin Wolf geschrieben:
> qemu-io provides a 'reopen' command that allows switching from writable
> to read-only access. We need to make sure that we don't try to keep
> write permissions to a BlockBackend that becomes read-only, otherwise
> things are going to fail.
> 
> command() already makes sure to request any additional permissions that
> a qemu-io command requires, so just resetting the permissions to values
> that are safe for read-only images is enough to fix this.
> 
> As a side effect, this makes the output of qemu-iotests case 187 more
> consistent.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

This seems to break qemu-iotests 077 and 081 for raw. I'll look into
it tomorrow.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 1/6] qemu-io: Reset qemuio_blk permissions before each command

[Kevin Wolf]

Am 21.09.2017 um 15:53 hat Kevin Wolf geschrieben:
> Am 15.09.2017 um 12:10 hat Kevin Wolf geschrieben:
> > qemu-io provides a 'reopen' command that allows switching from writable
> > to read-only access. We need to make sure that we don't try to keep
> > write permissions to a BlockBackend that becomes read-only, otherwise
> > things are going to fail.
> > 
> > command() already makes sure to request any additional permissions that
> > a qemu-io command requires, so just resetting the permissions to values
> > that are safe for read-only images is enough to fix this.
> > 
> > As a side effect, this makes the output of qemu-iotests case 187 more
> > consistent.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> This seems to break qemu-iotests 077 and 081 for raw. I'll look into
> it tomorrow.

The problem seems to be related to 'aio_write': We already reset the
permissions while the request is still in flight and might still start
new internal requests that aren't allowed any more. We would have to
drain blk around resetting the permissions, but this would obviously
defeat the purpose of the aio_* commands.

I tried creating a separate temporary BB, but it doesn't seem to be that
straightforward (still crashes during 'aio_flush'). It would also
provide the wrong semantics, HMP 'qemu-io' commands are supposed to be
executed on the exact BlockBackend that was given.

So unless someone has an idea how to salvage this patch, I'm afraid I'll
just have to drop it. The original state isn't really correct either, but
it errs on the side of having more permissions than necessary, so
failure is less likely.

Kevin

[Qemu-devel] [PATCH v2 1/6] qemu-io: Drop write permissions before read-only reopen

[Kevin Wolf]

qemu-io provides a 'reopen' command that allows switching from writable
to read-only access. We need to make sure that we don't try to keep
write permissions to a BlockBackend that becomes read-only, otherwise
things are going to fail.

This requires a bdrv_drain() call because otherwise in-flight AIO
write requests could issue new internal requests while the permission
has already gone away, which would cause assertion failures. Draining
the queue doesn't break AIO requests in any new way, bdrv_reopen() would
drain it anyway only a few lines later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c             | 12 ++++++++++++
 tests/qemu-iotests/187.out |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2811a89099..3727fb43f3 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2010,6 +2010,18 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }
 
+    if (!(flags & BDRV_O_RDWR)) {
+        uint64_t orig_perm, orig_shared_perm;
+
+        bdrv_drain(bs);
+
+        blk_get_perm(blk, &orig_perm, &orig_shared_perm);
+        blk_set_perm(blk,
+                     orig_perm & ~(BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED),
+                     orig_shared_perm,
+                     &error_abort);
+    }
+
     qopts = qemu_opts_find(&reopen_opts, NULL);
     opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
     qemu_opts_reset(&reopen_opts);
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
index 68fb944cd5..30b987f71f 100644
--- a/tests/qemu-iotests/187.out
+++ b/tests/qemu-iotests/187.out
@@ -12,7 +12,7 @@ Start from read-write
 
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-write failed: Operation not permitted
+Block node is read-only
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.13.5

Re: [Qemu-devel] [PATCH v2 1/6] qemu-io: Drop write permissions before read-only reopen

[Fam Zheng]

On Fri, 09/22 14:55, Kevin Wolf wrote:
> qemu-io provides a 'reopen' command that allows switching from writable
> to read-only access. We need to make sure that we don't try to keep
> write permissions to a BlockBackend that becomes read-only, otherwise
> things are going to fail.
> 
> This requires a bdrv_drain() call because otherwise in-flight AIO
> write requests could issue new internal requests while the permission
> has already gone away, which would cause assertion failures. Draining
> the queue doesn't break AIO requests in any new way, bdrv_reopen() would
> drain it anyway only a few lines later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c             | 12 ++++++++++++
>  tests/qemu-iotests/187.out |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 2811a89099..3727fb43f3 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -2010,6 +2010,18 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
>          return 0;
>      }
>  
> +    if (!(flags & BDRV_O_RDWR)) {
> +        uint64_t orig_perm, orig_shared_perm;
> +
> +        bdrv_drain(bs);
> +
> +        blk_get_perm(blk, &orig_perm, &orig_shared_perm);
> +        blk_set_perm(blk,
> +                     orig_perm & ~(BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED),
> +                     orig_shared_perm,
> +                     &error_abort);
> +    }
> +
>      qopts = qemu_opts_find(&reopen_opts, NULL);
>      opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
>      qemu_opts_reset(&reopen_opts);
> diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
> index 68fb944cd5..30b987f71f 100644
> --- a/tests/qemu-iotests/187.out
> +++ b/tests/qemu-iotests/187.out
> @@ -12,7 +12,7 @@ Start from read-write
>  
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -write failed: Operation not permitted
> +Block node is read-only
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> -- 
> 2.13.5
> 

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

[Qemu-devel] [PATCH 2/6] block: Add reopen_queue to bdrv_child_perm()

[Kevin Wolf]

In the context of bdrv_reopen(), we'll have to look at the state of the
graph as it will be after the reopen. This interface addition is in
preparation for the change.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h |  7 +++++++
 block.c                   | 19 ++++++++++++-------
 block/commit.c            |  1 +
 block/mirror.c            |  1 +
 block/replication.c       |  1 +
 block/vvfat.c             |  1 +
 6 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..99abe2ce74 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -411,9 +411,14 @@ struct BlockDriver {
      *
      * If @c is NULL, return the permissions for attaching a new child for the
      * given @role.
+     *
+     * If @reopen_queue is non-NULL, don't return the currently needed
+     * permissions, but those that will be needed after applying the
+     * @reopen_queue.
      */
      void (*bdrv_child_perm)(BlockDriverState *bs, BdrvChild *c,
                              const BdrvChildRole *role,
+                             BlockReopenQueue *reopen_queue,
                              uint64_t parent_perm, uint64_t parent_shared,
                              uint64_t *nperm, uint64_t *nshared);
 
@@ -983,6 +988,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
  * all children */
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
+                               BlockReopenQueue *reopen_queue,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared);
 
@@ -992,6 +998,7 @@ void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
  * CONSISTENT_READ and doesn't share WRITE. */
 void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
+                               BlockReopenQueue *reopen_queue,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared);
 
diff --git a/block.c b/block.c
index 6dd47e414e..c7724c85e3 100644
--- a/block.c
+++ b/block.c
@@ -1537,16 +1537,17 @@ static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
 static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
-                            BdrvChild *c,
-                            const BdrvChildRole *role,
+                            BdrvChild *c, const BdrvChildRole *role,
+                            BlockReopenQueue *reopen_queue,
                             uint64_t parent_perm, uint64_t parent_shared,
                             uint64_t *nperm, uint64_t *nshared)
 {
     if (bs->drv && bs->drv->bdrv_child_perm) {
-        bs->drv->bdrv_child_perm(bs, c, role,
+        bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
                                  parent_perm, parent_shared,
                                  nperm, nshared);
     }
+    /* TODO Take force_share from reopen_queue */
     if (child_bs && child_bs->force_share) {
         *nshared = BLK_PERM_ALL;
     }
@@ -1596,7 +1597,7 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
-        bdrv_child_perm(bs, c->bs, c, c->role,
+        bdrv_child_perm(bs, c->bs, c, c->role, NULL,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
         ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
@@ -1658,7 +1659,7 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     /* Update all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
-        bdrv_child_perm(bs, c->bs, c, c->role,
+        bdrv_child_perm(bs, c->bs, c, c->role, NULL,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
         bdrv_child_set_perm(c, cur_perm, cur_shared);
@@ -1827,6 +1828,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
+                               BlockReopenQueue *reopen_queue,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared)
 {
@@ -1844,6 +1846,7 @@ void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
 
 void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                const BdrvChildRole *role,
+                               BlockReopenQueue *reopen_queue,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared)
 {
@@ -1853,9 +1856,11 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
     if (!backing) {
         /* Apart from the modifications below, the same permissions are
          * forwarded and left alone as for filters */
-        bdrv_filter_default_perms(bs, c, role, perm, shared, &perm, &shared);
+        bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared,
+                                  &perm, &shared);
 
         /* Format drivers may touch metadata even if the guest doesn't write */
+        /* TODO Take flags from reopen_queue */
         if (bdrv_is_writable(bs)) {
             perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
         }
@@ -1999,7 +2004,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 
     assert(parent_bs->drv);
     assert(bdrv_get_aio_context(parent_bs) == bdrv_get_aio_context(child_bs));
-    bdrv_child_perm(parent_bs, child_bs, NULL, child_role,
+    bdrv_child_perm(parent_bs, child_bs, NULL, child_role, NULL,
                     perm, shared_perm, &perm, &shared_perm);
 
     child = bdrv_root_attach_child(child_bs, child_name, child_role,
diff --git a/block/commit.c b/block/commit.c
index 898d91f653..8f0e83578a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -257,6 +257,7 @@ static void bdrv_commit_top_close(BlockDriverState *bs)
 
 static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
                                        const BdrvChildRole *role,
+                                       BlockReopenQueue *reopen_queue,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
diff --git a/block/mirror.c b/block/mirror.c
index 6531652d73..6f5cb9f26c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1084,6 +1084,7 @@ static void bdrv_mirror_top_close(BlockDriverState *bs)
 
 static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c,
                                        const BdrvChildRole *role,
+                                       BlockReopenQueue *reopen_queue,
                                        uint64_t perm, uint64_t shared,
                                        uint64_t *nperm, uint64_t *nshared)
 {
diff --git a/block/replication.c b/block/replication.c
index bf4462c8e7..3a4e6822e4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -157,6 +157,7 @@ static void replication_close(BlockDriverState *bs)
 
 static void replication_child_perm(BlockDriverState *bs, BdrvChild *c,
                                    const BdrvChildRole *role,
+                                   BlockReopenQueue *reopen_queue,
                                    uint64_t perm, uint64_t shared,
                                    uint64_t *nperm, uint64_t *nshared)
 {
diff --git a/block/vvfat.c b/block/vvfat.c
index c54fa94651..ee894bbe98 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3210,6 +3210,7 @@ err:
 
 static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,
                              const BdrvChildRole *role,
+                             BlockReopenQueue *reopen_queue,
                              uint64_t perm, uint64_t shared,
                              uint64_t *nperm, uint64_t *nshared)
 {
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/6] block: Add reopen_queue to bdrv_child_perm()

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> In the context of bdrv_reopen(), we'll have to look at the state of the
> graph as it will be after the reopen. This interface addition is in
> preparation for the change.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int.h |  7 +++++++
>  block.c                   | 19 ++++++++++++-------
>  block/commit.c            |  1 +
>  block/mirror.c            |  1 +
>  block/replication.c       |  1 +
>  block/vvfat.c             |  1 +
>  6 files changed, 23 insertions(+), 7 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 3/6] block: Add reopen queue to bdrv_check_perm()

[Kevin Wolf]

In the context of bdrv_reopen(), we'll have to look at the state of the
graph as it will be after the reopen. This interface addition is in
preparation for the change.

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

diff --git a/block.c b/block.c
index c7724c85e3..0b499fda4c 100644
--- a/block.c
+++ b/block.c
@@ -1531,7 +1531,8 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     return 0;
 }
 
-static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
+static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
+                                 uint64_t perm, uint64_t shared,
                                  GSList *ignore_children, Error **errp);
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
@@ -1562,7 +1563,8 @@ static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
  * A call to this function must always be followed by a call to bdrv_set_perm()
  * or bdrv_abort_perm_update().
  */
-static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
+static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
+                           uint64_t cumulative_perms,
                            uint64_t cumulative_shared_perms,
                            GSList *ignore_children, Error **errp)
 {
@@ -1597,11 +1599,11 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms,
     /* Check all children */
     QLIST_FOREACH(c, &bs->children, next) {
         uint64_t cur_perm, cur_shared;
-        bdrv_child_perm(bs, c->bs, c, c->role, NULL,
+        bdrv_child_perm(bs, c->bs, c, c->role, q,
                         cumulative_perms, cumulative_shared_perms,
                         &cur_perm, &cur_shared);
-        ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children,
-                                    errp);
+        ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
+                                    ignore_children, errp);
         if (ret < 0) {
             return ret;
         }
@@ -1727,7 +1729,8 @@ char *bdrv_perm_names(uint64_t perm)
  *
  * 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,
+static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
+                                  uint64_t new_used_perm,
                                   uint64_t new_shared_perm,
                                   GSList *ignore_children, Error **errp)
 {
@@ -1769,19 +1772,20 @@ 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,
+    return bdrv_check_perm(bs, q, 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(). */
-static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
+static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
+                                 uint64_t perm, uint64_t shared,
                                  GSList *ignore_children, Error **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);
+    ret = bdrv_check_update_perm(c->bs, q, perm, shared, ignore_children, errp);
     g_slist_free(ignore_children);
 
     return ret;
@@ -1809,7 +1813,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, NULL, errp);
+    ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, errp);
     if (ret < 0) {
         bdrv_child_abort_perm_update(c);
         return ret;
@@ -1950,7 +1954,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, NULL, &error_abort);
+        bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, &error_abort);
         bdrv_set_perm(old_bs, perm, shared_perm);
     }
 
@@ -1969,7 +1973,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     BdrvChild *child;
     int ret;
 
-    ret = bdrv_check_update_perm(child_bs, perm, shared_perm, NULL, errp);
+    ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(child_bs);
         return NULL;
@@ -3184,7 +3188,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
 
     /* 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);
+    ret = bdrv_check_update_perm(to, NULL, perm, shared, list, errp);
     if (ret < 0) {
         bdrv_abort_perm_update(to);
         goto out;
@@ -4054,7 +4058,7 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 
     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-    ret = bdrv_check_perm(bs, perm, shared_perm, NULL, &local_err);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
     if (ret < 0) {
         bs->open_flags |= BDRV_O_INACTIVE;
         error_propagate(errp, local_err);
@@ -4121,7 +4125,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
 
         /* Update permissions, they may differ for inactive nodes */
         bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-        bdrv_check_perm(bs, perm, shared_perm, NULL, &error_abort);
+        bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
         bdrv_set_perm(bs, perm, shared_perm);
     }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH 3/6] block: Add reopen queue to bdrv_check_perm()

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> In the context of bdrv_reopen(), we'll have to look at the state of the
> graph as it will be after the reopen. This interface addition is in
> preparation for the change.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 4/6] block: Base permissions on rw state after reopen

[Kevin Wolf]

When new permissions are calculated during bdrv_reopen(), they need to
be based on the state of the graph as it will be after the reopen has
completed, not on the current state of the involved nodes.

This patch makes bdrv_is_writable() optionally accept a BlockReopenQueue
from which the new flags are taken. This is then used for determining
the new bs->file permissions of format drivers as soon as we add the
code to actually pass a non-NULL reopen queue to the .bdrv_child_perm
callbacks.

While moving bdrv_is_writable(), make it static. It isn't used outside
block.c.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  1 -
 block.c               | 52 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 2ad18775af..082eb2cd9c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -435,7 +435,6 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
-bool bdrv_is_writable(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
diff --git a/block.c b/block.c
index 0b499fda4c..ed8d51dd42 100644
--- a/block.c
+++ b/block.c
@@ -239,12 +239,6 @@ bool bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
-/* Returns whether the image file can be written to right now */
-bool bdrv_is_writable(BlockDriverState *bs)
-{
-    return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
-}
-
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp)
 {
@@ -1537,6 +1531,41 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
 static void bdrv_child_abort_perm_update(BdrvChild *c);
 static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
+typedef struct BlockReopenQueueEntry {
+     bool prepared;
+     BDRVReopenState state;
+     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+} BlockReopenQueueEntry;
+
+/*
+ * Return the flags that @bs will have after the reopens in @q have
+ * successfully completed. If @q is NULL (or @bs is not contained in @q),
+ * return the current flags.
+ */
+static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)
+{
+    BlockReopenQueueEntry *entry;
+
+    if (q != NULL) {
+        QSIMPLEQ_FOREACH(entry, q, entry) {
+            if (entry->state.bs == bs) {
+                return entry->state.flags;
+            }
+        }
+    }
+
+    return bs->open_flags;
+}
+
+/* Returns whether the image file can be written to after the reopen queue @q
+ * has been successfully applied, or right now if @q is NULL. */
+static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)
+{
+    int flags = bdrv_reopen_get_flags(q, bs);
+
+    return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR;
+}
+
 static void bdrv_child_perm(BlockDriverState *bs, BlockDriverState *child_bs,
                             BdrvChild *c, const BdrvChildRole *role,
                             BlockReopenQueue *reopen_queue,
@@ -1574,7 +1603,7 @@ static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
 
     /* Write permissions never work with read-only images */
     if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
-        !bdrv_is_writable(bs))
+        !bdrv_is_writable(bs, q))
     {
         error_setg(errp, "Block node is read-only");
         return -EPERM;
@@ -1864,8 +1893,7 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                   &perm, &shared);
 
         /* Format drivers may touch metadata even if the guest doesn't write */
-        /* TODO Take flags from reopen_queue */
-        if (bdrv_is_writable(bs)) {
+        if (bdrv_is_writable(bs, reopen_queue)) {
             perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
         }
 
@@ -2642,12 +2670,6 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
                              NULL, errp);
 }
 
-typedef struct BlockReopenQueueEntry {
-     bool prepared;
-     BDRVReopenState state;
-     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
-} BlockReopenQueueEntry;
-
 /*
  * Adds a BlockDriverState to a simple queue for an atomic, transactional
  * reopen of multiple devices.
-- 
2.13.5

Re: [Qemu-devel] [PATCH 4/6] block: Base permissions on rw state after reopen

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> When new permissions are calculated during bdrv_reopen(), they need to
> be based on the state of the graph as it will be after the reopen has
> completed, not on the current state of the involved nodes.
> 
> This patch makes bdrv_is_writable() optionally accept a BlockReopenQueue
> from which the new flags are taken. This is then used for determining
> the new bs->file permissions of format drivers as soon as we add the
> code to actually pass a non-NULL reopen queue to the .bdrv_child_perm
> callbacks.
> 
> While moving bdrv_is_writable(), make it static. It isn't used outside
> block.c.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  1 -
>  block.c               | 52 ++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 

> + * Return the flags that @bs will have after the reopens in @q have
> + * successfully completed. If @q is NULL (or @bs is not contained in @q),
> + * return the current flags.
> + */
> +static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)

> +/* Returns whether the image file can be written to after the reopen queue @q
> + * has been successfully applied, or right now if @q is NULL. */
> +static bool bdrv_is_writable(BlockDriverState *bs, BlockReopenQueue *q)

Is it worth having both functions with arguments in the same order?

> +{
> +    int flags = bdrv_reopen_get_flags(q, bs);
> +

No real semantic impact to leave it as is, but it would avoid the odd
swap of arguments here.  So either way,

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 5/6] block: reopen: Queue children after their parents

[Kevin Wolf]

We will calculate the required new permissions in the prepare stage of a
reopen. Required permissions of children can be influenced by the
changes made to their parents, but parents are independent from their
children. This means that permissions need to be calculated top-down. In
order to achieve this, queue parents before their children rather than
queuing the children first.

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

diff --git a/block.c b/block.c
index ed8d51dd42..204cbb46c7 100644
--- a/block.c
+++ b/block.c
@@ -2768,6 +2768,19 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
+    if (!bs_entry) {
+        bs_entry = g_new0(BlockReopenQueueEntry, 1);
+        QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+    } else {
+        QDECREF(bs_entry->state.options);
+        QDECREF(bs_entry->state.explicit_options);
+    }
+
+    bs_entry->state.bs = bs;
+    bs_entry->state.options = options;
+    bs_entry->state.explicit_options = explicit_options;
+    bs_entry->state.flags = flags;
+
     QLIST_FOREACH(child, &bs->children, next) {
         QDict *new_child_options;
         char *child_key_dot;
@@ -2787,19 +2800,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
                                 child->role, options, flags);
     }
 
-    if (!bs_entry) {
-        bs_entry = g_new0(BlockReopenQueueEntry, 1);
-        QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
-    } else {
-        QDECREF(bs_entry->state.options);
-        QDECREF(bs_entry->state.explicit_options);
-    }
-
-    bs_entry->state.bs = bs;
-    bs_entry->state.options = options;
-    bs_entry->state.explicit_options = explicit_options;
-    bs_entry->state.flags = flags;
-
     return bs_queue;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH 5/6] block: reopen: Queue children after their parents

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> We will calculate the required new permissions in the prepare stage of a
> reopen. Required permissions of children can be influenced by the
> changes made to their parents, but parents are independent from their
> children. This means that permissions need to be calculated top-down. In
> order to achieve this, queue parents before their children rather than
> queuing the children first.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

If we switch between read-only and read-write, the permissions that
image format drivers need on bs->file change, too. Make sure to update
the permissions during bdrv_reopen().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 082eb2cd9c..3c3af462e4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
+    uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
     void *opaque;
diff --git a/block.c b/block.c
index 204cbb46c7..5c65fac672 100644
--- a/block.c
+++ b/block.c
@@ -2781,6 +2781,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     bs_entry->state.explicit_options = explicit_options;
     bs_entry->state.flags = flags;
 
+    /* This needs to be overwritten in bdrv_reopen_prepare() */
+    bs_entry->state.perm = UINT64_MAX;
+    bs_entry->state.shared_perm = 0;
+
     QLIST_FOREACH(child, &bs->children, next) {
         QDict *new_child_options;
         char *child_key_dot;
@@ -2887,6 +2891,52 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
     return ret;
 }
 
+static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
+                                                          BdrvChild *c)
+{
+    BlockReopenQueueEntry *entry;
+
+    QSIMPLEQ_FOREACH(entry, q, entry) {
+        BlockDriverState *bs = entry->state.bs;
+        BdrvChild *child;
+
+        QLIST_FOREACH(child, &bs->children, next) {
+            if (child == c) {
+                return entry;
+            }
+        }
+    }
+
+    return NULL;
+}
+
+static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
+                             uint64_t *perm, uint64_t *shared)
+{
+    BdrvChild *c;
+    BlockReopenQueueEntry *parent;
+    uint64_t cumulative_perms = 0;
+    uint64_t cumulative_shared_perms = BLK_PERM_ALL;
+
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        parent = find_parent_in_reopen_queue(q, c);
+        if (!parent) {
+            cumulative_perms |= c->perm;
+            cumulative_shared_perms &= c->shared_perm;
+        } else {
+            uint64_t nperm, nshared;
+
+            bdrv_child_perm(parent->state.bs, bs, c, c->role, q,
+                            parent->state.perm, parent->state.shared_perm,
+                            &nperm, &nshared);
+
+            cumulative_perms |= nperm;
+            cumulative_shared_perms &= nshared;
+        }
+    }
+    *perm = cumulative_perms;
+    *shared = cumulative_shared_perms;
+}
 
 /*
  * Prepares a BlockDriverState for reopen. All changes are staged in the
@@ -2952,6 +3002,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /* Calculate required permissions after reopening */
+    bdrv_reopen_perm(queue, reopen_state->bs,
+                     &reopen_state->perm, &reopen_state->shared_perm);
 
     ret = bdrv_flush(reopen_state->bs);
     if (ret) {
@@ -3007,6 +3060,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         } while ((entry = qdict_next(reopen_state->options, entry)));
     }
 
+    ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
+                          reopen_state->shared_perm, NULL, errp);
+    if (ret < 0) {
+        goto error;
+    }
+
     ret = 0;
 
 error:
@@ -3047,6 +3106,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
     bdrv_refresh_limits(bs, NULL);
 
+    bdrv_set_perm(reopen_state->bs, reopen_state->perm,
+                  reopen_state->shared_perm);
+
     new_can_write =
         !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
     if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
@@ -3080,6 +3142,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     }
 
     QDECREF(reopen_state->explicit_options);
+
+    bdrv_abort_perm_update(reopen_state->bs);
 }
 
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Eric Blake]

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

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> If we switch between read-only and read-write, the permissions that
> image format drivers need on bs->file change, too. Make sure to update
> the permissions during bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 

> +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
> +                                                          BdrvChild *c)
> +{
> +    BlockReopenQueueEntry *entry;
> +
> +    QSIMPLEQ_FOREACH(entry, q, entry) {
> +        BlockDriverState *bs = entry->state.bs;
> +        BdrvChild *child;
> +
> +        QLIST_FOREACH(child, &bs->children, next) {
> +            if (child == c) {
> +                return entry;

An O(n^2) loop. Is it going to bite us at any point in the future, or
are we generally dealing with a small enough queue size and BDS graph to
not worry about it?

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

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

Am 15.09.2017 um 21:06 hat Eric Blake geschrieben:
> On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> > If we switch between read-only and read-write, the permissions that
> > image format drivers need on bs->file change, too. Make sure to update
> > the permissions during bdrv_reopen().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/block.h |  1 +
> >  block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> > 
> 
> > +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
> > +                                                          BdrvChild *c)
> > +{
> > +    BlockReopenQueueEntry *entry;
> > +
> > +    QSIMPLEQ_FOREACH(entry, q, entry) {
> > +        BlockDriverState *bs = entry->state.bs;
> > +        BdrvChild *child;
> > +
> > +        QLIST_FOREACH(child, &bs->children, next) {
> > +            if (child == c) {
> > +                return entry;
> 
> An O(n^2) loop. Is it going to bite us at any point in the future, or
> are we generally dealing with a small enough queue size and BDS graph to
> not worry about it?

The loops you're quoting aren't O(n^2), they don't loop over the same
thing. This part is O(n) in terms of BdrvChild elements looked at.

The thing that worried me a bit more is the caller:

+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        parent = find_parent_in_reopen_queue(q, c);

This is indeed O(n^2) (again with n = number of BdrvChild elements) in
the pathological worst case of a quorum node where all children point to
the same node.

As soon as all parents of the node are distinct - and I don't see any
reason why they wouldn't in practice - we're back to O(n) because each
BdrvChild belongs to only one parent. Even if we ever introduce a driver
where having the same node as a child in a constant number of different
roles makes sense for a parent (i.e. anything that doesn't involve an
(unbounded) array of children), we would still be O(n) with an additional
small constant factor.

So I think in practice we should be okay.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Fam Zheng]

On Fri, 09/15 12:10, Kevin Wolf wrote:
> If we switch between read-only and read-write, the permissions that
> image format drivers need on bs->file change, too. Make sure to update
> the permissions during bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 082eb2cd9c..3c3af462e4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
>  typedef struct BDRVReopenState {
>      BlockDriverState *bs;
>      int flags;
> +    uint64_t perm, shared_perm;
>      QDict *options;
>      QDict *explicit_options;
>      void *opaque;
> diff --git a/block.c b/block.c
> index 204cbb46c7..5c65fac672 100644
> --- a/block.c
> +++ b/block.c
> @@ -2781,6 +2781,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      bs_entry->state.explicit_options = explicit_options;
>      bs_entry->state.flags = flags;
>  
> +    /* This needs to be overwritten in bdrv_reopen_prepare() */
> +    bs_entry->state.perm = UINT64_MAX;

Probably doesn't matter because as the comment says it will be overwritten soon,
but is BLK_PERM_ALL more appropriate?

> +    bs_entry->state.shared_perm = 0;
> +
>      QLIST_FOREACH(child, &bs->children, next) {
>          QDict *new_child_options;
>          char *child_key_dot;
> @@ -2887,6 +2891,52 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>      return ret;
>  }
>  
> +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
> +                                                          BdrvChild *c)
> +{
> +    BlockReopenQueueEntry *entry;
> +
> +    QSIMPLEQ_FOREACH(entry, q, entry) {
> +        BlockDriverState *bs = entry->state.bs;
> +        BdrvChild *child;
> +
> +        QLIST_FOREACH(child, &bs->children, next) {
> +            if (child == c) {
> +                return entry;
> +            }
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
> +                             uint64_t *perm, uint64_t *shared)
> +{
> +    BdrvChild *c;
> +    BlockReopenQueueEntry *parent;
> +    uint64_t cumulative_perms = 0;
> +    uint64_t cumulative_shared_perms = BLK_PERM_ALL;
> +
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        parent = find_parent_in_reopen_queue(q, c);
> +        if (!parent) {
> +            cumulative_perms |= c->perm;
> +            cumulative_shared_perms &= c->shared_perm;
> +        } else {
> +            uint64_t nperm, nshared;
> +
> +            bdrv_child_perm(parent->state.bs, bs, c, c->role, q,
> +                            parent->state.perm, parent->state.shared_perm,
> +                            &nperm, &nshared);
> +
> +            cumulative_perms |= nperm;
> +            cumulative_shared_perms &= nshared;
> +        }
> +    }
> +    *perm = cumulative_perms;
> +    *shared = cumulative_shared_perms;
> +}
>  
>  /*
>   * Prepares a BlockDriverState for reopen. All changes are staged in the
> @@ -2952,6 +3002,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          goto error;
>      }
>  
> +    /* Calculate required permissions after reopening */
> +    bdrv_reopen_perm(queue, reopen_state->bs,
> +                     &reopen_state->perm, &reopen_state->shared_perm);
>  
>      ret = bdrv_flush(reopen_state->bs);
>      if (ret) {
> @@ -3007,6 +3060,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          } while ((entry = qdict_next(reopen_state->options, entry)));
>      }
>  
> +    ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
> +                          reopen_state->shared_perm, NULL, errp);
> +    if (ret < 0) {
> +        goto error;
> +    }
> +
>      ret = 0;
>  
>  error:
> @@ -3047,6 +3106,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  
>      bdrv_refresh_limits(bs, NULL);
>  
> +    bdrv_set_perm(reopen_state->bs, reopen_state->perm,
> +                  reopen_state->shared_perm);
> +
>      new_can_write =
>          !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
>      if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
> @@ -3080,6 +3142,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>      }
>  
>      QDECREF(reopen_state->explicit_options);
> +
> +    bdrv_abort_perm_update(reopen_state->bs);
>  }
>  
>  
> -- 
> 2.13.5
> 

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

Am 18.09.2017 um 09:37 hat Fam Zheng geschrieben:
> On Fri, 09/15 12:10, Kevin Wolf wrote:
> > If we switch between read-only and read-write, the permissions that
> > image format drivers need on bs->file change, too. Make sure to update
> > the permissions during bdrv_reopen().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/block.h |  1 +
> >  block.c               | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 082eb2cd9c..3c3af462e4 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
> >  typedef struct BDRVReopenState {
> >      BlockDriverState *bs;
> >      int flags;
> > +    uint64_t perm, shared_perm;
> >      QDict *options;
> >      QDict *explicit_options;
> >      void *opaque;
> > diff --git a/block.c b/block.c
> > index 204cbb46c7..5c65fac672 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2781,6 +2781,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> >      bs_entry->state.explicit_options = explicit_options;
> >      bs_entry->state.flags = flags;
> >  
> > +    /* This needs to be overwritten in bdrv_reopen_prepare() */
> > +    bs_entry->state.perm = UINT64_MAX;
> 
> Probably doesn't matter because as the comment says it will be overwritten soon,
> but is BLK_PERM_ALL more appropriate?

I had BLK_PERM_ALL at first, but after debugging some assertion failures
in gdb, I came to the conclusion that UINT64_MAX is easier to identify as
uninitialised than BLK_PERM_ALL, which could be a valid result of the
permission calculation.

Kevin

[Qemu-devel] [PATCH 7/6] qemu-iotests: Test change-backing-file command

[Kevin Wolf]

This involves a temporary read-write reopen if the backing file link in
the middle of a backing file chain should be changed and is therefore a
good test for the latest bdrv_reopen() vs. op blockers fixes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

I actually managed to find a simple case that reproduces the bug that
is fixed in this series, but outside of my commit job improvements that
originally led me to this work.

 tests/qemu-iotests/195     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/195.out | 78 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 171 insertions(+)
 create mode 100755 tests/qemu-iotests/195
 create mode 100644 tests/qemu-iotests/195.out

diff --git a/tests/qemu-iotests/195 b/tests/qemu-iotests/195
new file mode 100755
index 0000000000..05a239cbf5
--- /dev/null
+++ b/tests/qemu-iotests/195
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Test change-backing-file command
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "$TEST_IMG.mid"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@" | _filter_imgfmt
+    $QEMU -nographic -qmp-pretty stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp \
+                          | _filter_qemu_io | _filter_generated_node_ids
+}
+
+size=64M
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
+_make_test_img -b "$TEST_IMG.mid"
+
+echo
+echo "Change backing file of mid (opened read-only)"
+echo
+
+run_qemu -drive if=none,file="$TEST_IMG",backing.node-name=mid <<EOF
+{"execute":"qmp_capabilities"}
+{"execute":"change-backing-file", "arguments":{"device":"none0","image-node-name":"mid","backing-file":"/dev/null"}}
+{"execute":"quit"}
+EOF
+
+TEST_IMG="$TEST_IMG.mid" _img_info
+
+echo
+echo "Change backing file of top (opened writable)"
+echo
+
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
+
+run_qemu -drive if=none,file="$TEST_IMG",node-name=top <<EOF
+{"execute":"qmp_capabilities"}
+{"execute":"change-backing-file", "arguments":{"device":"none0","image-node-name":"top","backing-file":"/dev/null"}}
+{"execute":"quit"}
+EOF
+
+_img_info
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/195.out b/tests/qemu-iotests/195.out
new file mode 100644
index 0000000000..7613575c64
--- /dev/null
+++ b/tests/qemu-iotests/195.out
@@ -0,0 +1,78 @@
+QA output created by 195
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+
+Change backing file of mid (opened read-only)
+
+Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,backing.node-name=mid
+{
+    QMP_VERSION
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "SHUTDOWN",
+    "data": {
+        "guest": false
+    }
+}
+
+image: TEST_DIR/t.IMGFMT.mid
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: /dev/null
+backing file format: IMGFMT
+
+Change backing file of top (opened writable)
+
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Testing: -drive if=none,file=TEST_DIR/t.IMGFMT,node-name=top
+{
+    QMP_VERSION
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "SHUTDOWN",
+    "data": {
+        "guest": false
+    }
+}
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: /dev/null
+backing file format: IMGFMT
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 94e764865a..108339cd03 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -189,3 +189,4 @@
 190 rw auto quick
 192 rw auto quick
 194 rw auto migration quick
+195 rw auto quick
-- 
2.13.5

Re: [Qemu-devel] [PATCH 7/6] qemu-iotests: Test change-backing-file command

[Eric Blake]

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

On 09/15/2017 12:02 PM, Kevin Wolf wrote:
> This involves a temporary read-write reopen if the backing file link in
> the middle of a backing file chain should be changed and is therefore a
> good test for the latest bdrv_reopen() vs. op blockers fixes.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> 
> I actually managed to find a simple case that reproduces the bug that
> is fixed in this series, but outside of my commit job improvements that
> originally led me to this work.
> 
>  tests/qemu-iotests/195     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/195.out | 78 +++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +

The usual collision in test numbering ;)


> +_cleanup()
> +{
> +    _cleanup_test_img
> +    rm -f "$TEST_IMG.mid"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15

Semantic conflict with Jeff's work to add './check -s' to save
intermediate files in a per-test temp directory.

> +
> +echo
> +echo "Change backing file of mid (opened read-only)"
> +echo
> +
> +run_qemu -drive if=none,file="$TEST_IMG",backing.node-name=mid <<EOF
> +{"execute":"qmp_capabilities"}
> +{"execute":"change-backing-file", "arguments":{"device":"none0","image-node-name":"mid","backing-file":"/dev/null"}}

Since the images don't contain any content, I guess this works.  But
another possible change would be rewriting the backing file to alternate
between absolute and relative (or even between 'file' and './file') (so
that it's pointing to the same file at all times, just by different names)

> +{"execute":"quit"}
> +EOF
> +
> +TEST_IMG="$TEST_IMG.mid" _img_info
> +
> +echo
> +echo "Change backing file of top (opened writable)"
> +echo
> +
> +TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
> +
> +run_qemu -drive if=none,file="$TEST_IMG",node-name=top <<EOF
> +{"execute":"qmp_capabilities"}
> +{"execute":"change-backing-file", "arguments":{"device":"none0","image-node-name":"top","backing-file":"/dev/null"}}
> +{"execute":"quit"}

You've quit qemu between runs.  Would we get any further coverage by doing:

change mid
change active
change mid

all within a single run? (That is, make SURE that actions on one part of
the graph don't interfere with later actions elsewhere)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Fam Zheng]

On Fri, 09/15 12:10, Kevin Wolf wrote:
> bdrv_reopen() can switch nodes between read-only and read-write modes.
> This has implications for the required permissions on their child nodes.
> For example, a qcow2 node requests write permissions on bs->file only if
> it is writable itself.
> 
> This means that during bdrv_reopen(), the permissions need to be
> recalculated in order to prevent failures where the bs->file
> permissions don't match its actual read-only state (e.g. bs->file is a
> read-write node, but the permission still enforces read-only access).

Passing reopen_queue around makes the interface and implementations complicated.
I wonder if any of the alternatives make sense:

1) Don't pass reopen_queue as a parameter, just pass the one interesting
BDRVReopenState pointer. So that callees don't need to call
bdrv_reopen_get_flags().

2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract
so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the
new state that would be commited once .bdrv_reopen_commit() is called, or
reverted if .bdrv_reopen_abort().

3) Don't change the prototypes at all, track the reopen progress in block.c
generically, (e.g. ignore conflicts and voilations) and update the permissions
only after bdrv_reopen_commit().

Fam

> 
> Kevin Wolf (6):
>   qemu-io: Reset qemuio_blk permissions before each command
>   block: Add reopen_queue to bdrv_child_perm()
>   block: Add reopen queue to bdrv_check_perm()
>   block: Base permissions on rw state after reopen
>   block: reopen: Queue children after their parents
>   block: Fix permissions after bdrv_reopen()
> 
>  include/block/block.h      |   2 +-
>  include/block/block_int.h  |   7 ++
>  block.c                    | 191 +++++++++++++++++++++++++++++++++------------
>  block/commit.c             |   1 +
>  block/mirror.c             |   1 +
>  block/replication.c        |   1 +
>  block/vvfat.c              |   1 +
>  qemu-io.c                  |  13 +++
>  tests/qemu-iotests/187.out |   2 +-
>  9 files changed, 169 insertions(+), 50 deletions(-)
> 
> -- 
> 2.13.5
> 

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Kevin Wolf]

Am 18.09.2017 um 09:51 hat Fam Zheng geschrieben:
> On Fri, 09/15 12:10, Kevin Wolf wrote:
> > bdrv_reopen() can switch nodes between read-only and read-write modes.
> > This has implications for the required permissions on their child nodes.
> > For example, a qcow2 node requests write permissions on bs->file only if
> > it is writable itself.
> > 
> > This means that during bdrv_reopen(), the permissions need to be
> > recalculated in order to prevent failures where the bs->file
> > permissions don't match its actual read-only state (e.g. bs->file is a
> > read-write node, but the permission still enforces read-only access).
> 
> Passing reopen_queue around makes the interface and implementations
> complicated.

Yes. I don't like it, but I couldn't find any easier way.

> I wonder if any of the alternatives make sense:
> 
> 1) Don't pass reopen_queue as a parameter, just pass the one
> interesting BDRVReopenState pointer. So that callees don't need to
> call bdrv_reopen_get_flags().

There isn't a single interesting BDRVReopenState. The subset that a
single BDS needs to determine its new state is the the BDRVReopenState
of all of its parents. This can be an arbitrary number.

> 2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract
> so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the
> new state that would be commited once .bdrv_reopen_commit() is called, or
> reverted if .bdrv_reopen_abort().

Hm, .bdrv_reopen_prepare already gets the whole queue passed, so I guess
this could technically work. I'm not sure if it is a good idea, though.

Such a change would still make .bdrv_child_perm depend on the reopen
queue, just without actually passing it as a parameter. I like such
hidden data flows even less than adding an explicit one.

It would also mean that each block driver would have to save the queue
in its local bs->opaque structure so that .bdrv_child_perm can access it
later. Alternatively, bdrv_reopen_prepare could already store the new
cumulative parent permissions, but it would still involve two new fields
in bs->opaque for storing something of a rather temporary nature.

Though maybe I'm just missing another way to implement this that you had
in mind?

> 3) Don't change the prototypes at all, track the reopen progress in block.c
> generically, (e.g. ignore conflicts and voilations) and update the permissions
> only after bdrv_reopen_commit().

Both permission updates and reopen are transactional. You need to do
both prepare stages first before you can do commits. If you only start
doing the prepare stage of permissions during the commit stage of
reopen, you break the error cases.

Kevin

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Fam Zheng]

On Mon, 09/18 10:11, Kevin Wolf wrote:
> > 2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract
> > so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the
> > new state that would be commited once .bdrv_reopen_commit() is called, or
> > reverted if .bdrv_reopen_abort().
> 
> Hm, .bdrv_reopen_prepare already gets the whole queue passed, so I guess
> this could technically work. I'm not sure if it is a good idea, though.
> 
> Such a change would still make .bdrv_child_perm depend on the reopen
> queue, just without actually passing it as a parameter. I like such
> hidden data flows even less than adding an explicit one.
> 
> It would also mean that each block driver would have to save the queue
> in its local bs->opaque structure so that .bdrv_child_perm can access it
> later. Alternatively, bdrv_reopen_prepare could already store the new
> cumulative parent permissions, but it would still involve two new fields
> in bs->opaque for storing something of a rather temporary nature.

What about this?

1) drv->bdrv_reopen_prepare() saves the desired new perms in BDRVReopenState.
2) bdrv_reopen_prepare() checks the new perms after drv->bdrv_reopen_prepare()
   returns.
3) bdrv_reopen_commit() updates the bs to new perms.

Fam

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Kevin Wolf]

Am 18.09.2017 um 13:53 hat Fam Zheng geschrieben:
> On Mon, 09/18 10:11, Kevin Wolf wrote:
> > > 2) Don't change the prototypes at all, just change .bdrv_reopen_prepare contract
> > > so that after it returns, .bdrv_child_perm/.bdrv_check_perm should comply to the
> > > new state that would be commited once .bdrv_reopen_commit() is called, or
> > > reverted if .bdrv_reopen_abort().
> > 
> > Hm, .bdrv_reopen_prepare already gets the whole queue passed, so I guess
> > this could technically work. I'm not sure if it is a good idea, though.
> > 
> > Such a change would still make .bdrv_child_perm depend on the reopen
> > queue, just without actually passing it as a parameter. I like such
> > hidden data flows even less than adding an explicit one.
> > 
> > It would also mean that each block driver would have to save the queue
> > in its local bs->opaque structure so that .bdrv_child_perm can access it
> > later. Alternatively, bdrv_reopen_prepare could already store the new
> > cumulative parent permissions, but it would still involve two new fields
> > in bs->opaque for storing something of a rather temporary nature.
> 
> What about this?
> 
> 1) drv->bdrv_reopen_prepare() saves the desired new perms in
>    BDRVReopenState.

But how does it determine the desired new perms? Either you duplicate
the logic of the .bdrv_child_perm implementation into a new set of
functions that does the same thing, but based on the new state; or you
extend the existing function with a BlockReopenQueue parameter. The
latter is basically this series, except with an additional unnecessary
detour through the driver code instead of doing it in common code.

Also note that storing it in BDRVReopenState would have to involve a new
list of an additional data structure because permissions are per
BdrvChild, not per BlockDriverState.

> 2) bdrv_reopen_prepare() checks the new perms after drv->bdrv_reopen_prepare()
>    returns.
> 3) bdrv_reopen_commit() updates the bs to new perms.

2) and 3) are already implemented in this series like you suggest.

Kevin

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Fam Zheng]

On Mon, 09/18 14:11, Kevin Wolf wrote:
> But how does it determine the desired new perms? Either you duplicate
> the logic of the .bdrv_child_perm implementation into a new set of
> functions that does the same thing, but based on the new state; or you
> extend the existing function with a BlockReopenQueue parameter. The
> latter is basically this series, except with an additional unnecessary
> detour through the driver code instead of doing it in common code.
> 
> Also note that storing it in BDRVReopenState would have to involve a new
> list of an additional data structure because permissions are per
> BdrvChild, not per BlockDriverState.

Indeed, this is not going to remove any complexity. :(

Fam

Re: [Qemu-devel] [PATCH 0/6] block: Fix permissions after ro/rw reopen

[Kevin Wolf]

Am 15.09.2017 um 12:10 hat Kevin Wolf geschrieben:
> bdrv_reopen() can switch nodes between read-only and read-write modes.
> This has implications for the required permissions on their child nodes.
> For example, a qcow2 node requests write permissions on bs->file only if
> it is writable itself.
> 
> This means that during bdrv_reopen(), the permissions need to be
> recalculated in order to prevent failures where the bs->file
> permissions don't match its actual read-only state (e.g. bs->file is a
> read-write node, but the permission still enforces read-only access).

Applied to the block branch.

Kevin