On Fri, Mar 26, 2021 at 11:47:57AM -0500, Jonathon Jongsma wrote:
> When an mdevctl command fails, there is not much information available
> to the user about why it failed. This is partly because we were not
> making use of the error message that mdevctl itself prints upon failure.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/node_device/node_device_driver.c | 46 +++++++++++++++++-----------
> src/node_device/node_device_driver.h | 6 ++--
> tests/nodedevmdevctltest.c | 6 ++--
> 3 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 543e5bb90a..3851a3568f 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -700,7 +700,8 @@ nodeDeviceFindAddressByName(const char *name)
>
> virCommandPtr
> nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> - char **uuid_out)
> + char **uuid_out,
> + char **errmsg)
> {
> virCommandPtr cmd;
> g_autofree char *json = NULL;
> @@ -725,15 +726,17 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
>
> virCommandSetInputBuffer(cmd, json);
> virCommandSetOutputBuffer(cmd, uuid_out);
> + virCommandSetErrorBuffer(cmd, errmsg);
>
> return cmd;
> }
>
> static int
> -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg)
> {
> int status;
> - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid);
> + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid,
> + errmsg);
> if (!cmd)
> return -1;
>
> @@ -754,6 +757,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn,
> virNodeDeviceDefPtr def)
> {
> g_autofree char *uuid = NULL;
> + g_autofree char *errmsg = NULL;
>
> if (!def->parent) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -761,9 +765,10 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn,
> return NULL;
> }
>
> - if (virMdevctlStart(def, &uuid) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Unable to start mediated device"));
> + if (virMdevctlStart(def, &uuid, &errmsg) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to start mediated device '%s': %s"), def->name,
> + errmsg && errmsg[0] ? errmsg : "Unknown Error");
"Unknown Error" is fairly unhelpful to the user. It should also be reported
*only* when errmsg exists otherwise an error should have been logged from other
internal helpers up the stack.
The scenario when I hit it was when I tried to define an mdev with parent
"0000:06:00" not knowing where exactly it failed. Turns out, both
nodeDeviceGetMdevctlDefineStartCommand and nodeDeviceFindAddressByName call
virReportError which would tell me more about it, but little did I know that
VIR_ERR_NO_XYZ are intended/reserved error codes for public APIs, not internal
use. That's because we call daemonErrorLogFilter just before logging the error
to perform the final log priority change which demotes every error message
generated internally with an error code of VIR_ERROR_NO_XYZ (see [1]).
Therefore we'll have to fix the two callers mentioned above to report an
internal error instead, but we can easily do that in a follow up patch - let's
just not forget about it :).
Otherwise the patch looks good. With the "Unknown Error" message gone:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
[1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_daemon.c#L89)
> return NULL;
> }
>
> @@ -828,23 +833,25 @@ nodeDeviceCreateXML(virConnectPtr conn,
>
>
> virCommandPtr
> -nodeDeviceGetMdevctlStopCommand(const char *uuid)
> -{
> - return virCommandNewArgList(MDEVCTL,
> - "stop",
> - "-u",
> - uuid,
> - NULL);
> +nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg)
> +{
> + virCommandPtr cmd = virCommandNewArgList(MDEVCTL,
> + "stop",
> + "-u",
> + uuid,
> + NULL);
> + virCommandSetErrorBuffer(cmd, errmsg);
> + return cmd;
>
> }
>
> static int
> -virMdevctlStop(virNodeDeviceDefPtr def)
> +virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg)
> {
> int status;
> g_autoptr(virCommand) cmd = NULL;
>
> - cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid);
> + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg);
>
> if (virCommandRun(cmd, &status) < 0 || status != 0)
> return -1;
> @@ -901,9 +908,12 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>
> ret = 0;
> } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
> - if (virMdevctlStop(def) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Unable to stop mediated device"));
> + g_autofree char *errmsg = NULL;
> +
> + if (virMdevctlStop(def, &errmsg) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to destroy '%s': %s"), def->name,
> + errmsg && errmsg[0] ? errmsg : "Unknown error");
> goto cleanup;
> }
> ret = 0;
> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> index 2113d2b0a5..4a40aa51f6 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -115,6 +115,8 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
>
> virCommandPtr
> nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> - char **uuid_out);
> + char **uuid_out,
> + char **errmsg);
> virCommandPtr
> -nodeDeviceGetMdevctlStopCommand(const char *uuid);
> +nodeDeviceGetMdevctlStopCommand(const char *uuid,
> + char **errmsg);
> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> index 1bad65549b..c12feaac55 100644
> --- a/tests/nodedevmdevctltest.c
> +++ b/tests/nodedevmdevctltest.c
> @@ -58,6 +58,7 @@ testMdevctlStart(const char *virt_type,
> const char *actualCmdline = NULL;
> int ret = -1;
> g_autofree char *uuid = NULL;
> + g_autofree char *errmsg = NULL;
> g_autofree char *stdinbuf = NULL;
> g_autoptr(virCommand) cmd = NULL;
>
> @@ -66,7 +67,7 @@ testMdevctlStart(const char *virt_type,
>
> /* this function will set a stdin buffer containing the json configuration
> * of the device. The json value is captured in the callback above */
> - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid);
> + cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg);
>
> if (!cmd)
> goto cleanup;
> @@ -117,11 +118,12 @@ testMdevctlStop(const void *data)
> const char *actualCmdline = NULL;
> int ret = -1;
> g_autoptr(virCommand) cmd = NULL;
> + g_autofree char *errmsg = NULL;
> g_autofree char *cmdlinefile =
> g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv",
> abs_srcdir);
>
> - cmd = nodeDeviceGetMdevctlStopCommand(uuid);
> + cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg);
>
> if (!cmd)
> goto cleanup;
> --
> 2.26.3
>
