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);
+
/**
* VIR_NODE_DEVICE_EVENT_CALLBACK:
*
diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h
index d0fc7f19cf..16d787f3fc 100644
--- a/src/driver-nodedev.h
+++ b/src/driver-nodedev.h
@@ -74,6 +74,11 @@ typedef virNodeDevicePtr
typedef int
(*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
+typedef virNodeDevicePtr
+(*virDrvNodeDeviceDefineXML)(virConnectPtr conn,
+ const char *xmlDesc,
+ unsigned int flags);
+
typedef int
(*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn,
virNodeDevicePtr dev,
@@ -113,4 +118,5 @@ struct _virNodeDeviceDriver {
virDrvNodeDeviceListCaps nodeDeviceListCaps;
virDrvNodeDeviceCreateXML nodeDeviceCreateXML;
virDrvNodeDeviceDestroy nodeDeviceDestroy;
+ virDrvNodeDeviceDefineXML nodeDeviceDefineXML;
};
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index 6dc61304a0..1704de929e 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -761,6 +761,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev)
}
+/**
+ * virNodeDeviceDefineXML:
+ * @conn: pointer to the hypervisor connection
+ * @xmlDesc: string containing an XML description of the device to be defined
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Define a new device on the VM host machine, for example, a mediated device
+ *
+ * virNodeDeviceFree should be used to free the resources after the
+ * node device object is no longer needed.
+ *
+ * Returns a node device object if successful, NULL in case of failure
+ */
+virNodeDevicePtr
+virNodeDeviceDefineXML(virConnectPtr conn,
+ const char *xmlDesc,
+ unsigned int flags)
+{
+ VIR_DEBUG("conn=%p, xmlDesc=%s, flags=0x%x", conn, NULLSTR(xmlDesc), flags);
+
+ virResetLastError();
+
+ virCheckConnectReturn(conn, NULL);
+ virCheckReadOnlyGoto(conn->flags, error);
+ virCheckNonNullArgGoto(xmlDesc, error);
+
+ if (conn->nodeDeviceDriver &&
+ conn->nodeDeviceDriver->nodeDeviceDefineXML) {
+ virNodeDevicePtr dev = conn->nodeDeviceDriver->nodeDeviceDefineXML(conn, xmlDesc, flags);
+ if (dev == NULL)
+ goto error;
+ return dev;
+ }
+
+ virReportUnsupportedError();
+
+ error:
+ virDispatchError(conn);
+ return NULL;
+}
+
+
/**
* virConnectNodeDeviceEventRegisterAny:
* @conn: pointer to the connection
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 539d2e3943..27c637e37c 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -873,4 +873,8 @@ LIBVIRT_6.0.0 {
virDomainBackupGetXMLDesc;
} LIBVIRT_5.10.0;
+LIBVIRT_6.5.0 {
+ global:
+ 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 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);
@@ -689,11 +693,29 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
return cmd;
}
+virCommandPtr
+nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
+ char **uuid_out)
+{
+ 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;
@@ -709,6 +731,20 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
}
+static int
+virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
+{
+ return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlStartCommand, uuid);
+}
+
+
+static int
+virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid)
+{
+ return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlDefineCommand, uuid);
+}
+
+
static virNodeDevicePtr
nodeDeviceCreateXMLMdev(virConnectPtr conn,
virNodeDeviceDefPtr def)
@@ -1008,6 +1044,55 @@ nodeDeviceDestroy(virNodeDevicePtr device)
return ret;
}
+virNodeDevicePtr
+nodeDeviceDefineXML(virConnectPtr conn,
+ const char *xmlDesc,
+ unsigned int flags)
+{
+ g_autoptr(virNodeDeviceDef) def = NULL;
+ virNodeDevicePtr device = NULL;
+ const char *virt_type = NULL;
+
+ virCheckFlags(0, NULL);
+
+ if (nodeDeviceWaitInit() < 0)
+ return NULL;
+
+ virt_type = virConnectGetType(conn);
+
+ if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
+ return NULL;
+
+ if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0)
+ return NULL;
+
+ if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
+ g_autofree char *uuid = NULL;
+
+ if (!def->parent) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("cannot define a mediated device without a parent"));
+ return NULL;
+ }
+
+ if (virMdevctlDefine(def, &uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to define mediated device"));
+ return NULL;
+ }
+
+ def->caps->data.mdev.uuid = g_strdup(uuid);
+ mdevGenerateDeviceName(def);
+ device = nodeDeviceFindNewMediatedDevice(conn, uuid);
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Unsupported device type"));
+ }
+
+ return device;
+}
+
+
int
nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index b5c1da4ff3..cf03e0b3cf 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -107,6 +107,11 @@ nodeDeviceCreateXML(virConnectPtr conn,
int
nodeDeviceDestroy(virNodeDevicePtr dev);
+virNodeDevicePtr
+nodeDeviceDefineXML(virConnectPtr conn,
+ const char *xmlDesc,
+ unsigned int flags);
+
int
nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn,
virNodeDevicePtr dev,
@@ -121,6 +126,11 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
virCommandPtr
nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
char **uuid_out);
+
+virCommandPtr
+nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def,
+ char **uuid_out);
+
virCommandPtr
nodeDeviceGetMdevctlStopCommand(const char *uuid);
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index a85418e549..5101fff146 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2198,6 +2198,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {
.nodeDeviceListCaps = nodeDeviceListCaps, /* 0.7.3 */
.nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */
.nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */
+ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 6.5.0 */
};
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 0331060a2d..64d0d0c49b 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8814,6 +8814,7 @@ static virNodeDeviceDriver node_device_driver = {
.nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */
.nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */
.nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */
+ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 6.5.0 */
.nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */
};
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index d4393680e9..ddfdb9c084 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2139,6 +2139,15 @@ struct remote_node_device_destroy_args {
remote_nonnull_string name;
};
+struct remote_node_device_define_xml_args {
+ remote_nonnull_string xml_desc;
+ unsigned int flags;
+};
+
+struct remote_node_device_define_xml_ret {
+ remote_nonnull_node_device dev;
+};
+
/*
* Events Register/Deregister:
@@ -6664,5 +6673,12 @@ enum remote_procedure {
* @priority: high
* @acl: domain:read
*/
- REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422
+ REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422,
+
+ /**
+ * @generate: both
+ * @acl: node_device:write
+ * @acl: node_device:start
+ */
+ REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index bae0f0b545..c325923eaf 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1600,6 +1600,13 @@ struct remote_node_device_create_xml_ret {
struct remote_node_device_destroy_args {
remote_nonnull_string name;
};
+struct remote_node_device_define_xml_args {
+ remote_nonnull_string xml_desc;
+ u_int flags;
+};
+struct remote_node_device_define_xml_ret {
+ remote_nonnull_node_device dev;
+};
struct remote_connect_domain_event_register_ret {
int cb_registered;
};
@@ -3558,4 +3565,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_AGENT_SET_RESPONSE_TIMEOUT = 420,
REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 421,
REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422,
+ REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423,
};
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 0b2ae59910..fa409fe3ab 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -567,6 +567,7 @@ elsif ($mode eq "server") {
if ($argtype =~ m/^remote_node_device_/ and
!($argtype =~ m/^remote_node_device_lookup_by_name_/) and
!($argtype =~ m/^remote_node_device_create_xml_/) and
+ !($argtype =~ m/^remote_node_device_define_xml_/) and
!($argtype =~ m/^remote_node_device_lookup_scsi_host_by_wwn_/)) {
$has_node_device = 1;
push(@vars_list, "virNodeDevicePtr dev = NULL");
diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
new file mode 100644
index 0000000000..773e98b963
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
@@ -0,0 +1 @@
+$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin
diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json
new file mode 100644
index 0000000000..bfc6dcace3
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json
@@ -0,0 +1 @@
+{"mdev_type":"i915-GVTg_V5_8","start":"manual"}
\ No newline at end of file
diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
new file mode 100644
index 0000000000..773e98b963
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
@@ -0,0 +1 @@
+$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin
diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json
new file mode 100644
index 0000000000..e5b22b2c44
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json
@@ -0,0 +1 @@
+{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute-1":"attribute-value-1"},{"example-attribute-2":"attribute-value-2"}]}
\ No newline at end of file
diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
new file mode 100644
index 0000000000..773e98b963
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
@@ -0,0 +1 @@
+$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin
diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json
new file mode 100644
index 0000000000..2e03d0bd8e
--- /dev/null
+++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json
@@ -0,0 +1 @@
+{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute":"attribute-value"}]}
\ No newline at end of file
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;
+
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 {
+ 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);
}
static int
@@ -353,15 +378,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) \
do { \
- struct startTestInfo info = { virt_type, create, filename }; \
- DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \
+ struct startTestInfo info = { virt_type, create, filename, command }; \
+ DO_TEST_FULL(desc, testMdevctlStartHelper, info); \
} \
while (0)
#define DO_TEST_START(filename) \
- DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename)
+ DO_TEST_START_FULL("mdevctl start " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_START)
+
+#define DO_TEST_DEFINE(filename) \
+ DO_TEST_START_FULL("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE)
#define DO_TEST_STOP(uuid) \
DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid)
@@ -390,6 +418,10 @@ mymain(void)
DO_TEST_PARSE_JSON("mdevctl-list-multiple");
DO_TEST_PARSE_JSON("mdevctl-list-multiple-parents");
+ DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
+ DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
+ DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
+
done:
nodedevTestDriverFree(driver);
--
2.26.2
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>, ...)
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>
> ---
...
> +virNodeDevicePtr
> +nodeDeviceDefineXML(virConnectPtr conn,
> + const char *xmlDesc,
> + unsigned int flags)
> +{
> + g_autoptr(virNodeDeviceDef) def = NULL;
> + virNodeDevicePtr device = NULL;
> + const char *virt_type = NULL;
> +
> + virCheckFlags(0, NULL);
> +
> + if (nodeDeviceWaitInit() < 0)
> + return NULL;
> +
> + virt_type = virConnectGetType(conn);
> +
> + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
> + return NULL;
> +
> + if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0)
> + return NULL;
> +
> + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
> + g_autofree char *uuid = NULL;
> +
> + if (!def->parent) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("cannot define a mediated device without a parent"));
> + return NULL;
> + }
> +
> + if (virMdevctlDefine(def, &uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to define mediated device"));
> + return NULL;
> + }
> +
> + def->caps->data.mdev.uuid = g_strdup(uuid);
> + mdevGenerateDeviceName(def);
> + device = nodeDeviceFindNewMediatedDevice(conn, uuid);
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Unsupported device type"));
Per our coding style guideline, can we flip this condition and keep the shorter
block first?
Erik
© 2016 - 2026 Red Hat, Inc.