Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers

[Kevin Wolf]

Am 25.02.2017 um 12:57 hat Max Reitz geschrieben:
> On 21.02.2017 15:58, Kevin Wolf wrote:
> > Almost all format drivers have the same characteristics as far as
> > permissions are concerned: They have one or more children for storing
> > their own data and, more importantly, metadata (can be written to and
> > grow even without external write requests, must be protected against
> > other writers and present consistent data) and optionally a backing file
> > (this is just data, so like for a filter, it only depends on what the
> > parent nodes need).
> > 
> > This provides a default implementation that can be shared by most of
> > our format drivers.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  block.c                   | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block_int.h |  8 ++++++++
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 523cbd3..f2e7178 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> > BdrvChild *c,
> >                 (c->shared_perm & DEFAULT_PERM_UNCHANGED);
> >  }
> >  
> > +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> > +                               const BdrvChildRole *role,
> > +                               uint64_t perm, uint64_t shared,
> > +                               uint64_t *nperm, uint64_t *nshared)
> > +{
> > +    bool backing = (role == &child_backing);
> > +    assert(role == &child_backing || role == &child_file);
> > +
> > +    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);
> > +
> > +        /* Format drivers may touch metadata even if the guest doesn't 
> > write */
> > +        if (!bdrv_is_read_only(bs)) {
> > +            perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> > +        }
> > +
> > +        /* bs->file always needs to be consistent because of the metadata. 
> > We
> > +         * can never allow other users to resize or write to it. */
> > +        perm |= BLK_PERM_CONSISTENT_READ;
> > +        shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > +    } else {
> > +        /* We want consistent read from backing files if the parent needs 
> > it.
> > +         * No other operations are performed on backing files. */
> > +        perm &= BLK_PERM_CONSISTENT_READ;
> > +
> > +        /* If the parent can deal with changing data, we're okay with a
> > +         * writable and resizable backing file. */
> > +        if (shared & BLK_PERM_WRITE) {
> > +            shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
> 
> Wouldn't this break CONSISTENT_READ?

WRITE (even for multiple users) and CONSISTENT_READ aren't mutually
exclusive. I was afraid that I didn't define CONSISTENT_READ right, but
it appears that the definition is fine:

 * A user that has the "permission" of consistent reads is guaranteed that
 * their view of the contents of the block device is complete and
 * self-consistent, representing the contents of a disk at a specific
 * point.

Kevin

pgpprB30Zcsaj.pgp
Description: PGP signature