[libvirt PATCH v2 0/5] mdev tweaks
A few minor fixes to mdev support in the nodedev driver Changes in v2: - split out the error-reporting macro into a separate commit as recommended by Peter - Since virCommandRun() may report an error, ensure that the virMdevctl$COMMAND() functions always set an error to make error-handling consistent. v1 tried to return an error message and have the caller report the error. - Added a new patch (destroying inactive device) Jonathon Jongsma (5): nodedev: Remove useless device name from error message nodedev: Handle NULL command variable nodedev: add macro to handle command errors nodedev: handle mdevctl errors consistently nodedev: improve error message when destroying an inactive device src/node_device/node_device_driver.c | 147 +++++++++++++++++---------- 1 file changed, 94 insertions(+), 53 deletions(-) -- 2.31.1
At the point where the error message is emitted, the field def->name is still set to "new device", so the error message becomes: Unable to start mediated device 'new device': ... Since the name doesn't contain anything useful, just omit it from the error message altogether. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/node_device/node_device_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8a0a2c3847..0f13cb4849 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -847,8 +847,8 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, if (virMdevctlCreate(def, &uuid, &errmsg) < 0) { if (errmsg) virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to start mediated device '%s': %s"), - def->name, errmsg); + _("Unable to start mediated device: %s"), + errmsg); return NULL; } -- 2.31.1
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
At the point where the error message is emitted, the field def->name is still set to "new device", so the error message becomes:
Unable to start mediated device 'new device': ...
Since the name doesn't contain anything useful, just omit it from the error message altogether.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/node_device/node_device_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8a0a2c3847..0f13cb4849 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -847,8 +847,8 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, if (virMdevctlCreate(def, &uuid, &errmsg) < 0) { if (errmsg) virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to start mediated device '%s': %s"), - def->name, errmsg); + _("Unable to start mediated device: %s"), + errmsg); return NULL; }
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
In commit 68580a51, I removed the checks for NULL cmd variables because virCommandRun() already handles the case where it is called with a NULL cmd. Unfortunately, it handles this case by raising a generic error which is both unhelpful and overwrites our existing error message. So for example, when I attempt to create a mediated device with an invalid parent, I get the following output: virsh # nodedev-create mdev-test.xml error: Failed to create node device from mdev-test.xml error: internal error: invalid use of command API With this patch, I now get a useful error message again: virsh # nodedev-create mdev-test.xml error: Failed to create node device from mdev-test.xml error: internal error: unable to find parent device 'pci_0000_00_03_0' Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0f13cb4849..43a8c1bf60 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -799,6 +799,10 @@ virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_CREATE, 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) @@ -819,6 +823,9 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_DEFINE, 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) @@ -925,6 +932,9 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg); + if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -940,6 +950,9 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg); + if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -955,6 +968,9 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg); + if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; -- 2.31.1
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
In commit 68580a51, I removed the checks for NULL cmd variables because virCommandRun() already handles the case where it is called with a NULL cmd. Unfortunately, it handles this case by raising a generic error which is both unhelpful and overwrites our existing error message. So for example, when I attempt to create a mediated device with an invalid parent, I get the following output:
virsh # nodedev-create mdev-test.xml error: Failed to create node device from mdev-test.xml error: internal error: invalid use of command API
With this patch, I now get a useful error message again:
virsh # nodedev-create mdev-test.xml error: Failed to create node device from mdev-test.xml error: internal error: unable to find parent device 'pci_0000_00_03_0'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0f13cb4849..43a8c1bf60 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -799,6 +799,10 @@ virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_CREATE, 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) @@ -819,6 +823,9 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_DEFINE, 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) @@ -925,6 +932,9 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
+ if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1;
@@ -940,6 +950,9 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg);
+ if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1;
@@ -955,6 +968,9 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg);
+ if (!cmd) + return -1; + if (virCommandRun(cmd, &status) < 0 || status != 0) return -1;
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
This macro will be utilized in the following patch. Since mdevctl commands can fail with or without an error message, this macro makes it easy to print a fallback error in the case that the error message is not set. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 43a8c1bf60..eb85cc0439 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -57,6 +57,9 @@ VIR_ENUM_IMPL(virMdevctlCommand, ); +#define MDEVCTL_ERROR(msg) (msg && msg[0] != '\0' ? msg : _("Unknown error")) + + virDrvOpenStatus nodeConnectOpen(virConnectPtr conn, virConnectAuthPtr auth G_GNUC_UNUSED, @@ -1387,7 +1390,7 @@ nodeDeviceUndefine(virNodeDevice *device, if (virMdevctlUndefine(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to undefine mediated device: %s"), - errmsg && errmsg[0] ? errmsg : "Unknown Error"); + MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0; @@ -1434,7 +1437,7 @@ nodeDeviceCreate(virNodeDevice *device, if (virMdevctlStart(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to create mediated device: %s"), - errmsg && errmsg[0] ? errmsg : "Unknown Error"); + MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0; -- 2.31.1
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
This macro will be utilized in the following patch. Since mdevctl commands can fail with or without an error message, this macro makes it easy to print a fallback error in the case that the error message is not set.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 43a8c1bf60..eb85cc0439 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -57,6 +57,9 @@ VIR_ENUM_IMPL(virMdevctlCommand, );
+#define MDEVCTL_ERROR(msg) (msg && msg[0] != '\0' ? msg : _("Unknown error")) + + virDrvOpenStatus nodeConnectOpen(virConnectPtr conn, virConnectAuthPtr auth G_GNUC_UNUSED, @@ -1387,7 +1390,7 @@ nodeDeviceUndefine(virNodeDevice *device, if (virMdevctlUndefine(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to undefine mediated device: %s"), - errmsg && errmsg[0] ? errmsg : "Unknown Error"); + MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0; @@ -1434,7 +1437,7 @@ nodeDeviceCreate(virNodeDevice *device, if (virMdevctlStart(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to create mediated device: %s"), - errmsg && errmsg[0] ? errmsg : "Unknown Error"); + MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0;
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
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", -- 2.31.1
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
When trying to destroy a node device that is not active, we end up with a confusing error message: # nodedev-destroy mdev_88a6b868_46bd_4015_8e5b_26107f82da38 error: Failed to destroy node device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' error: failed to access '/sys/bus/mdev/devices/88a6b868-46bd-4015-8e5b-26107f82da38/iommu_group': No such file or directory With this patch, the error is more clear: # nodedev-destroy mdev_88a6b868_46bd_4015_8e5b_26107f82da38 error: Failed to destroy node device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' error: Requested operation is not valid: Device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' is not active Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 497db0006a..721ba96203 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1227,6 +1227,15 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *vfiogroup = NULL; + VIR_AUTOCLOSE fd = -1; + + if (!virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Device '%s' is not active"), def->name); + goto cleanup; + } + /* If this mediated device is in use by a vm, attempting to stop it * will block until the vm closes the device. The nodedev driver * cannot query the hypervisor driver to determine whether the device @@ -1236,10 +1245,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be opened by one user at a time. So if we get EBUSY when opening * the group, we infer that the device is in use and therefore we * shouldn't try to remove the device. */ - g_autofree char *vfiogroup = - virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); - VIR_AUTOCLOSE fd = -1; - + vfiogroup = virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); if (!vfiogroup) goto cleanup; -- 2.31.1
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
When trying to destroy a node device that is not active, we end up with a confusing error message:
# nodedev-destroy mdev_88a6b868_46bd_4015_8e5b_26107f82da38 error: Failed to destroy node device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' error: failed to access '/sys/bus/mdev/devices/88a6b868-46bd-4015-8e5b-26107f82da38/iommu_group': No such file or directory
With this patch, the error is more clear:
# nodedev-destroy mdev_88a6b868_46bd_4015_8e5b_26107f82da38 error: Failed to destroy node device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' error: Requested operation is not valid: Device 'mdev_88a6b868_46bd_4015_8e5b_26107f82da38' is not active
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 497db0006a..721ba96203 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1227,6 +1227,15 @@ nodeDeviceDestroy(virNodeDevicePtr device)
ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *vfiogroup = NULL; + VIR_AUTOCLOSE fd = -1; + + if (!virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Device '%s' is not active"), def->name); + goto cleanup; + } + /* If this mediated device is in use by a vm, attempting to stop it * will block until the vm closes the device. The nodedev driver * cannot query the hypervisor driver to determine whether the device @@ -1236,10 +1245,7 @@ nodeDeviceDestroy(virNodeDevicePtr device) * to be opened by one user at a time. So if we get EBUSY when opening * the group, we infer that the device is in use and therefore we * shouldn't try to remove the device. */ - g_autofree char *vfiogroup = - virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); - VIR_AUTOCLOSE fd = -1; - + vfiogroup = virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); if (!vfiogroup) goto cleanup;
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
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
A few minor fixes to mdev support in the nodedev driver
Changes in v2: - split out the error-reporting macro into a separate commit as recommended by Peter - Since virCommandRun() may report an error, ensure that the virMdevctl$COMMAND() functions always set an error to make error-handling consistent. v1 tried to return an error message and have the caller report the error. - Added a new patch (destroying inactive device)
Jonathon Jongsma (5): nodedev: Remove useless device name from error message nodedev: Handle NULL command variable nodedev: add macro to handle command errors nodedev: handle mdevctl errors consistently nodedev: improve error message when destroying an inactive device
src/node_device/node_device_driver.c | 147 +++++++++++++++++---------- 1 file changed, 94 insertions(+), 53 deletions(-)
I'm sorry for not picking this earlier up. We are pretty close to the release (which is supposed to happen tomorrow). Are you okay with me waiting one more day and push this after the release? Michal
On Wed, Jun 30, 2021 at 3:18 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
A few minor fixes to mdev support in the nodedev driver
Changes in v2: - split out the error-reporting macro into a separate commit as recommended by Peter - Since virCommandRun() may report an error, ensure that the virMdevctl$COMMAND() functions always set an error to make error-handling consistent. v1 tried to return an error message and have the caller report the error. - Added a new patch (destroying inactive device)
Jonathon Jongsma (5): nodedev: Remove useless device name from error message nodedev: Handle NULL command variable nodedev: add macro to handle command errors nodedev: handle mdevctl errors consistently nodedev: improve error message when destroying an inactive device
src/node_device/node_device_driver.c | 147 +++++++++++++++++---------- 1 file changed, 94 insertions(+), 53 deletions(-)
I'm sorry for not picking this earlier up. We are pretty close to the release (which is supposed to happen tomorrow). Are you okay with me waiting one more day and push this after the release?
Michal
These are fairly minor fixes so it doesn't really matter if they wait for the next release. The one that I was hoping would get into a release sooner is this one: https://listman.redhat.com/archives/libvir-list/2021-June/msg00271.html
On 6/30/21 3:56 PM, Jonathon Jongsma wrote:
On Wed, Jun 30, 2021 at 3:18 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 6/22/21 9:53 PM, Jonathon Jongsma wrote:
A few minor fixes to mdev support in the nodedev driver
Changes in v2: - split out the error-reporting macro into a separate commit as recommended by Peter - Since virCommandRun() may report an error, ensure that the virMdevctl$COMMAND() functions always set an error to make error-handling consistent. v1 tried to return an error message and have the caller report the error. - Added a new patch (destroying inactive device)
Jonathon Jongsma (5): nodedev: Remove useless device name from error message nodedev: Handle NULL command variable nodedev: add macro to handle command errors nodedev: handle mdevctl errors consistently nodedev: improve error message when destroying an inactive device
src/node_device/node_device_driver.c | 147 +++++++++++++++++---------- 1 file changed, 94 insertions(+), 53 deletions(-)
I'm sorry for not picking this earlier up. We are pretty close to the release (which is supposed to happen tomorrow). Are you okay with me waiting one more day and push this after the release?
Michal
These are fairly minor fixes so it doesn't really matter if they wait for the next release. The one that I was hoping would get into a release sooner is this one: https://listman.redhat.com/archives/libvir-list/2021-June/msg00271.html
Pushed now. Michal
participants (3)
-
Boris Fiuczynski -
Jonathon Jongsma -
Michal Prívozník