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(a)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>, ...)