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.