[libvirt PATCH 0/3] mdev tweaks
A few minor fixes to mdev support in the nodedev driver Jonathon Jongsma (3): nodedev: Remove useless device name from error message nodedev: Handle NULL command variable nodedev: handle mdevctl errors consistently src/node_device/node_device_driver.c | 45 ++++++++++++++-------------- 1 file changed, 23 insertions(+), 22 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> --- 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 Tue, Jun 15, 2021 at 11:28:38 -0500, 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> --- src/node_device/node_device_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0f13cb4849..e6d4e6ccb1 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -799,9 +799,10 @@ virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_CREATE, uuid, errmsg); + /* 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 (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1; /* remove newline */ @@ -821,7 +822,7 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg) /* 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 (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1; /* remove newline */ @@ -925,7 +926,7 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg); - if (virCommandRun(cmd, &status) < 0 || status != 0) + if (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1; return 0; @@ -940,7 +941,7 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg); - if (virCommandRun(cmd, &status) < 0 || status != 0) + if (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1; return 0; @@ -955,7 +956,7 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg) cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg); - if (virCommandRun(cmd, &status) < 0 || status != 0) + if (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1; return 0; -- 2.31.1
On Tue, Jun 15, 2021 at 11:28:39 -0500, 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 | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0f13cb4849..e6d4e6ccb1 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -799,9 +799,10 @@ virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg) MDEVCTL_CMD_CREATE, uuid, errmsg); + /* 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 (!cmd || virCommandRun(cmd, &status) < 0 || status != 0) return -1;
Preferably make the '!cmd' condition separate or around the call to 'nodeDeviceGetMdevctlCommand' so that it's obvious that the error originates from there. Additionally virCommandRun if the @exitstatus is non-NULL and the command returns a non-zero value, which you check for will not set an error, so you still have inconsistent behaviour in error reporting.
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@redhat.com> --- 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; @@ -1310,9 +1310,9 @@ nodeDeviceDefineXML(virConnect *conn, } if (virMdevctlDefine(def, &uuid, &errmsg) < 0) { - if (errmsg) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to define mediated device: %s"), errmsg); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to define mediated device: %s"), + MDEVCTL_ERROR(errmsg)); return NULL; } @@ -1372,7 +1372,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; @@ -1419,7 +1419,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 Tue, Jun 15, 2021 at 11:28:40 -0500, Jonathon Jongsma wrote:
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@redhat.com> --- 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; }
I'm not sure whether this works properly with translations.
@@ -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,
This error message should contain also "mediated device" so that it's easier to translate.
+ MDEVCTL_ERROR(errmsg)); goto cleanup; } ret = 0;
[...]
@@ -1372,7 +1372,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; @@ -1419,7 +1419,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;
These two almost look like a separate refactor.
participants (2)
-
Jonathon Jongsma -
Peter Krempa