Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2125 bytes --]

On 28.04.20 16:21, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Right now, all users of bdrv_make_empty() call the BlockDriver method
>> directly.  That is not only bad style, it is also wrong, unless the
>> caller has a BdrvChild with a WRITE permission.
>>
>> Introduce bdrv_make_empty() that verifies that it does.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block.h |  1 +
>>  block.c               | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index b05995fe9c..d947fb4080 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>>  void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>>  int bdrv_commit(BlockDriverState *bs);
>> +int bdrv_make_empty(BdrvChild *c, Error **errp);
>>  int bdrv_change_backing_file(BlockDriverState *bs,
>>      const char *backing_file, const char *backing_fmt);
>>  void bdrv_register(BlockDriver *bdrv);
>> diff --git a/block.c b/block.c
>> index 2e3905c99e..b0d5b98617 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>>  
>>      parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>>  }
>> +
>> +int bdrv_make_empty(BdrvChild *c, Error **errp)
>> +{
>> +    BlockDriver *drv = c->bs->drv;
>> +    int ret;
>> +
>> +    assert(c->perm & BLK_PERM_WRITE);
> 
> If I understand correctly, bdrv_make_empty() is called to drop an
> overlay whose content is identical to what it would read from its
> backing file (in particular after a commit operation). This means that
> the caller promises that the visible content doesn't change.
> 
> So should we check BLK_PERM_WRITE_UNCHANGED instead?

Ah, right.  Yes, that would be better.  (Or to check both, whether any
of them has been taken.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]