[libvirt PATCH v2 4/5] nodedev: handle mdevctl errors consistently
Boris Fiuczynski
fiuczy at linux.ibm.com
Wed Jun 23 13:02:23 UTC 2021
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 at 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 at 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
More information about the libvir-list
mailing list