Re: [Qemu-devel] [RFC PATCH 09/41] block: Default .bdrv_child_perm() for format drivers

[Kevin Wolf]

Am 15.02.2017 um 18:11 hat Max Reitz geschrieben:
> On 13.02.2017 18:22, 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                   | 37 +++++++++++++++++++++++++++++++++++++
> >  include/block/block_int.h |  8 ++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 290768d..8e99bb5 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1459,6 +1459,43 @@ 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 backing file. */
> 
> Are we OK with a resizable backing file, too? I'm not sure, actually.
> Maybe we should just forbid it and hope nobody asks for it.

That's pretty much the same thought that I had, so unless I'm mistaken,
this is exactly what the patch implements.

I can't think of a reason why resizing a backing file would hurt as long
as the parent allows writes to the backing file (though we might have to
audit that no driver caches the backing file size), but then I can't
think of a reason either why anyone would want to do this.

If we ever find out that there is a good reason, we can still change it.

...

Hm, scratch that. I guess a commit block job with a smaller backing file
is enough... Good thing that I neglected to get the RESIZE permission
for s->base there. :-)

Kevin

> Max
> 
> > +        shared &= BLK_PERM_WRITE;
> > +        shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
> > +                  BLK_PERM_WRITE_UNCHANGED;
> > +    }
> > +
> > +    *nperm = perm;
> > +    *nshared = shared;
> > +}
> >  
> >  static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
> >  {
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 2d74f92..46f51a6 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -885,6 +885,14 @@ void bdrv_filter_default_perms(BlockDriverState *bs, 
> > BdrvChild *c,
> >                                 uint64_t perm, uint64_t shared,
> >                                 uint64_t *nperm, uint64_t *nshared);
> >  
> > +/* Default implementation for BlockDriver.bdrv_child_perm() that can be 
> > used by
> > + * (non-raw) image formats: Like above for bs->backing, but for bs->file it
> > + * requires WRITE | RESIZE for read-write images, always requires
> > + * CONSISTENT_READ and doesn't share WRITE. */
> > +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> > +                               const BdrvChildRole *role,
> > +                               uint64_t perm, uint64_t shared,
> > +                               uint64_t *nperm, uint64_t *nshared);
> >  
> >  const char *bdrv_get_parent_name(const BlockDriverState *bs);
> >  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
> > 
> 
>

pgpNqq5h38ukN.pgp
Description: PGP signature