[PATCH v2 0/2] qemu: Improve block I/O throttle API errors
This is v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PNHOL... Changes since v1: - Report more accurate errors instead of suppressing the error message when the drive is empty. - Add check for this condition earlier in the API code. Fabian Leditzky (2): qemu: Check empty drives in block I/O throttle API qemu: Clarify block I/O throttle JSON parsing errors src/qemu/qemu_driver.c | 8 ++++++++ src/qemu/qemu_monitor_json.c | 11 +++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) -- 2.48.1
Provide a proper user facing error when attempting to query block I/O throttling settings for an empty drive. Without this patch, a less meaningful internal error produced by qemuMonitorJSONBlockIoThrottleInfo would be propagated to the user. Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_driver.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2eddbd9ae..1dd30ed444 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15368,6 +15368,14 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; + /* qemu won't return block IO throttle settings for an empty cd-rom drive */ + if (virStorageSourceIsEmpty(disk->src)) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk '%1$s' does not currently have a source assigned"), + path); + goto endjob; + } + qemuDomainObjEnterMonitor(vm); rc = qemuMonitorGetBlockIoThrottle(priv->mon, QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName, &reply); qemuDomainObjExitMonitor(vm); -- 2.48.1
On Tue, Jan 21, 2025 at 14:33:25 +0000, Fabian Leditzky via Devel wrote:
Provide a proper user facing error when attempting to query block I/O throttling settings for an empty drive. Without this patch, a less meaningful internal error produced by qemuMonitorJSONBlockIoThrottleInfo would be propagated to the user.
Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_driver.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2eddbd9ae..1dd30ed444 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15368,6 +15368,14 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob;
+ /* qemu won't return block IO throttle settings for an empty cd-rom drive */ + if (virStorageSourceIsEmpty(disk->src)) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk '%1$s' does not currently have a source assigned"), + path); + goto endjob; + } + qemuDomainObjEnterMonitor(vm); rc = qemuMonitorGetBlockIoThrottle(priv->mon, QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName, &reply); qemuDomainObjExitMonitor(vm);
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The original code incorrectly assumes that the 'inserted' entry is not in the right format (wrong JSON value type, not an 'Object') when virJSONValueObjectGetObject(temp_dev, "inserted") returns NULL. However, NULL is also returned when the 'inserted' entry is simply not present. This is the case when querying an empty CD-ROM device. With this patch, both conditions are reported accurately. Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_monitor_json.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f417d27c6..06e7bb07a3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4581,8 +4581,15 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle, found = true; if (!(inserted = virJSONValueObjectGetObject(temp_dev, "inserted"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block_io_throttle inserted entry was not in expected format")); + if (virJSONValueObjectGet(temp_dev, "inserted")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry was not in expected format")); + } else { + /* caller should validate source presence prior, hence this is an internal error */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry is not present")); + } + return -1; } GET_THROTTLE_STATS("bps", total_bytes_sec); -- 2.48.1
On Tue, Jan 21, 2025 at 14:33:30 +0000, Fabian Leditzky via Devel wrote:
The original code incorrectly assumes that the 'inserted' entry is not in the right format (wrong JSON value type, not an 'Object') when virJSONValueObjectGetObject(temp_dev, "inserted") returns NULL. However, NULL is also returned when the 'inserted' entry is simply not present. This is the case when querying an empty CD-ROM device.
With this patch, both conditions are reported accurately.
I don't think it makes sense to have two distinct error messages for almost impossible errors: - first one being a qemu error where it'd return a non-object - second one being a libvirt programming error We can modify the error to simply state that it's missing or not in expected format but adding the extra condition for the above reason IMO doesn't make sense.
Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_monitor_json.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f417d27c6..06e7bb07a3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4581,8 +4581,15 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle,
found = true; if (!(inserted = virJSONValueObjectGetObject(temp_dev, "inserted"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block_io_throttle inserted entry was not in expected format")); + if (virJSONValueObjectGet(temp_dev, "inserted")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry was not in expected format")); + } else { + /* caller should validate source presence prior, hence this is an internal error */ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry is not present")); + } + return -1; } GET_THROTTLE_STATS("bps", total_bytes_sec); -- 2.48.1
participants (2)
-
Fabian Leditzky -
Peter Krempa