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

[Max Reitz]

On 15.02.2017 18:29, Kevin Wolf wrote:
> 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.

Yes, it is. I'm just dumping random thoughts. :-)

> 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. :-)

TIL commit resizes the base in that case.

Max

signature.asc
Description: OpenPGP digital signature