[libvirt PATCH 2/2] nodedev: handle failure to generate mdevctl cmd

[Michal Privoznik]

On 4/20/21 4:28 PM, Jonathon Jongsma wrote:
> Coverity complained that the 'default' case of the switch in
> nodeDeviceGetMdevctlCommand() was falling through without initializing
> 'cmd'. Return NULL in this case even though it should never happen.
> Also, make sure calling functions handle the NULL return consistently.
> Some callers already handled it, but some didn't.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>   src/node_device/node_device_driver.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index e565cc29ec..c1fe9db149 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -743,7 +743,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
>       case MDEVCTL_CMD_LAST:
>       default:
>           /* SHOULD NEVER HAPPEN */
> -        break;
> +        return NULL;
>       }
>   
>       switch (cmd_type) {

ACK to this hunk.

> @@ -931,6 +931,9 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg)
>   
>       cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
>   
> +    if (!cmd)
> +        return -1;
> +

But this one and the rest is redundant IMO. The whole design of 
virCommand wraps around caller not checking NULL. For instance the 
following is perfectly valid:

virCommandPtr cmd = virCommandNew("someBinary");

virCommandAddArg(cmd, "arg1");
virCommandPassFD(cmd, 1);

if (virCommandRun(cmd, NULL) < 0) {
   /* error */
}

>       if (virCommandRun(cmd, &status) < 0 || status != 0)

Therefore, if @cmd was NULL then this virCommandRun() returns -1 with 
appropriate error message set. I think instead of introducing these new 
checks the old ones should be removed.

Michal