Re: [Qemu-devel] [PATCH 1/1] parallels: wrong call to bdrv_truncate

[Denis V. Lunev]

On 03/28/2017 07:26 PM, Kevin Wolf wrote:
> [ Cc: qemu-block ]
>
> Am 27.03.2017 um 16:38 hat Denis V. Lunev geschrieben:
>> Parallels driver should not call bdrv_truncate if the image was opened
>> in the read-only mode. Without the patch
>>     qemu-img check harddisk.hds
>> asserts with
>>     bdrv_truncate: Assertion `child->perm & BLK_PERM_RESIZE' failed.
>>
>> Parameters used on the write path are not needed if the image is opened
>> in the read-only mode.
>>
>> Signed-off-by: Denis V. Lunev <address@hidden>
>> Reported-by: Edgar Kaziahmedov <address@hidden>
>> CC: Stefan Hajnoczi <address@hidden>
>> ---
>>  block/parallels.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6bf9375..4173b3f 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -687,7 +687,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
>> *options, int flags,
>>      if (local_err != NULL) {
>>          goto fail_options;
>>      }
>> -    if (!bdrv_has_zero_init(bs->file->bs) ||
>> +
>> +    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
>>              bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs)) != 0) {
>>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>>      }
> Relying on BDRV_O_RESIZE in block drivers is wrong. It is set in some
> paths (specifically the users of blk_new_open), but not in others. We
> should probably have filtered out the flag before passing it to the
> drivers.
>
> As a concrete example, if you're using -blockdev, the bdrv_truncate()
> call won't be executed after applying this patch.
>
> I think the correct way would be to check bdrv_is_read_only() instead.
>
> Kevin
hmmm. But why do we have

int bdrv_truncate(BdrvChild *child, int64_t offset)
{
    BlockDriverState *bs = child->bs;
    BlockDriver *drv = bs->drv;
    int ret;

    assert(child->perm & BLK_PERM_RESIZE);

    if (!drv)
        return -ENOMEDIUM;
    if (!drv->bdrv_truncate)
        return -ENOTSUP;
    if (bs->read_only)
        return -EACCES;

    ret = drv->bdrv_truncate(bs, offset);

instead of

int bdrv_truncate(BdrvChild *child, int64_t offset)
{
    BlockDriverState *bs = child->bs;
    BlockDriver *drv = bs->drv;
    int ret;

    if (!drv)
        return -ENOMEDIUM;
    if (!drv->bdrv_truncate)
        return -ENOTSUP;
    if (bs->read_only)
        return -EACCES;

    assert(child->perm & BLK_PERM_RESIZE);
    ret = drv->bdrv_truncate(bs, offset);

technically this will work properly for my case and calling of
bdrv_truncate could be valid.

Another thing, should we add assert like added into bdrv_co_pwritev,
namely
    assert(!(bs->open_flags & BDRV_O_INACTIVE));
in the same place below access check.

Technically, the requested change is not a problem it looks a bit
strange and not consistent to me.

Den