On Tue, Aug 18, 2020 at 09:48:00AM -0500, 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>
> ---
> include/libvirt/libvirt-nodedev.h | 4 +
> src/driver-nodedev.h | 6 ++
> src/libvirt-nodedev.c | 42 ++++++++
> src/libvirt_public.syms | 4 +
> src/node_device/node_device_driver.c | 97 +++++++++++++++++--
> src/node_device/node_device_driver.h | 10 ++
> src/node_device/node_device_udev.c | 1 +
> src/remote/remote_driver.c | 1 +
> src/remote/remote_protocol.x | 18 +++-
> src/remote_protocol-structs | 8 ++
> src/rpc/gendispatch.pl | 1 +
> ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 +
> ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 +
> ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 +
> ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 +
> ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 +
> ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 +
> tests/nodedevmdevctltest.c | 68 +++++++++----
> 18 files changed, 241 insertions(+), 25 deletions(-)
> create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
> create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json
> create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
> create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json
> create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
> create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json
>
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index 423a583d45..02aa9d9750 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -127,6 +127,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn,
>
> int virNodeDeviceDestroy (virNodeDevicePtr dev);
>
> +virNodeDevicePtr virNodeDeviceDefineXML (virConnectPtr conn,
> + const char *xmlDesc,
> + unsigned int flags);
Pre-existing, but this forced space-padded alignment feels weird nowadays and
we should use the same policy as for the internal headers - no padding, 1 blank
line separating the function declarations. In fact at the end of the header,
we're breaking the padded "consistency" anyway. So, I'd suggest to abandon
the padding, use a single space as a delimiter and we can send a follow-up
to fix the rest.
the public side of things looks good to me
...
>
> +LIBVIRT_6.5.0 {
> + global:
> + virNodeDeviceDefineXML;
> +} LIBVIRT_6.0.0;
6.8.0 - this is just a reminder as we've managed to push new symbols
referencing an old release once IIRC :)
> # .... 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 affd707a65..16f3537776 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -658,9 +658,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;
> @@ -678,7 +682,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> return NULL;
> }
>
> - cmd = virCommandNewArgList(MDEVCTL, "start",
> + cmd = virCommandNewArgList(MDEVCTL, subcommand,
> "-p", parent_pci,
> "--jsonfile", "/dev/stdin",
> NULL);
Okay, but could we actually make ^this function the internal "public" gateway
to all the mdevctl commands accepting a larger set of arguments and...
> @@ -689,11 +693,29 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> return cmd;
> }
>
> +virCommandPtr
> +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> + char **uuid_out)
...make ^these the static implementations handling the subcommand nuances?
> +{
> + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out);
> +}
> +
> +virCommandPtr
> +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def,
> + char **uuid_out)
> +{
> + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out);
> +}
> +
> +
> +
> static int
> -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +virMdevctlDefineCommon(virNodeDeviceDefPtr def,
> + virCommandPtr (*func)(virNodeDeviceDefPtr, char**),
> + char **uuid)
> {
> int status;
> - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid);
> + g_autoptr(virCommand) cmd = func(def, uuid);
> if (!cmd)
> return -1;
I'm not sure whether this standalone function brings that much value in terms
of abstracting code.
>
> @@ -709,6 +731,20 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> }
>
>
> +static int
> +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +{
> + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlStartCommand, uuid);
I wouldn't mind doing what virMdevctlDefineCommon does right here. In a later
patch you're introducing a virMdevctlCreate command which I think should
actually be part of this function with a bool selector whether we're starting a
transient or a persistent device.
> +}
> +
> +
> +static int
> +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid)
> +{
> + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlDefineCommand, uuid);
here too..
[snip]
> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> index cee0d913dd..f821622ff6 100644
> --- a/tests/nodedevmdevctltest.c
> +++ b/tests/nodedevmdevctltest.c
> @@ -10,10 +10,16 @@
>
> #define VIR_FROM_THIS VIR_FROM_NODEDEV
>
> +typedef enum {
> + MDEVCTL_CMD_START,
> + MDEVCTL_CMD_DEFINE,
> +} MdevctlCmd;
Hmm, with the suggestion I outlined above, we could define all the commands
we're going to use as an enum in node_device_driver.h like VIR_ENUM_DECL.
> +
> struct startTestInfo {
> const char *virt_type;
> int create;
> const char *filename;
> + MdevctlCmd command;
> };
>
> /* capture stdin passed to command */
> @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual,
> return virTestCompareToFile(replacedCmdline, filename);
> }
>
> +
> +typedef virCommandPtr (*GetStartDefineCmdFunc)(virNodeDeviceDefPtr, char **);
> +
> +
> static int
> testMdevctlStart(const char *virt_type,
> int create,
> + GetStartDefineCmdFunc get_cmd_func,
> 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;
> + } else if (info->command == MDEVCTL_CMD_DEFINE) {
> + cmd = "define";
> + func = nodeDeviceGetMdevctlDefineCommand;
> + } else {
^This would then not be needed and we'd simply call
virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)
