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

[Denis V. Lunev]

On 03/29/2017 01:41 PM, Kevin Wolf wrote:
> Am 28.03.2017 um 19:12 hat Denis V. Lunev geschrieben:
>> 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.
> The question is what the contract of bdrv_truncate() is. I think "you
> can only call this when you got resize permissions" is the clearest
> interface, and the current code enforces it.
but in the original patch I have made check exactly over this simple
condition and you says that it was not accurate. If this is wrong, I'll be
rejected later on with EACCESS and will still be on the safe side.
Original patch just avoids the assert().

> Your proposal would change it into "you can only call this when you get
> resize permissions, except when it would fail anyway because the image
> is closed, the driver doesn't support resizing or the node is
> read-only". I wouldn't make such exceptions unless there is a good
> reason to have them, e.g. if it made the life of the callers
> substantially easier. But it don't think it does in this case.
Actually we have had an error condition as the image was really read-only
and all was safe. Right now we have an assert even if the image is
read-only.
This looks the same to me as to raise an assert in write for read-only
image. Is there any difference?


>> 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.
> You mean asserting that we have write permission? We already do that in
> bdrv_aligned_pwritev(), which is called by bdrv_co_pwritev().
I mean that we should disallow image change if it is disallowed
by the contract. Current contract says that we can not change
image content once BDRV_O_INACTIVE is set. Should we
Do we have implicit rule that (child->perm & BLK_PERM_RESIZE)
is set only when INACTIVE is not set?

>> Technically, the requested change is not a problem it looks a bit
>> strange and not consistent to me.
> Hm, okay? Why do you think so? To me, it feels correct that
> bdrv_truncate() can only be called for writable images.
I was unclear here. I have trying to say that "the change requested by you
is not a problem, I'll do that once we will agree". Sorry :(

> It's the current code in the parallels driver that calls it for
> read-only images and implicitly expects an error return on the normal
> code path (without even having a comment about this) that feels somewhat
> unclean to me.
Actually I tend to drop this truncate at all. It was set for a state of
insurance and should not be actually used.

Den