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

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
> 

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

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

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

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

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