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@xxxxxxxxxx>
> ---
> 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
>
