[PATCH] qemu monitor: Fix incorrect error message condition
Fixes 'block_io_throttle inserted entry was not in expected format' error message spam. Without this patch, libvirtd writes an error message every time this function runs for a domain which has CD-ROM drives without a media attached. The 'inserted' key is not present when the drive is empty. 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. Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_monitor_json.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f417d27c6..baf934c2af 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4581,8 +4581,16 @@ 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")); + /* + * Ensure that we only print the error message when the 'inserted' key is actually the wrong type/format. + * Some device types (e.g. SATA CD-ROM) will have no 'inserted' key present if no media is present. + * Avoid spamming this error in such cases. + */ + if (virJSONValueObjectGet(temp_dev, "inserted")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry was not in expected format")); + } + return -1; } GET_THROTTLE_STATS("bps", total_bytes_sec); -- 2.47.1
On Thu, Jan 02, 2025 at 15:14:35 +0000, Fabian Leditzky via Devel wrote:
Fixes 'block_io_throttle inserted entry was not in expected format' error message spam.
[1]
Without this patch, libvirtd writes an error message every time this function runs for a domain which has CD-ROM drives without a media attached. The 'inserted' key is not present when the drive is empty.
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 analysis is good ...
Signed-off-by: Fabian Leditzky <fabian@ldsoft.dev> --- src/qemu/qemu_monitor_json.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f417d27c6..baf934c2af 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4581,8 +4581,16 @@ 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")); + /* + * Ensure that we only print the error message when the 'inserted' key is actually the wrong type/format. + * Some device types (e.g. SATA CD-ROM) will have no 'inserted' key present if no media is present. + * Avoid spamming this error in such cases. + */ + if (virJSONValueObjectGet(temp_dev, "inserted")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block_io_throttle inserted entry was not in expected format")); + }
But this change is not. -1 is an error path and all other error paths set error message in this function. Doing this creates an error path without message. You'll either need to return a different error code (or success) if the caller can use the data (even partially) or you need to keep the error. Now looking at the caller we could return a partial reply if the media is empty, but reallistically querying throttling info for an empty drive doesn't make sense. I'd argue that while the error message is not great there's no real reasonable information to return in this case anyways so an error from the API is a reasonable outcome. Another possibility would be to report the configured parameters from the XML instead of values queried from qemu. In case a medium is inserted those should be applied. Regarding log spam [1] ... something on your system is repeatedly querying throttling config of an empty drive which makes no sense. So perhaps fixing that might make more sense? I'm not even going to question the need to query throttling info repeatedly. Another possibility is to introduce an error code for this usage which will be demoted to DEBUG priority similarly how we do it for the error codes returned e.g. when a VM doesn't exist.
+ return -1; } GET_THROTTLE_STATS("bps", total_bytes_sec); -- 2.47.1
Hi Peter Thank you for the thorough review! On Tue, Jan 14th, 2025 at 8:29 PM, Peter Krempa <pkrempa@redhat.com> wrote:
I'd argue that while the error message is not great there's no real reasonable information to return in this case anyways so an error from the API is a reasonable outcome.
I agree. To address this, I will post a new patch set that improves error reporting, making it easier for application developers to understand the issue.
Regarding log spam [1] ... something on your system is repeatedly querying throttling config of an empty drive which makes no sense. So perhaps fixing that might make more sense? I'm not even going to question the need to query throttling info repeatedly.
The application [1] leading to this error message spam on my hypervisor is a prometheus exporter which does not check the disk type or state when querying the API. I will address this in the exporter application. Kind regards, Fabian Leditzky [1] https://github.com/Tinkoff/libvirt-exporter
participants (2)
-
Fabian Leditzky -
Peter Krempa