4:33 p.m.
This is v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PN...
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
4:33 p.m.
New subject: [PATCH v2 1/2] qemu: Check empty drives in block I/O throttle API
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(a)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
1:27 p.m.
New subject: [PATCH v2 1/2] qemu: Check empty drives in block I/O throttle API
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(a)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(a)redhat.com>
4:33 p.m.
New subject: [PATCH v2 2/2] qemu: Clarify block I/O throttle JSON parsing errors
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(a)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
1:30 p.m.
New subject: [PATCH v2 2/2] qemu: Clarify block I/O throttle JSON parsing
errors
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(a)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
1
days inactive
9
days old
4 comments
2 participants
participants (2)
-
Fabian Leditzky -
Peter Krempa