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@redhat.com> --- 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>, ...)