The VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON event allows @reason to be reported to the user, but currently we only report 'enospc'. Extend the documentation so that we allow the passthrough of the verbatim error, prefixed by 'other: ' in order to prevent collisions and note that users must not attempt to parse them. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 2a4b81f4df..e031b23547 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4790,8 +4790,12 @@ typedef void (*virConnectDomainEventIOErrorCallback)(virConnectPtr conn, * If the I/O error is known to be caused by an ENOSPC condition in * the host (where resizing the disk to be larger will allow the guest * to be resumed as if nothing happened), @reason will be "enospc". - * Otherwise, @reason will be "", although future strings may be added - * if determination of other error types becomes possible. + * + * Otherwise, if the hypervisor reported an error @reason will be the verbatim + * error message from hypervisor prefixed by "other: ". Note that this error + * may not be stable and thus is only really usable for human use. In case + * when the hypervisor doesn't report the error @reason will be an empty string + * "".

On Mon, Jan 27, 2025 at 10:31:28 +0000, Daniel P. Berrangé wrote: > On Fri, Jan 24, 2025 at 05:33:04PM +0100, Peter Krempa wrote: > > The VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON event allows @reason to be > > reported to the user, but currently we only report 'enospc'. > > > > Extend the documentation so that we allow the passthrough of the > > verbatim error, prefixed by 'other: ' in order to prevent collisions and > > note that users must not attempt to parse them. > > > > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > > --- > > include/libvirt/libvirt-domain.h | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h > > index 2a4b81f4df..e031b23547 100644 > > --- a/include/libvirt/libvirt-domain.h > > +++ b/include/libvirt/libvirt-domain.h > > @@ -4790,8 +4790,12 @@ typedef void (*virConnectDomainEventIOErrorCallback)(virConnectPtr conn, > > * If the I/O error is known to be caused by an ENOSPC condition in > > * the host (where resizing the disk to be larger will allow the guest > > * to be resumed as if nothing happened), @reason will be "enospc". > > - * Otherwise, @reason will be "", although future strings may be added > > - * if determination of other error types becomes possible. > > + * > > + * Otherwise, if the hypervisor reported an error @reason will be the verbatim > > + * error message from hypervisor prefixed by "other: ". Note that this error > > + * may not be stable and thus is only really usable for human use. In case > > + * when the hypervisor doesn't report the error @reason will be an empty string > > + * "". > > Hmmm, this makes me feel pretty uncomfortable. > > When set, the 'reason' field has clear long term stable supported semantics, > which applications are permitted to match against. > > Essentially it is an enum field as currently defined. > > This is now being turned into a free-format human readable string > which applications are told not to interpret at all. > > Effectively we've overloaded the field to have two completely > different sets of semantics. > > I don't think we should do this. > > If we want a human readable string, it should be distinct from the > the enum reason we support already. You're right about this effectively being an 'enum' while not being an enum. A bit of history how we ended here because it was at one point in time passtrhough of arbitrary strings. In certain ancient downstreams this was originally a field which was passed through from qemu verbatim and could have also other values than just 'enospc'. The current state was done after qemu formalized the event upstream, where the only stable field we get is 'nospace' which we've mapped to the exact string ('enospc') the downstream version did especially since it was used by oVirt. No other value did get a stable flag in the event so in turn libvirt didn't ever codify any other value, while still keeping the 'string' field.

Now over time this did in fact become an enum with two possible options: "" and "enospc". At this point it felt convenient to use this field as it isn't really an enum, to encode a catch-all/default case for the user-originated string rather than adding yet another libvirt event: 1) Adding event is pain. 2) I'd be a new interface, thus potential users such as Kubevirt would need to use new libvirt instead of using existing interface.

3) It is a string when the CPU looks at it. 4) Adding events is pain!

Pass the generic error message from qemu to the event so that users can possibly use the information. As documented the error is prefixed with "other: " to prevent collisions with already documented values in our API. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_monitor_json.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f417d27c6..6de34b03bb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -695,7 +695,7 @@ qemuMonitorJSONHandleIOError(qemuMonitor *mon, virJSONValue *data) const char *device; const char *nodename; const char *action; - const char *reason = ""; + g_autofree char *reason = NULL; bool nospc = false; int actionID; @@ -713,14 +713,27 @@ qemuMonitorJSONHandleIOError(qemuMonitor *mon, virJSONValue *data) nodename = virJSONValueObjectGetString(data, "node-name"); - if (virJSONValueObjectGetBoolean(data, "nospace", &nospc) == 0 && nospc) - reason = "enospc"; + if (virJSONValueObjectGetBoolean(data, "nospace", &nospc) == 0 && nospc) { + reason = g_strdup("enospc"); + } else { + /* For any unknown error we prefix the qemu-originating error with 'other: ' + * so that applications can use that to filter them from errors we + * define in our API */ + const char *qemureason; + + if ((qemureason = virJSONValueObjectGetString(data, "reason"))) { + reason = g_strdup_printf("other: %s", qemureason); + } + } if ((actionID = qemuMonitorIOErrorActionTypeFromString(action)) < 0) { VIR_WARN("unknown disk io error action '%s'", action); actionID = VIR_DOMAIN_EVENT_IO_ERROR_NONE; } + if (!reason) + reason = g_strdup(""); + qemuMonitorEmitIOError(mon, device, nodename, actionID, reason);