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@xxxxxxxxxx>
> ---
...
> 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
