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

[Kevin Wolf <kwolf@redhat.com>]

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