[libvirt PATCH v2 07/12] nodedev: driver: Create a generic mdevctl command translator

[John Ferlan]

On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
> From: Erik Skultety <eskultet at redhat.com>
> 
> Currently there are dedicated wrappers to construct mdevctl command.
> These are mostly fine except for the one that translates both "start"
> and "define" commands, only because mdevctl takes the same set of
> arguments. Instead, keep the wrappers, but let them call a single
> global translator that handles all the mdevctl command differences and
> commonalities.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>   src/node_device/node_device_driver.c | 125 ++++++++++++++++-----------
>   src/node_device/node_device_driver.h |   6 +-
>   tests/nodedevmdevctltest.c           |   4 +-
>   3 files changed, 79 insertions(+), 56 deletions(-)
> 

Coverity found some issues...

> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index da55e386f0..bbb01c3967 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -702,42 +702,75 @@ nodeDeviceFindAddressByName(const char *name)
>   }
>   
>   
> -/* the mdevctl 'start' and 'define' commands accept almost the exact same
> - * arguments, so provide a common implementation that can be wrapped by a more
> - * specific function */
> -static virCommand*
> -nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
> -                                        const char *subcommand,
> -                                        char **uuid_out,
> -                                        char **errmsg)
> +static virCommand *
> +nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
> +                            virMdevctlCommand cmd_type,
> +                            char **outbuf,
> +                            char **errbuf)
>   {
> -    virCommand *cmd;
> -    g_autofree char *json = NULL;
> -    g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
> -
> -    if (!parent_addr) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("unable to find parent device '%s'"), def->parent);
> -        return NULL;
> +    g_autofree char *parent_addr = NULL;
> +    virCommand *cmd = NULL;
> +    const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
> +    g_autofree char *inbuf = NULL;
> +
> +    switch (cmd_type) {
> +    case MDEVCTL_CMD_CREATE:
> +        /* now is the time to make sure "create" is replaced with "start" on
> +         * mdevctl cmdline */
> +        cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
> +        break;
> +    case MDEVCTL_CMD_STOP:
> +    case MDEVCTL_CMD_START:
> +    case MDEVCTL_CMD_DEFINE:
> +    case MDEVCTL_CMD_UNDEFINE:
> +        cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
> +        break;
> +    case MDEVCTL_CMD_LAST:
> +    default:
> +        /* SHOULD NEVER HAPPEN */

Shouldn't happen, but allows code to fall thru w/ later access to @cmd 
which will fail. It's a false positive technically...


> +        break;
>       }
>   
> -    if (nodeDeviceDefToMdevctlConfig(def, &json) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("couldn't convert node device def to mdevctl JSON"));
> -        return NULL;
> -    }
> +    switch (cmd_type) {
> +    case MDEVCTL_CMD_CREATE:
> +    case MDEVCTL_CMD_DEFINE:
> +        parent_addr = nodeDeviceFindAddressByName(def->parent);
>   
> -    cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
> -    virCommandAddArgPair(cmd, "--parent", parent_addr);
> -    virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
> +        if (!parent_addr) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unable to find parent device '%s'"), def->parent);

Leaks @cmd

> +            return NULL;
> +        }
> +
> +        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("couldn't convert node device def to mdevctl JSON"));

Leaks @cmd

> +            return NULL;
> +        }
>   
> -    virCommandSetInputBuffer(cmd, json);
> +        virCommandAddArgPair(cmd, "--parent", parent_addr);
> +        virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
> +
> +        virCommandSetInputBuffer(cmd, inbuf);
> +        virCommandSetOutputBuffer(cmd, outbuf);
> +        break;
> +
> +    case MDEVCTL_CMD_UNDEFINE:
> +    case MDEVCTL_CMD_STOP:
> +    case MDEVCTL_CMD_START:
> +        /* No special handling here, we only need to pass UUID with these */
> +        break;
> +    case MDEVCTL_CMD_LAST:
> +    default:
> +        /* SHOULD NEVER HAPPEN */
> +        break;
> +    }
>   
[...]

Hopefully I haven't missed some other patch along the way as I don't 
keep up to date as much any more. Coverity just keeps running though...

John