[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 1/2] qemu-img: Report bdrv_block_status failures
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH 1/2] qemu-img: Report bdrv_block_status failures |
Date: |
Mon, 25 Mar 2019 09:48:48 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
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