Re: [Qemu-block] [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 <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().

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