[libvirt PATCH v3 12/21] api: add virNodeDeviceDefineXML()

[Erik Skultety]

On Thu, Dec 24, 2020 at 08:14:36AM -0600, Jonathon Jongsma wrote:
> With mediated devices, we can now define persistent node devices that
> can be started and stopped. In order to take advantage of this, we need
> an API to define new node devices.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
...

> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index cf31f937d5..8fae4352ff 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -877,6 +877,7 @@ LIBVIRT_6.10.0 {

This will likely be 7.1.0+ - just so that we don't forget to update it :)

>      global:
>          virDomainAuthorizedSSHKeysGet;
>          virDomainAuthorizedSSHKeysSet;
> +        virNodeDeviceDefineXML;
>  } LIBVIRT_6.0.0;
>  
>  # .... define new API here using predicted next version number ....
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 0bebd534d0..4fbe8743b4 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -698,9 +698,13 @@ nodeDeviceFindAddressByName(const char *name)
>  }
>  
>  
> -virCommandPtr
> -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> -                                 char **uuid_out)
> +/* 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 virCommandPtr
> +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDefPtr def,
> +                                       const char *subcommand,
> +                                       char **uuid_out)
>  {
>      virCommandPtr cmd;
>      g_autofree char *json = NULL;
> @@ -718,7 +722,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
>          return NULL;
>      }
>  
> -    cmd = virCommandNewArgList(MDEVCTL, "start",
> +    cmd = virCommandNewArgList(MDEVCTL, subcommand,
>                                 "-p", parent_addr,
>                                 "--jsonfile", "/dev/stdin",
>                                 NULL);
> @@ -729,6 +733,22 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
>      return cmd;
>  }
>  
> +virCommandPtr
> +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> +                                 char **uuid_out)
> +{
> +    return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out);
> +}
> +

^These hunks should be in a separate preparation patch

...

> +    .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 6.5.0 */

7.x.0

>      .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */
> +    .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 6.5.0 */

7.x.0

>      .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */
>  };
>  
>  
>  /* capture stdin passed to command */
> @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual,
>      return virTestCompareToFile(replacedCmdline, filename);
>  }

<HUNK_START

>  
> +
> +typedef virCommandPtr (*GetStartDefineCmdFunc)(virNodeDeviceDefPtr, char **);

MdevctlCmdFunc would be probably a better name.

> +
> +
>  static int
>  testMdevctlStart(const char *virt_type,
>                   int create,
> +                 GetStartDefineCmdFunc get_cmd_func,

mdevctl_cmd_func probably?

>                   const char *mdevxml,
> -                 const char *startcmdfile,
> -                 const char *startjsonfile)
> +                 const char *cmdfile,
> +                 const char *jsonfile)
>  {
>      g_autoptr(virNodeDeviceDef) def = NULL;
>      virNodeDeviceObjPtr obj = NULL;
> @@ -66,7 +77,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 = get_cmd_func(def, &uuid);
>  
>      if (!cmd)
>          goto cleanup;
> @@ -78,10 +89,10 @@ testMdevctlStart(const char *virt_type,
>      if (!(actualCmdline = virBufferCurrentContent(&buf)))
>          goto cleanup;
>  
> -    if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0)
> +    if (nodedevCompareToFile(actualCmdline, cmdfile) < 0)
>          goto cleanup;
>  
> -    if (virTestCompareToFile(stdinbuf, startjsonfile) < 0)
> +    if (virTestCompareToFile(stdinbuf, jsonfile) < 0)
>          goto cleanup;
>  
>      ret = 0;
> @@ -96,17 +107,31 @@ static int
>  testMdevctlStartHelper(const void *data)
>  {
>      const struct startTestInfo *info = data;
> +    const char *cmd;
> +    GetStartDefineCmdFunc func;
> +    g_autofree char *mdevxml = NULL;
> +    g_autofree char *cmdlinefile = NULL;
> +    g_autofree char *jsonfile = NULL;
> +
> +    if (info->command == MDEVCTL_CMD_START) {
> +        cmd = "start";
> +        func = nodeDeviceGetMdevctlStartCommand;

<HUNK_END

^These hunks should be in a separate preparation patch (part of the one I
mentioned earlier in this reply).

> +    } else if (info->command == MDEVCTL_CMD_DEFINE) {
> +        cmd = "define";
> +        func = nodeDeviceGetMdevctlDefineCommand;
> +    } else {
> +        return -1;
> +    }
>  
> -    g_autofree char *mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml",
> -                                               abs_srcdir, info->filename);
> -    g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.argv",
> -                                                   abs_srcdir, info->filename);
> -    g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.json",
> -                                                   abs_srcdir, info->filename);
> +    mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
> +                              info->filename);
> +    cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.argv",
> +                                  abs_srcdir, info->filename, cmd);
> +    jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir,
> +                               info->filename, cmd);
>  
> -    return testMdevctlStart(info->virt_type,
> -                            info->create, mdevxml, cmdlinefile,
> -                            jsonfile);
> +    return testMdevctlStart(info->virt_type, info->create, func,
> +                            mdevxml, cmdlinefile, jsonfile);
>  }

The same applies to ^these hunks as well.

>  
>  static int
> @@ -347,15 +372,18 @@ mymain(void)
>      if (virTestRun(desc, func, info) < 0) \
>          ret = -1;
>  
> -#define DO_TEST_START_FULL(virt_type, create, filename) \
> +#define DO_TEST_START_FULL(desc, virt_type, create, filename, command) \

At this point I think this wrapper macro should be called DO_TEST_CMD instead.

The API functionality looks okay, ACK.

Erik