qemu: Store I/O error messages for disks

[PATCH v2 01/15] qemu: Handle quirks of 'device' field of BLOCK_IO_ERROR event in monitor code

Posted by Peter Krempa 7 months, 1 week ago

BLOCK_IO_ERROR's 'device' field is an empty string in case when it isn't
applicable as it was originally mandatory in the qemu API docs.

Move the logic that convert's empty string back to NULL from
'qemuProcessHandleIOError()' to 'qemuMonitorJSONHandleIOError()'

This also fixes a hypothetical NULL-dereference if qemu would indeed
report an IO error without the 'device' field present.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_monitor_json.c | 9 ++++++++-
 src/qemu/qemu_process.c      | 3 ---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 9f417d27c6..5ca76d2d80 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -708,8 +708,15 @@ qemuMonitorJSONHandleIOError(qemuMonitor *mon, virJSONValue *data)
         action = "ignore";
     }

-    if ((device = virJSONValueObjectGetString(data, "device")) == NULL)
+    if ((device = virJSONValueObjectGetString(data, "device")) == NULL) {
         VIR_WARN("missing device in disk io error event");
+    } else {
+        /* 'device' was documented as mandatory in the qemu event, but later became
+         * optional, in which case an empty string is sent by qemu. Convert it back
+         * to NULL */
+        if (*device == '\0')
+            device = NULL;
+    }

     nodename = virJSONValueObjectGetString(data, "node-name");

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 34a755a49a..edcd8ac3a8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -840,9 +840,6 @@ qemuProcessHandleIOError(qemuMonitor *mon G_GNUC_UNUSED,
     virObjectLock(vm);
     priv = QEMU_DOMAIN_PRIVATE(vm);

-    if (*diskAlias == '\0')
-        diskAlias = NULL;
-
     if (diskAlias)
         disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL);
     else if (nodename)
-- 
2.48.1

Re: [PATCH v2 01/15] qemu: Handle quirks of 'device' field of BLOCK_IO_ERROR event in monitor code

Posted by Daniel P. Berrangé 7 months, 1 week ago

On Tue, Jan 28, 2025 at 05:28:05PM +0100, Peter Krempa wrote:
> BLOCK_IO_ERROR's 'device' field is an empty string in case when it isn't
> applicable as it was originally mandatory in the qemu API docs.
> 
> Move the logic that convert's empty string back to NULL from
> 'qemuProcessHandleIOError()' to 'qemuMonitorJSONHandleIOError()'
> 
> This also fixes a hypothetical NULL-dereference if qemu would indeed
> report an IO error without the 'device' field present.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_monitor_json.c | 9 ++++++++-
>  src/qemu/qemu_process.c      | 3 ---
>  2 files changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 9f417d27c6..5ca76d2d80 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -708,8 +708,15 @@ qemuMonitorJSONHandleIOError(qemuMonitor *mon, virJSONValue *data)
>          action = "ignore";
>      }
> 
> -    if ((device = virJSONValueObjectGetString(data, "device")) == NULL)
> +    if ((device = virJSONValueObjectGetString(data, "device")) == NULL) {
>          VIR_WARN("missing device in disk io error event");
> +    } else {
> +        /* 'device' was documented as mandatory in the qemu event, but later became
> +         * optional, in which case an empty string is sent by qemu. Convert it back
> +         * to NULL */
> +        if (*device == '\0')
> +            device = NULL;

Oh, I'm surprised that QEMU doesn't send a JSON "null" in this case
already.

> +    }
> 
>      nodename = virJSONValueObjectGetString(data, "node-name");
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 34a755a49a..edcd8ac3a8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -840,9 +840,6 @@ qemuProcessHandleIOError(qemuMonitor *mon G_GNUC_UNUSED,
>      virObjectLock(vm);
>      priv = QEMU_DOMAIN_PRIVATE(vm);
> 
> -    if (*diskAlias == '\0')
> -        diskAlias = NULL;
> -
>      if (diskAlias)
>          disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL);
>      else if (nodename)
> -- 
> 2.48.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 01/15] qemu: Handle quirks of 'device' field of BLOCK_IO_ERROR event in monitor code

Posted by Peter Krempa 7 months, 1 week ago

On Thu, Jan 30, 2025 at 11:10:18 +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 28, 2025 at 05:28:05PM +0100, Peter Krempa wrote:
> > BLOCK_IO_ERROR's 'device' field is an empty string in case when it isn't
> > applicable as it was originally mandatory in the qemu API docs.
> > 
> > Move the logic that convert's empty string back to NULL from
> > 'qemuProcessHandleIOError()' to 'qemuMonitorJSONHandleIOError()'
> > 
> > This also fixes a hypothetical NULL-dereference if qemu would indeed
> > report an IO error without the 'device' field present.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_monitor_json.c | 9 ++++++++-
> >  src/qemu/qemu_process.c      | 3 ---
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> > 
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 9f417d27c6..5ca76d2d80 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -708,8 +708,15 @@ qemuMonitorJSONHandleIOError(qemuMonitor *mon, virJSONValue *data)
> >          action = "ignore";
> >      }
> > 
> > -    if ((device = virJSONValueObjectGetString(data, "device")) == NULL)
> > +    if ((device = virJSONValueObjectGetString(data, "device")) == NULL) {
> >          VIR_WARN("missing device in disk io error event");
> > +    } else {
> > +        /* 'device' was documented as mandatory in the qemu event, but later became
> > +         * optional, in which case an empty string is sent by qemu. Convert it back
> > +         * to NULL */
> > +        if (*device == '\0')
> > +            device = NULL;
> 
> Oh, I'm surprised that QEMU doesn't send a JSON "null" in this case
> already.

The idea of sending empty string is to prevent API breakage as the
'device' field was documented to be mandatory and a string when
BLOCK_IO_ERROR was initially added. Switching to JSON 'null' would be
almost equivalent to just skipping the field (as
virJSONValueObjectGetString would return NULL) at least for the above
code in libvirt.