On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
Currently, we have three different types of mdevctl errors: 1. the command cannot be constructed ecause of unsatisfied preconditions 2. the command cannot be executed due to some error 3. the command is executed, but returns an error status
These different failures are handled differently. Some cases set an error and return and error status, and some return a error message but do not set an error.
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 ensuring that virReportError() is called for all error conditions rather than returning an error message back to the calling function.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 114 +++++++++++++++------------ 1 file changed, 65 insertions(+), 49 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index eb85cc0439..497db0006a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -746,6 +746,8 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, case MDEVCTL_CMD_LAST: default: /* SHOULD NEVER HAPPEN */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown Command '%i'"), cmd_type); return NULL; }
@@ -795,48 +797,62 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
static int -virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg) +virMdevctlCreate(virNodeDeviceDef *def, char **uuid) { int status; + g_autofree char *errmsg = NULL; g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_CREATE, uuid, - errmsg); + &errmsg);
if (!cmd) return -1;
/* an auto-generated uuid is returned via stdout if no uuid is specified in * the mdevctl args */ - if (virCommandRun(cmd, &status) < 0 || status != 0) + if (virCommandRun(cmd, &status) < 0) + return -1; + + if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device: %s"), + MDEVCTL_ERROR(errmsg)); return -1; + }
/* remove newline */ *uuid = g_strstrip(*uuid); - return 0; }
static int -virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg) +virMdevctlDefine(virNodeDeviceDef *def, char **uuid) { int status; + g_autofree char *errmsg = NULL; g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_DEFINE, - uuid, errmsg); + uuid, &errmsg);
if (!cmd) return -1;
/* an auto-generated uuid is returned via stdout if no uuid is specified in * the mdevctl args */ - if (virCommandRun(cmd, &status) < 0 || status != 0) + if (virCommandRun(cmd, &status) < 0) return -1;
+ if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to define mediated device: %s"), + MDEVCTL_ERROR(errmsg)); + return -1; + } + /* remove newline */ *uuid = g_strstrip(*uuid); - return 0; }
@@ -846,7 +862,6 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDef *def) { g_autofree char *uuid = NULL; - g_autofree char *errmsg = NULL;
if (!def->parent) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -854,11 +869,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; }
- if (virMdevctlCreate(def, &uuid, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to start mediated device: %s"), - errmsg); + if (virMdevctlCreate(def, &uuid) < 0) { return NULL; }
@@ -928,55 +939,79 @@ nodeDeviceCreateXML(virConnectPtr conn,
static int -virMdevctlStop(virNodeDeviceDef *def, char **errmsg) +virMdevctlStop(virNodeDeviceDef *def) { int status; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL;
- cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg); + cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, &errmsg);
if (!cmd) return -1;
- if (virCommandRun(cmd, &status) < 0 || status != 0) + if (virCommandRun(cmd, &status) < 0) return -1;
+ if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name, + MDEVCTL_ERROR(errmsg)); + return -1; + } + return 0; }
static int -virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) +virMdevctlUndefine(virNodeDeviceDef *def) { int status; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL;
- cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg); + cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, &errmsg);
if (!cmd) return -1;
- if (virCommandRun(cmd, &status) < 0 || status != 0) + if (virCommandRun(cmd, &status) < 0) return -1;
+ if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to undefine mediated device: %s"), + MDEVCTL_ERROR(errmsg)); + return -1; + } + return 0; }
static int -virMdevctlStart(virNodeDeviceDef *def, char **errmsg) +virMdevctlStart(virNodeDeviceDef *def) { int status; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL;
- cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg); + cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, &errmsg);
if (!cmd) return -1;
- if (virCommandRun(cmd, &status) < 0 || status != 0) + if (virCommandRun(cmd, &status) < 0) return -1;
+ if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to create mediated device: %s"), + MDEVCTL_ERROR(errmsg)); + return -1; + } + return 0; }
@@ -1204,7 +1239,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) g_autofree char *vfiogroup = virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); VIR_AUTOCLOSE fd = -1; - g_autofree char *errmsg = NULL;
if (!vfiogroup) goto cleanup; @@ -1218,13 +1252,9 @@ nodeDeviceDestroy(virNodeDevicePtr device) goto cleanup; }
- if (virMdevctlStop(def, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to destroy '%s': %s"), def->name, - errmsg); + if (virMdevctlStop(def) < 0) goto cleanup; - } + ret = 0; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1299,7 +1329,6 @@ nodeDeviceDefineXML(virConnect *conn, g_autoptr(virNodeDeviceDef) def = NULL; const char *virt_type = NULL; g_autofree char *uuid = NULL; - g_autofree char *errmsg = NULL; g_autofree char *name = NULL;
virCheckFlags(0, NULL); @@ -1327,10 +1356,7 @@ nodeDeviceDefineXML(virConnect *conn, return NULL; }
- if (virMdevctlDefine(def, &uuid, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to define mediated device: %s"), errmsg); + if (virMdevctlDefine(def, &uuid) < 0) { return NULL; }
@@ -1385,14 +1411,9 @@ nodeDeviceUndefine(virNodeDevice *device, }
if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { - g_autofree char *errmsg = NULL; - - if (virMdevctlUndefine(def, &errmsg) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to undefine mediated device: %s"), - MDEVCTL_ERROR(errmsg)); + if (virMdevctlUndefine(def) < 0) goto cleanup; - } + ret = 0; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -1432,14 +1453,9 @@ nodeDeviceCreate(virNodeDevice *device, goto cleanup;
if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { - g_autofree char *errmsg = NULL; - - if (virMdevctlStart(def, &errmsg) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to create mediated device: %s"), - MDEVCTL_ERROR(errmsg)); + if (virMdevctlStart(def) < 0) goto cleanup; - } + ret = 0; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
Just wondering... after reading method "create" throwing "start" and method "start" throwing "create"... I created these tables Method => New error message =============================================================== virMdevctlCreate => "Unable to start mediated device: %s" virMdevctlDefine => "Unable to define mediated device: %s" virMdevctlStop => "Unable to destroy '%s': %s" virMdevctlUndefine => "Unable to undefine mediated device: %s" virMdevctlStart => "Unable to create mediated device: %s" Method => Old error message => Calls now =============================================================== nodeDeviceCreateXMLMdev => "Unable to start mediated device: %s" => virMdevctlCreate nodeDeviceDestroy => "Unable to destroy '%s': %s" => virMdevctlStop nodeDeviceDefineXML => "Unable to define mediated device: %s" => virMdevctlDefine nodeDeviceUndefine => "Unable to undefine mediated device: %s" => virMdevctlUndefine nodeDeviceCreate => "Unable to create mediated device: %s" => virMdevctlStart Overall all messages remain as before... just the "method name" - "error message"-matching is sometimes a bit strange. Anyway Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294