Re: [Qemu-block] [PATCH 1/2] qemu-img: Report bdrv_block_status failures

[Kevin Wolf]

Am 25.03.2019 um 11:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2019 0:26, Eric Blake wrote:
> > If bdrv_block_status_above() fails, we are aborting the convert
> > process but failing to print an error message.  Broken in commit
> > 690c7301 (v2.4) when rewriting convert's logic.
> > 
> > Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and
> > accidentally violating the protocol by returning more than one extent
> > in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE.  The qemu NBD code
> > should probably handle the server's non-compliance more gracefully
> > than failing with EINVAL, but qemu-img shouldn't be silently
> > squelching any block status failures. It doesn't help that qemu 3.1
> > masks the qemu-img bug with extra noise that the nbd code is dumping
> > to stderr (that noise was cleaned up in d8b4bad8).
> > 
> > Reported-by: Richard W.M. Jones <address@hidden>
> > Signed-off-by: Eric Blake <address@hidden>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> > ---
> >   qemu-img.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 5fac8407428..a67c28e9ae5 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState 
> > *s, int64_t sector_num)
> >                                             count, &count, NULL, NULL);
> >           }
> >           if (ret < 0) {
> > +            error_report("Could not read sector %" PRId64 " metadata: %s",
> > +                         sector_num, strerror(-ret));
> 
> hmm first time I see that is called "metadata", more common pattern is
> just s/ metadata:/:/

I think that would be misleading, because I would understand that actual
data I/O (bdrv_co_preadv) has failed.

Actually, before sending my R-b, for a moment I thought of suggesting to
make the message something like "Could not get block status for sector
..." to make it less likely that someone just reads the start and
misinterprets the message. But then I decided that the colour of this
specific bike shed isn't important enough to me to request a respin.

Kevin