On 3/25/19 6:53 AM, Kevin Wolf wrote: > 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).
>>> @@ -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.
Prior to commit 690c7301, it was "error while reading block status of
sector % "PRId64 " %s"
I can change that while queuing through my NBD tree.
Thanks; applying to my queue for 4.0.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
