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