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