Re: [libvirt PATCH 3/3] nodedev: handle mdevctl errors consistently

Currently, we have two different types of mdevctl errors: 1. the command cannot be executed due to some error 2. the command is executed, but returns an error status These two different failures are handled differently. Scenario 1 calls virReportError() from within nodeDeviceGetMdevctlCommand() and returns an error status (-1). Scenario 2 also returns -1, but does not call virReportError() and instead passes the error message back in the errmsg argument. This means that the caller has to check both whether the return value is negative and whether the errmsg parameter is non-NULL before deciding whether to report the error or not. The situation is further complicated by the fact that there are occasional instances where mdevctl exits with an error status but does not print an error message. This results in errmsg being an empty string "" (i.e. non-NULL). Simplify the situation by not calling virReportError() at all from within nodeDeviceGetMdevctlCommand() and instead returning an error message for those that previously called virReportError(). The caller is now always responsible for reporting the error. Also introduce a simple macro that converts NULL or empty errmsg to "Unknown Error". Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 34 ++++++++++++++-------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index e6d4e6ccb1..3cf9fc129f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -752,14 +752,13 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, parent_addr = nodeDeviceFindAddressByName(def->parent); if (!parent_addr) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to find parent device '%s'"), def->parent); + *errbuf = g_strdup_printf(_("unable to find parent device '%s'"), + def->parent); return NULL; } if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("couldn't convert node device def to mdevctl JSON")); + *errbuf = g_strdup(_("couldn't convert node device def to mdevctl JSON")); return NULL; }

@@ -832,6 +831,9 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg) } +#define MDEVCTL_ERROR(msg) msg && msg[0] != '\0' ? msg : _("Unknown error") + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDef *def) @@ -846,10 +848,9 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, } if (virMdevctlCreate(def, &uuid, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to start mediated device: %s"), - errmsg); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device: %s"), + MDEVCTL_ERROR(errmsg)); return NULL; } @@ -1201,10 +1202,9 @@ nodeDeviceDestroy(virNodeDevicePtr device) } if (virMdevctlStop(def, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to destroy '%s': %s"), def->name, - errmsg); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name,

+ MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0;

+ MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0;

+ MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0;