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@xxxxxxxxxx>
> ---
> 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.
