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

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

[Denis V. Lunev]

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 <den@openvz.org>
Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 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;
     }
-- 
2.7.4

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Mon, Mar 27, 2017 at 05:38:08PM +0300, Denis V. Lunev wrote:
> 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 <den@openvz.org>
> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Kevin Wolf]

[ 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 <den@openvz.org>
> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  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

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 <den@openvz.org>
>> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  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

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

[Kevin Wolf]

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 <den@openvz.org>
> >> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
> >> CC: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  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.

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.

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

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

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.

Kevin

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 <den@openvz.org>
>>>> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>  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

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

[Kevin Wolf]

Am 29.03.2017 um 15:53 hat Denis V. Lunev geschrieben:
> 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 <den@openvz.org>
> >>>> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
> >>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>>> ---
> >>>>  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().

No, you checked BDRV_O_RESIZE in bs->open_flags, not BLK_PERM_RESIZE in
child->perm. If you checked for BLK_PERM_RESIZE, that would work (though
I still think that checking for read-only gets closer to the actual
intent).

> > 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?

Hm, no, not really. You're right that we're inconsistent there. Not sure
which one we should change, but you do have a point there.

> >> 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?

Ah, you want to assert cleared BDRV_O_INACTIVE in bdrv_truncate()?
That sounds like a good idea to me.

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

Well, that's the best solution then for this specific case. :-)

Kevin

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

[Denis V. Lunev]

On 03/29/2017 05:11 PM, Kevin Wolf wrote:
> Am 29.03.2017 um 15:53 hat Denis V. Lunev geschrieben:
>> 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 <den@openvz.org>
>>>>>> Reported-by: Edgar Kaziahmedov <edos@virtuozzo.mipt.ru>
>>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>> ---
>>>>>>  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().
> No, you checked BDRV_O_RESIZE in bs->open_flags, not BLK_PERM_RESIZE in
> child->perm. If you checked for BLK_PERM_RESIZE, that would work (though
> I still think that checking for read-only gets closer to the actual
> intent).
OK. That is clear now, I'll send a fixup.
Thank you for the explanation.


>
>>> 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?
> Hm, no, not really. You're right that we're inconsistent there. Not sure
> which one we should change, but you do have a point there.
>
>>>> 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?
> Ah, you want to assert cleared BDRV_O_INACTIVE in bdrv_truncate()?
> That sounds like a good idea to me.
but this is for 2.10. I think it is too late to do that right now.

>>>> 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.
> Well, that's the best solution then for this specific case. :-)
>
> Kevin
;)

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

[Kevin Wolf]

Am 29.03.2017 um 16:18 hat Denis V. Lunev geschrieben:
> On 03/29/2017 05:11 PM, Kevin Wolf wrote:
> > Am 29.03.2017 um 15:53 hat Denis V. Lunev geschrieben:
> >> On 03/29/2017 01:41 PM, Kevin Wolf wrote:
> >>> 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().
> > No, you checked BDRV_O_RESIZE in bs->open_flags, not BLK_PERM_RESIZE in
> > child->perm. If you checked for BLK_PERM_RESIZE, that would work (though
> > I still think that checking for read-only gets closer to the actual
> > intent).
> OK. That is clear now, I'll send a fixup.
> Thank you for the explanation.

Ok, thanks.

> >>>> 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?
> > Ah, you want to assert cleared BDRV_O_INACTIVE in bdrv_truncate()?
> > That sounds like a good idea to me.
> but this is for 2.10. I think it is too late to do that right now.

Yes, definitely for 2.10. If you like, you can already send a patch
anyway, I always have a block-next queue while we're in freeze.

Kevin