When an mdevctl command fails, there is not much information available to the user about why it failed. This is partly because we were not making use of the error message that mdevctl itself prints upon failure. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 46 +++++++++++++++++----------- src/node_device/node_device_driver.h | 6 ++-- tests/nodedevmdevctltest.c | 6 ++-- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 543e5bb90a..3851a3568f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -700,7 +700,8 @@ nodeDeviceFindAddressByName(const char *name) virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -725,15 +726,17 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, virCommandSetInputBuffer(cmd, json); virCommandSetOutputBuffer(cmd, uuid_out); + virCommandSetErrorBuffer(cmd, errmsg); return cmd; } static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid, + errmsg); if (!cmd) return -1; @@ -754,6 +757,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) { g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; if (!def->parent) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -761,9 +765,10 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; } - if (virMdevctlStart(def, &uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to start mediated device")); + if (virMdevctlStart(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown Error"); return NULL; } @@ -828,23 +833,25 @@ nodeDeviceCreateXML(virConnectPtr conn, virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid) -{ - return virCommandNewArgList(MDEVCTL, - "stop", - "-u", - uuid, - NULL); +nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "stop", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; } static int -virMdevctlStop(virNodeDeviceDefPtr def) +virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { int status; g_autoptr(virCommand) cmd = NULL; - cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid); + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg); if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -901,9 +908,12 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { - if (virMdevctlStop(def) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to stop mediated device")); + g_autofree char *errmsg = NULL; + + if (virMdevctlStop(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown error"); goto cleanup; } ret = 0; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 2113d2b0a5..4a40aa51f6 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -115,6 +115,8 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out); + char **uuid_out, + char **errmsg); virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid); +nodeDeviceGetMdevctlStopCommand(const char *uuid, + char **errmsg); diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 1bad65549b..c12feaac55 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -58,6 +58,7 @@ testMdevctlStart(const char *virt_type, const char *actualCmdline = NULL; int ret = -1; g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; g_autofree char *stdinbuf = NULL; g_autoptr(virCommand) cmd = NULL; @@ -66,7 +67,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 = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -117,11 +118,12 @@ testMdevctlStop(const void *data) const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL; g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid); + cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); if (!cmd) goto cleanup; -- 2.26.3

The macro should not have a trailing semicolon so that when the macro is used, the user can add a semicolon themselves. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tests/nodedevmdevctltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index c12feaac55..650e46a29f 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -273,7 +273,7 @@ mymain(void) struct startTestInfo info = { virt_type, create, filename }; \ DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ } \ - while (0); + while (0) #define DO_TEST_START(filename) \ DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) -- 2.26.3

Add two flag values for virConnectListAllNodeDevices() so that we can list only node devices that are active or inactive. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- include/libvirt/libvirt-nodedev.h | 9 +++-- src/conf/node_device_conf.h | 8 ++++ src/conf/virnodedeviceobj.c | 57 +++++++++++++++++----------- src/libvirt-nodedev.c | 2 + src/node_device/node_device_driver.c | 2 +- 5 files changed, 51 insertions(+), 27 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index eab8abf6ab..1a0e60b81f 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -61,10 +61,9 @@ int virNodeListDevices (virConnectPtr conn, * virConnectListAllNodeDevices: * * Flags used to filter the returned node devices. Flags in each group - * are exclusive. Currently only one group to filter the devices by cap - * type. - */ + * are exclusive. */ typedef enum { + /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV = 1 << 1, /* PCI device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV = 1 << 2, /* USB device */ @@ -86,6 +85,10 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD = 1 << 18, /* s390 AP Card device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE = 1 << 19, /* s390 AP Queue */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP Matrix */ + + /* filter the devices by active state */ + VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive devices */ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE = 1U << 31, /* Active devices */ } virConnectListAllNodeDeviceFlags; int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index c67b8e2aeb..3d7872fd6e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -422,6 +422,14 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX) +#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE \ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE |\ + VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE + +#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL \ + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP | \ + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE + int virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index f0ceb61a19..ab663fd5a0 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -848,8 +848,10 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, } -#define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ - virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG)) +#define MATCH_CAP(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ + virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG)) +#define MATCH(FLAG) (flags & (FLAG)) + static bool virNodeDeviceObjMatch(virNodeDeviceObjPtr obj, unsigned int flags) @@ -861,33 +863,42 @@ virNodeDeviceObjMatch(virNodeDeviceObjPtr obj, /* filter by cap type */ if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) { - if (!(MATCH(SYSTEM) || - MATCH(PCI_DEV) || - MATCH(USB_DEV) || - MATCH(USB_INTERFACE) || - MATCH(NET) || - MATCH(SCSI_HOST) || - MATCH(SCSI_TARGET) || - MATCH(SCSI) || - MATCH(STORAGE) || - MATCH(FC_HOST) || - MATCH(VPORTS) || - MATCH(SCSI_GENERIC) || - MATCH(DRM) || - MATCH(MDEV_TYPES) || - MATCH(MDEV) || - MATCH(CCW_DEV) || - MATCH(CSS_DEV) || - MATCH(VDPA) || - MATCH(AP_CARD) || - MATCH(AP_QUEUE) || - MATCH(AP_MATRIX))) + if (!(MATCH_CAP(SYSTEM) || + MATCH_CAP(PCI_DEV) || + MATCH_CAP(USB_DEV) || + MATCH_CAP(USB_INTERFACE) || + MATCH_CAP(NET) || + MATCH_CAP(SCSI_HOST) || + MATCH_CAP(SCSI_TARGET) || + MATCH_CAP(SCSI) || + MATCH_CAP(STORAGE) || + MATCH_CAP(FC_HOST) || + MATCH_CAP(VPORTS) || + MATCH_CAP(SCSI_GENERIC) || + MATCH_CAP(DRM) || + MATCH_CAP(MDEV_TYPES) || + MATCH_CAP(MDEV) || + MATCH_CAP(CCW_DEV) || + MATCH_CAP(CSS_DEV) || + MATCH_CAP(VDPA) || + MATCH_CAP(AP_CARD) || + MATCH_CAP(AP_QUEUE) || + MATCH_CAP(AP_MATRIX))) + return false; + } + + if (flags & (VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE)) { + if (!((MATCH(VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE) && + virNodeDeviceObjIsActive(obj)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE) && + !virNodeDeviceObjIsActive(obj)))) return false; } return true; } #undef MATCH +#undef MATCH_CAP typedef struct _virNodeDeviceObjListExportData virNodeDeviceObjListExportData; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index eb8c735a8c..375b907852 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -105,6 +105,8 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX + * VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE + * VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 3851a3568f..920fd815f2 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -217,7 +217,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, unsigned int flags) { - virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); + virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL, -1); if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; -- 2.26.3

It doesn't make sense to list all of the flag values in the function documentation. This is unnecessary duplication, we already refer to the enum type. Also, remove reference to exclusive groups of flags, since that does not apply to this API. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- include/libvirt/libvirt-nodedev.h | 3 +-- src/libvirt-nodedev.c | 30 +----------------------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 1a0e60b81f..2deead0791 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -60,8 +60,7 @@ int virNodeListDevices (virConnectPtr conn, /* * virConnectListAllNodeDevices: * - * Flags used to filter the returned node devices. Flags in each group - * are exclusive. */ + * Flags used to filter the returned node devices. */ typedef enum { /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 375b907852..fb707b570f 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -78,35 +78,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * objects. * * Normally, all node devices are returned; however, @flags can be used to - * filter the results for a smaller list of targeted node devices. The valid - * flags are divided into groups, where each group contains bits that - * describe mutually exclusive attributes of a node device, and where all bits - * within a group describe all possible node devices. - * - * Only one group of the @flags is provided to filter the node devices by - * capability type, flags include: - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX - * VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE - * VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE + * filter the results for a smaller list of targeted node devices. * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to -- 2.26.3

Expose a helper function that can be used by udev and mdevctl to generate device names for node devices. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 25 +++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++++ src/node_device/node_device_udev.c | 19 +++---------------- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 920fd815f2..bada5bbf00 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -978,3 +978,28 @@ nodedevRegister(void) return udevNodeRegister(); #endif } + + +void +nodeDeviceGenerateName(virNodeDeviceDef *def, + const char *subsystem, + const char *sysname, + const char *s) +{ + size_t i; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s_%s", + subsystem, + sysname); + + if (s != NULL) + virBufferAsprintf(&buf, "_%s", s); + + def->name = virBufferContentAndReset(&buf); + + for (i = 0; i < strlen(def->name); i++) { + if (!(g_ascii_isalnum(*(def->name + i)))) + *(def->name + i) = '_'; + } +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 4a40aa51f6..76a40c05ad 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -120,3 +120,9 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); + +void +nodeDeviceGenerateName(virNodeDeviceDef *def, + const char *subsystem, + const char *sysname, + const char *s); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 373f36c41f..eae301cc95 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -311,22 +311,9 @@ udevGenerateDeviceName(struct udev_device *device, virNodeDeviceDefPtr def, const char *s) { - size_t i; - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - virBufferAsprintf(&buf, "%s_%s", - udev_device_get_subsystem(device), - udev_device_get_sysname(device)); - - if (s != NULL) - virBufferAsprintf(&buf, "_%s", s); - - def->name = virBufferContentAndReset(&buf); - - for (i = 0; i < strlen(def->name); i++) { - if (!(g_ascii_isalnum(*(def->name + i)))) - *(def->name + i) = '_'; - } + nodeDeviceGenerateName(def, + udev_device_get_subsystem(device), + udev_device_get_sysname(device), s); return 0; } -- 2.26.3

This function will parse the list of mediated devices that are returned by mdevctl and convert it into our internal node device representation. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 143 ++++++++++++++++++ src/node_device/node_device_driver.h | 4 + .../mdevctl-list-multiple.json | 59 ++++++++ .../mdevctl-list-multiple.out.xml | 39 +++++ tests/nodedevmdevctltest.c | 56 ++++++- 5 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index bada5bbf00..a646692870 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,149 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +static void mdevGenerateDeviceName(virNodeDeviceDef *dev) +{ + nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); +} + + +static virNodeDeviceDef* +nodeDeviceParseMdevctlChildDevice(const char *parent, + virJSONValue *json) +{ + virNodeDevCapMdev *mdev; + const char *uuid; + virJSONValue *props; + virJSONValue *attrs; + g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); + + /* the child object should have a single key equal to its uuid. + * The value is an object describing the properties of the mdev */ + if (virJSONValueObjectKeysNumber(json) != 1) + return NULL; + + uuid = virJSONValueObjectGetKey(json, 0); + props = virJSONValueObjectGetValue(json, 0); + + child->parent = g_strdup(parent); + child->caps = g_new0(virNodeDevCapsDef, 1); + child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; + + mdev = &child->caps->data.mdev; + mdev->uuid = g_strdup(uuid); + mdev->type = + g_strdup(virJSONValueObjectGetString(props, "mdev_type")); + + attrs = virJSONValueObjectGet(props, "attrs"); + + if (attrs && virJSONValueIsArray(attrs)) { + size_t i; + int nattrs = virJSONValueArraySize(attrs); + + mdev->attributes = g_new0(virMediatedDeviceAttr*, nattrs); + mdev->nattributes = nattrs; + + for (i = 0; i < nattrs; i++) { + virJSONValue *attr = virJSONValueArrayGet(attrs, i); + virMediatedDeviceAttr *attribute; + virJSONValue *value; + + if (!virJSONValueIsObject(attr) || + virJSONValueObjectKeysNumber(attr) != 1) + return NULL; + + attribute = g_new0(virMediatedDeviceAttr, 1); + attribute->name = g_strdup(virJSONValueObjectGetKey(attr, 0)); + value = virJSONValueObjectGetValue(attr, 0); + attribute->value = g_strdup(virJSONValueGetString(value)); + mdev->attributes[i] = attribute; + } + } + mdevGenerateDeviceName(child); + + return g_steal_pointer(&child); +} + + +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDef ***devs) +{ + int n; + g_autoptr(virJSONValue) json_devicelist = NULL; + virNodeDeviceDef **outdevs = NULL; + size_t noutdevs = 0; + size_t i; + size_t j; + + json_devicelist = virJSONValueFromString(jsonstring); + + if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("mdevctl JSON response contains no devices")); + goto error; + } + + n = virJSONValueArraySize(json_devicelist); + + for (i = 0; i < n; i++) { + virJSONValue *obj = virJSONValueArrayGet(json_devicelist, i); + const char *parent; + virJSONValue *child_array; + int nchildren; + + if (!virJSONValueIsObject(obj)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device is not an object")); + goto error; + } + + /* mdevctl returns an array of objects. Each object is a parent device + * object containing a single key-value pair which maps from the name + * of the parent device to an array of child devices */ + if (virJSONValueObjectKeysNumber(obj) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected format for parent device object")); + goto error; + } + + parent = virJSONValueObjectGetKey(obj, 0); + child_array = virJSONValueObjectGetValue(obj, 0); + + if (!virJSONValueIsArray(child_array)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device's JSON object data is not an array")); + goto error; + } + + nchildren = virJSONValueArraySize(child_array); + + for (j = 0; j < nchildren; j++) { + g_autoptr(virNodeDeviceDef) child = NULL; + virJSONValue *child_obj = virJSONValueArrayGet(child_array, j); + + if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to parse child device")); + goto error; + } + + if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) + goto error; + } + } + + *devs = outdevs; + return noutdevs; + + error: + for (i = 0; i < noutdevs; i++) + virNodeDeviceDefFree(outdevs[i]); + VIR_FREE(outdevs); + return -1; +} + + int nodeDeviceDestroy(virNodeDevicePtr device) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 76a40c05ad..f1c93f4b21 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -121,6 +121,10 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDef ***devs); + void nodeDeviceGenerateName(virNodeDeviceDef *def, const char *subsystem, diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.json b/tests/nodedevmdevctldata/mdevctl-list-multiple.json new file mode 100644 index 0000000000..eefcd90c62 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.json @@ -0,0 +1,59 @@ +[ + { + "0000:00:02.0": [ + { + "200f228a-c80a-4d50-bfb7-f5a0e4e34045": { + "mdev_type": "i915-GVTg_V5_4", + "start": "manual" + } + }, + { + "de807ffc-1923-4d5f-b6c9-b20ecebc6d4b": { + "mdev_type": "i915-GVTg_V5_4", + "start": "auto" + } + }, + { + "435722ea-5f43-468a-874f-da34f1217f13": { + "mdev_type": "i915-GVTg_V5_8", + "start": "manual", + "attrs": [ + { + "testattr": "42" + } + ] + } + } + ] + }, + { + "matrix": [ + { "783e6dbb-ea0e-411f-94e2-717eaad438bf": { + "mdev_type": "vfio_ap-passthrough", + "start": "manual", + "attrs": [ + { + "assign_adapter": "5" + }, + { + "assign_adapter": "6" + }, + { + "assign_domain": "0xab" + }, + { + "assign_control_domain": "0xab" + }, + { + "assign_domain": "4" + }, + { + "assign_control_domain": "4" + } + ] + } + } + ] + } +] + diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml new file mode 100644 index 0000000000..543ad916b7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -0,0 +1,39 @@ +<device> + <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + <attr name='testattr' value='42'/> + </capability> +</device> +<device> + <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> + <parent>matrix</parent> + <capability type='mdev'> + <type id='vfio_ap-passthrough'/> + <iommuGroup number='0'/> + <attr name='assign_adapter' value='5'/> + <attr name='assign_adapter' value='6'/> + <attr name='assign_domain' value='0xab'/> + <attr name='assign_control_domain' value='0xab'/> + <attr name='assign_domain' value='4'/> + <attr name='assign_control_domain' value='4'/> + </capability> +</device> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 650e46a29f..6c92d839d1 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -145,6 +145,53 @@ testMdevctlStop(const void *data) return ret; } + +static int +testMdevctlParse(const void *data) +{ + g_autofree char *buf = NULL; + const char *filename = data; + g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s.json", + abs_srcdir, filename); + g_autofree char *xmloutfile = g_strdup_printf("%s/nodedevmdevctldata/%s.out.xml", + abs_srcdir, filename); + int ret = -1; + int nmdevs = 0; + virNodeDeviceDef **mdevs = NULL; + virBuffer xmloutbuf = VIR_BUFFER_INITIALIZER; + size_t i; + + if (virFileReadAll(jsonfile, 1024*1024, &buf) < 0) { + VIR_TEST_DEBUG("Unable to read file %s", jsonfile); + return -1; + } + + if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) { + VIR_TEST_DEBUG("Unable to parse json for %s", filename); + return -1; + } + + for (i = 0; i < nmdevs; i++) { + g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]); + if (!devxml) + goto cleanup; + virBufferAddStr(&xmloutbuf, devxml); + } + + if (nodedevCompareToFile(virBufferCurrentContent(&xmloutbuf), xmloutfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&xmloutbuf); + for (i = 0; i < nmdevs; i++) + virNodeDeviceDefFree(mdevs[i]); + g_free(mdevs); + + return ret; +} + static void nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) { @@ -265,13 +312,13 @@ mymain(void) } #define DO_TEST_FULL(desc, func, info) \ - if (virTestRun(desc, func, &info) < 0) \ + if (virTestRun(desc, func, info) < 0) \ ret = -1; #define DO_TEST_START_FULL(virt_type, create, filename) \ do { \ struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ + DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ } \ while (0) @@ -281,6 +328,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) +#define DO_TEST_PARSE_JSON(filename) \ + DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) + /* Test mdevctl start commands */ DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); DO_TEST_START("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); @@ -289,6 +339,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576"); + DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + done: nodedevTestDriverFree(driver); -- 2.26.3

This adds an internal API to query for persistent mediated devices that are defined by mdevctl. Upcoming commits will make use of this information. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 18 +++++++++ src/node_device/node_device_driver.h | 3 ++ .../mdevctl-list-defined.argv | 1 + tests/nodedevmdevctltest.c | 39 +++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a646692870..6911fa36da 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,24 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +virCommand* +nodeDeviceGetMdevctlListCommand(bool defined, + char **output) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "list", + "--dumpjson", + NULL); + + if (defined) + virCommandAddArg(cmd, "--defined"); + + virCommandSetOutputBuffer(cmd, output); + + return cmd; +} + + static void mdevGenerateDeviceName(virNodeDeviceDef *dev) { nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f1c93f4b21..705d83f827 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -121,6 +121,9 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +virCommandPtr +nodeDeviceGetMdevctlListCommand(bool defined, char **output); + int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDef ***devs); diff --git a/tests/nodedevmdevctldata/mdevctl-list-defined.argv b/tests/nodedevmdevctldata/mdevctl-list-defined.argv new file mode 100644 index 0000000000..72b5906e9e --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-defined.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ list --dumpjson --defined diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 6c92d839d1..1ab4776a23 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -145,6 +145,40 @@ testMdevctlStop(const void *data) return ret; } +static int +testMdevctlListDefined(const void *data G_GNUC_UNUSED) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *output = NULL; + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-list-defined.argv", + abs_srcdir); + + cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (!cmd) + goto cleanup; + + virCommandSetDryRun(&buf, NULL, NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (nodedevCompareToFile(actualCmdline, cmdlinefile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} static int testMdevctlParse(const void *data) @@ -328,6 +362,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) +#define DO_TEST_LIST_DEFINED() \ + DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) + #define DO_TEST_PARSE_JSON(filename) \ DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) @@ -339,6 +376,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576"); + DO_TEST_LIST_DEFINED(); + DO_TEST_PARSE_JSON("mdevctl-list-multiple"); done: -- 2.26.3

Consistent with other objects (e.g. virDomainObj), add a field to indicate whether the node device is persistent or transient. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/virnodedeviceobj.c | 16 ++++++++++++++++ src/conf/virnodedeviceobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 24 insertions(+) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ab663fd5a0..ce84e4d8c1 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -40,6 +40,7 @@ struct _virNodeDeviceObj { bool skipUpdateCaps; /* whether to skip checking host caps, used by testdriver */ bool active; + bool persistent; }; struct _virNodeDeviceObjList { @@ -1003,3 +1004,18 @@ virNodeDeviceObjSetActive(virNodeDeviceObj *obj, { obj->active = active; } + + +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj) +{ + return obj->persistent; +} + + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, + bool persistent) +{ + obj->persistent = persistent; +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index e786a70f51..7f682b9dca 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -128,3 +128,9 @@ virNodeDeviceObjIsActive(virNodeDeviceObj *obj); void virNodeDeviceObjSetActive(virNodeDeviceObj *obj, bool active); +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, + bool persistent); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 09957c943b..047314ec19 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1273,6 +1273,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; @@ -1286,6 +1287,7 @@ virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjSetActive; +virNodeDeviceObjSetPersistent; # conf/virnwfilterbindingdef.h -- 2.26.3

Since a mediated device can be persistently defined by the mdevctl backend, we need additional lifecycle events beyond CREATED/DELETED to indicate that e.g. the device has been stopped but the device definition still exists. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- examples/c/misc/event-test.c | 4 ++++ include/libvirt/libvirt-nodedev.h | 2 ++ tools/virsh-nodedev.c | 4 +++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c index 76d4f3f6e8..10c707e66b 100644 --- a/examples/c/misc/event-test.c +++ b/examples/c/misc/event-test.c @@ -381,6 +381,10 @@ nodeDeviceEventToString(int event) return "Created"; case VIR_NODE_DEVICE_EVENT_DELETED: return "Deleted"; + case VIR_NODE_DEVICE_EVENT_DEFINED: + return "Defined"; + case VIR_NODE_DEVICE_EVENT_UNDEFINED: + return "Undefined"; case VIR_NODE_DEVICE_EVENT_LAST: break; } diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 2deead0791..77d814935e 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -196,6 +196,8 @@ int virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, typedef enum { VIR_NODE_DEVICE_EVENT_CREATED = 0, VIR_NODE_DEVICE_EVENT_DELETED = 1, + VIR_NODE_DEVICE_EVENT_DEFINED = 2, + VIR_NODE_DEVICE_EVENT_UNDEFINED = 3, # ifdef VIR_ENUM_SENTINELS VIR_NODE_DEVICE_EVENT_LAST diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index fedc8497f8..b9fe9b8be1 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -775,7 +775,9 @@ VIR_ENUM_DECL(virshNodeDeviceEvent); VIR_ENUM_IMPL(virshNodeDeviceEvent, VIR_NODE_DEVICE_EVENT_LAST, N_("Created"), - N_("Deleted")); + N_("Deleted"), + N_("Defined"), + N_("Undefined")); static const char * virshNodeDeviceEventToString(int event) -- 2.26.3

At startup, query devices that are defined by 'mdevctl' and add them to the node device list. This adds a complication: we now have two potential sources of information for a node device: - udev for all devices and for activated mediated devices - mdevctl for persistent mediated devices Unfortunately, neither backend returns full information for a mediated device. For example, if a persistent mediated device in the list (with information provided from mdevctl) is 'started', that same device will now be detected by udev. If we simply overwrite the existing device definition with the new one provided by the udev backend, we will lose extra information that was provided by mdevctl (e.g. attributes, etc). To avoid this, make sure to copy the extra information into the new device definition. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 153 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++ src/node_device/node_device_udev.c | 31 +++++- 3 files changed, 185 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 6911fa36da..efa524f317 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1164,3 +1164,156 @@ nodeDeviceGenerateName(virNodeDeviceDef *def, *(def->name + i) = '_'; } } + + +static int +virMdevctlListDefined(virNodeDeviceDef ***devs) +{ + int status; + g_autofree char *output = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + if (!output) + return -1; + + return nodeDeviceParseMdevctlJSON(output, devs); +} + + +int +nodeDeviceUpdateMediatedDevices(void) +{ + g_autofree virNodeDeviceDef **defs = NULL; + int ndefs; + size_t i; + + if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + virReportSystemError(errno, "%s", + _("failed to query mdevs from mdevctl")); + return -1; + } + + for (i = 0; i < ndefs; i++) { + virNodeDeviceObj *obj; + virObjectEvent *event; + g_autoptr(virNodeDeviceDef) def = defs[i]; + g_autofree char *name = g_strdup(def->name); + bool defined = false; + + def->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + virNodeDeviceDef *d = g_steal_pointer(&def); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, def); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + continue; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + } + + return 0; +} + + +/* returns true if any attributes were copied, else returns false */ +static bool +virMediatedDeviceAttrsCopy(virNodeDevCapMdev *dst, + virNodeDevCapMdev *src) +{ + bool ret = false; + size_t i; + + if (src->nattributes != dst->nattributes) { + ret = true; + for (i = 0; i < dst->nattributes; i++) + virMediatedDeviceAttrFree(dst->attributes[i]); + g_free(dst->attributes); + + dst->nattributes = src->nattributes; + dst->attributes = g_new0(virMediatedDeviceAttr*, + src->nattributes); + for (i = 0; i < dst->nattributes; i++) + dst->attributes[i] = virMediatedDeviceAttrNew(); + } + + for (i = 0; i < src->nattributes; i++) { + if (STRNEQ_NULLABLE(src->attributes[i]->name, + dst->attributes[i]->name)) { + ret = true; + g_free(dst->attributes[i]->name); + dst->attributes[i]->name = + g_strdup(src->attributes[i]->name); + } + if (STRNEQ_NULLABLE(src->attributes[i]->value, + dst->attributes[i]->value)) { + ret = true; + g_free(dst->attributes[i]->value); + dst->attributes[i]->value = + g_strdup(src->attributes[i]->value); + } + } + + return ret; +} + + +/* A mediated device definitions from mdevctl contains additional info that is + * not available from udev. Transfer this data to the new definition. + * Returns true if anything was copied, else returns false */ +bool +nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, + virNodeDeviceDef *src) +{ + bool ret = false; + virNodeDevCapMdev *srcmdev = &src->caps->data.mdev; + virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev; + + if (STRNEQ_NULLABLE(dstmdev->type, srcmdev->type)) { + ret = true; + g_free(dstmdev->type); + dstmdev->type = g_strdup(srcmdev->type); + } + + if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) { + ret = true; + g_free(dstmdev->uuid); + dstmdev->uuid = g_strdup(srcmdev->uuid); + } + + if (virMediatedDeviceAttrsCopy(dstmdev, srcmdev)) + ret = true; + + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 705d83f827..9b54efe4e3 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -128,8 +128,14 @@ int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDef ***devs); +int +nodeDeviceUpdateMediatedDevices(void); + void nodeDeviceGenerateName(virNodeDeviceDef *def, const char *subsystem, const char *sysname, const char *s); + +bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, + virNodeDeviceDef *src); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index eae301cc95..38ebe7b5c5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1421,9 +1421,17 @@ udevRemoveOneDeviceSysPath(const char *path) VIR_NODE_DEVICE_EVENT_DELETED, 0); - VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + /* If the device is a mediated device that has been 'stopped', it may still + * be defined by mdevctl and can therefore be started again. Don't drop it + * from the list of node devices */ + if (virNodeDeviceObjIsPersistent(obj)) { + VIR_FREE(def->sysfs_path); + virNodeDeviceObjSetActive(obj, false); + } else { + VIR_DEBUG("Removing device '%s' with sysfs path '%s'", + def->name, path); + virNodeDeviceObjListRemove(driver->devs, obj); + } virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1481,7 +1489,6 @@ udevSetParent(struct udev_device *device, return 0; } - static int udevAddOneDevice(struct udev_device *device) { @@ -1491,6 +1498,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEventPtr event = NULL; bool new_device = true; int ret = -1; + bool was_persistent = false; def = g_new0(virNodeDeviceDef, 1); @@ -1514,14 +1522,23 @@ udevAddOneDevice(struct udev_device *device) goto cleanup; if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + objdef = virNodeDeviceObjGetDef(obj); + + if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + nodeDeviceDefCopyFromMdevctl(def, objdef); + was_persistent = virNodeDeviceObjIsPersistent(obj); + /* If the device was defined by mdevctl and was never instantiated, it + * won't have a sysfs path. We need to emit a CREATED event... */ + new_device = (objdef->sysfs_path == NULL); + virNodeDeviceObjEndAPI(&obj); - new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + virNodeDeviceObjSetPersistent(obj, was_persistent); objdef = virNodeDeviceObjGetDef(obj); if (new_device) @@ -1941,6 +1958,10 @@ nodeStateInitializeEnumerate(void *opaque) /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) goto error; + /* Load persistent mdevs (which might not be activated yet) and additional + * information about active mediated devices from mdevctl */ + if (nodeDeviceUpdateMediatedDevices() != 0) + goto error; nodeDeviceLock(); driver->initialized = true; -- 2.26.3

When a mediated device is stopped or undefined by an application outside of libvirt, we need to remove it from our list of node devices within libvirt. This patch introduces virNodeDeviceObjListRemoveLocked() and virNodeDeviceObjListForEachRemove() (which are analogous to other types of object lists in libvirt) to facilitate that. They will be used in coming commits. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/conf/virnodedeviceobj.c | 58 ++++++++++++++++++++++++++++++++++--- src/conf/virnodedeviceobj.h | 11 +++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ce84e4d8c1..97e7d7ab11 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -507,23 +507,29 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - virNodeDeviceDefPtr def; - if (!obj) return; - def = obj->def; virObjectRef(obj); virObjectUnlock(obj); virObjectRWLockWrite(devs); virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); + virNodeDeviceObjListRemoveLocked(devs, obj); virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(devs); } +/* The caller must hold lock on 'devs' */ +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjList *devs, + virNodeDeviceObj *dev) +{ + virHashRemoveEntry(devs->objs, dev->def->name); +} + + /* * Return the NPIV dev's parent device name */ @@ -1019,3 +1025,47 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, { obj->persistent = persistent; } + + +struct virNodeDeviceObjListRemoveData +{ + virNodeDeviceObjListRemoveIter iter; + void *opaque; +}; + +static int virNodeDeviceObjListRemoveCb(void *key G_GNUC_UNUSED, + void *value, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData *data = opaque; + + return data->iter(value, data->opaque); +} + + +/** + * virNodeDeviceObjListForEachRemove + * @devs: Pointer to object list + * @iter: function to call for each device object + * @opaque: Opaque data to use as argument to helper + * + * For each object in @devs, call the @iter helper using @opaque as + * an argument. If @iter returns true, that item will be removed from the + * object list. + */ +void +virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, + virNodeDeviceObjListRemoveIter iter, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData data = { + .iter = iter, + .opaque = opaque + }; + + virObjectRWLockWrite(devs); + g_hash_table_foreach_remove(devs->objs, + virNodeDeviceObjListRemoveCb, + &data); + virObjectRWUnlock(devs); +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7f682b9dca..249115cf68 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -80,6 +80,10 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr dev); +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjList *devs, + virNodeDeviceObj *dev); + int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); @@ -134,3 +138,10 @@ virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); + +typedef bool (*virNodeDeviceObjListRemoveIter)(virNodeDeviceObj *obj, + const void *opaque); + +void virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, + virNodeDeviceObjListRemoveIter iter, + void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 047314ec19..f36400b5f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1280,12 +1280,14 @@ virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFindMediatedDeviceByUUID; virNodeDeviceObjListFindSCSIHostByWWNs; +virNodeDeviceObjListForEachRemove; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; virNodeDeviceObjSetPersistent; -- 2.26.3

mdevctl does not currently provide any events when the list of defined devices changes, so we will need to poll mdevctl for the list of defined devices periodically. When a mediated device no longer exists from one iteration to the next, we need to treat it as an "undefine" event. When we get such an event, we remove the device from the list if it's not active. Otherwise, we simply mark it as non-persistent. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 67 ++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index efa524f317..4be650ddef 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1183,20 +1183,81 @@ virMdevctlListDefined(virNodeDeviceDef ***devs) } +typedef struct _virMdevctlForEachData virMdevctlForEachData; +struct _virMdevctlForEachData { + int ndefs; + virNodeDeviceDef **defs; +}; + + +/* This function keeps the list of persistent mediated devices consistent + * between the nodedev driver and mdevctl. + * @obj is a device that is currently known by the nodedev driver, and @opaque + * contains the most recent list of devices defined by mdevctl. If @obj is no + * longer defined in mdevctl, mark it as undefined and possibly remove it from + * the driver as well. Returning 'true' from this function indicates that the + * device should be removed from the nodedev driver list. */ +static bool +removeMissingPersistentMdevs(virNodeDeviceObj *obj, + const void *opaque) +{ + bool remove = false; + const virMdevctlForEachData *data = opaque; + size_t i; + virNodeDeviceDef *def = virNodeDeviceObjGetDef(obj); + virObjectEvent *event; + + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return false; + + /* transient mdevs are populated via udev, so don't remove them from the + * nodedev driver just because they are not reported by by mdevctl */ + if (!virNodeDeviceObjIsPersistent(obj)) + return false; + + for (i = 0; i < data->ndefs; i++) { + /* OK, this mdev is still defined by mdevctl */ + if (STREQ(data->defs[i]->name, def->name)) + return false; + } + + event = virNodeDeviceEventLifecycleNew(def->name, + VIR_NODE_DEVICE_EVENT_UNDEFINED, + 0); + + /* The device is active, but no longer defined by mdevctl. Keep the device + * in the list, but mark it as non-persistent */ + if (virNodeDeviceObjIsActive(obj)) + virNodeDeviceObjSetPersistent(obj, false); + else + remove = true; + + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return remove; +} + + int nodeDeviceUpdateMediatedDevices(void) { g_autofree virNodeDeviceDef **defs = NULL; - int ndefs; + virMdevctlForEachData data = { 0, }; size_t i; - if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + if ((data.ndefs = virMdevctlListDefined(&defs)) < 0) { virReportSystemError(errno, "%s", _("failed to query mdevs from mdevctl")); return -1; } - for (i = 0; i < ndefs; i++) { + /* Any mdevs that were previously defined but were not returned in the + * latest mdevctl query should be removed from the device list */ + data.defs = defs; + virNodeDeviceObjListForEachRemove(driver->devs, + removeMissingPersistentMdevs, &data); + + for (i = 0; i < data.ndefs; i++) { virNodeDeviceObj *obj; virObjectEvent *event; g_autoptr(virNodeDeviceDef) def = defs[i]; -- 2.26.3

We need to query mdevctl for changes to device definitions since an administrator can define new devices by executing mdevctl outside of libvirt. In the future, mdevctl may add a way to signal device add/remove via events, but for now we resort to a bit of a workaround: monitoring the mdevctl config directory for changes to files. When a change is detected, we query mdevctl and update our device list. The mdevctl querying is handled in a throwaway thread, and these threads are synchronized with a mutex. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 161 +++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 38ebe7b5c5..78144b7762 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -19,6 +19,7 @@ */ #include <config.h> +#include <gio/gio.h> #include <libudev.h> #include <pciaccess.h> #include <scsi/scsi.h> @@ -69,6 +70,10 @@ struct _udevEventData { /* init thread */ virThread initThread; + + GList *mdevctlMonitors; + virMutex mdevctlLock; + int mdevctlTimeout; }; static virClassPtr udevEventDataClass; @@ -89,6 +94,11 @@ udevEventDataDispose(void *obj) udev_monitor_unref(priv->udev_monitor); udev_unref(udev); + virMutexLock(&priv->mdevctlLock); + g_list_free_full(priv->mdevctlMonitors, g_object_unref); + virMutexUnlock(&priv->mdevctlLock); + virMutexDestroy(&priv->mdevctlLock); + virCondDestroy(&priv->threadCond); } @@ -120,6 +130,11 @@ udevEventDataNew(void) return NULL; } + if (virMutexInit(&ret->mdevctlLock) < 0) { + virObjectUnref(ret); + return NULL; + } + ret->watch = -1; return ret; } @@ -2004,6 +2019,137 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) } +static void +mdevctlHandlerThread(void *opaque G_GNUC_UNUSED) +{ + udevEventData *priv = driver->privateData; + + /* ensure only a single thread can query mdevctl at a time */ + virMutexLock(&priv->mdevctlLock); + if (nodeDeviceUpdateMediatedDevices() < 0) + VIR_WARN("mdevctl failed to updated mediated devices"); + virMutexUnlock(&priv->mdevctlLock); +} + + +static void +scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque) +{ + udevEventData *priv = opaque; + virThread thread; + + if (priv->mdevctlTimeout > 0) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (virThreadCreateFull(&thread, false, mdevctlHandlerThread, + "mdevctl-thread", false, NULL) < 0) { + virReportSystemError(errno, "%s", + _("failed to create mdevctl thread")); + } +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data); + + +/* Recursively monitors a directory and its subdirectories for file changes and + * returns a GList of GFileMonitor objects */ +static GList* +monitorFileRecursively(udevEventData *udev, + GFile *file) +{ + GList *monitors = NULL; + g_autoptr(GError) error = NULL; + g_autoptr(GFileEnumerator) children = NULL; + GFileMonitor *mon; + + if (!(children = g_file_enumerate_children(file, "standard::*", + G_FILE_QUERY_INFO_NONE, NULL, &error))) + goto error; + + if (!(mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, &error))) + goto error; + + g_signal_connect(mon, "changed", + G_CALLBACK(mdevctlEventHandleCallback), udev); + + monitors = g_list_append(monitors, mon); + + while (true) { + GFileInfo *info = NULL; + GFile *child = NULL; + GList *child_monitors = NULL; + + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error)) + goto error; + + if (!info) + break; + + if (g_file_query_file_type(child, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) { + + child_monitors = monitorFileRecursively(udev, child); + if (child_monitors) + monitors = g_list_concat(monitors, child_monitors); + } + } + + return monitors; + + error: + g_list_free_full(monitors, g_object_unref); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to monitor directory: %s"), error->message); + g_clear_error(&error); + return NULL; +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data) +{ + udevEventData *priv = user_data; + /* if a new directory appears, monitor that directory for changes */ + if (event_type == G_FILE_MONITOR_EVENT_CREATED && + g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) { + GList *newmonitors = monitorFileRecursively(priv, file); + virMutexLock(&priv->mdevctlLock); + priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors); + virMutexUnlock(&priv->mdevctlLock); + } + + /* When mdevctl creates a device, it can result in multiple notify events + * emitted for a single logical change (e.g. several CHANGED events, or a + * CREATED and CHANGED event followed by CHANGES_DONE_HINT). To avoid + * spawning a mdevctl thread multiple times for a single logical + * configuration change, try to coalesce these changes by waiting for the + * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the + * signal if that event never comes */ + if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + if (priv->mdevctlTimeout > 0) + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler, + priv, NULL); + return; + } + + scheduleMdevctlHandler(-1, priv); +} + + static int nodeStateInitialize(bool privileged, const char *root, @@ -2012,6 +2158,7 @@ nodeStateInitialize(bool privileged, { udevEventDataPtr priv = NULL; struct udev *udev = NULL; + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2113,6 +2260,20 @@ nodeStateInitialize(bool privileged, if (priv->watch == -1) goto unlock; + /* mdevctl may add notification events in the future: + * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to + * monitoring the mdevctl configuration directory for changes. + * mdevctl configuration is stored in a directory tree within + * /etc/mdevctl.d/. There is a directory for each parent device, which + * contains a file defining each mediated device */ + virMutexLock(&priv->mdevctlLock); + if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, + mdevctlConfigDir))) { + virMutexUnlock(&priv->mdevctlLock); + goto cleanup; + } + virMutexUnlock(&priv->mdevctlLock); + virObjectUnlock(priv); /* Create a fictional 'computer' device to root the device tree. */ -- 2.26.3

Abstract out the function used to generate the commandline for 'mdevctl start' since they take the same arguments. Add tests to ensure that we're generating the command properly. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 34 +++++++-- src/node_device/node_device_driver.h | 6 ++ ...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 | 76 +++++++++++++------ 9 files changed, 95 insertions(+), 27 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/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 4be650ddef..abd45a6eab 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -698,10 +698,14 @@ nodeDeviceFindAddressByName(const char *name) } -virCommandPtr -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out, - char **errmsg) +/* 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 virCommand* +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def, + const char *subcommand, + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -719,7 +723,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; } - cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL); @@ -731,6 +735,26 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return cmd; } +virCommand* +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out, + errmsg); +} + +virCommand* +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out, + errmsg); +} + + + static int virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 9b54efe4e3..b319990f0f 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -117,6 +117,12 @@ virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, char **uuid_out, char **errmsg); + +virCommand* +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg); + virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); 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 1ab4776a23..99fa328d24 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 virCommand* (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char **); + + static int -testMdevctlStart(const char *virt_type, - int create, - const char *mdevxml, - const char *startcmdfile, - const char *startjsonfile) +testMdevctlStartOrDefine(const char *virt_type, + int create, + MdevctlCmdFunc mdevctl_cmd_func, + const char *mdevxml, + const char *cmdfile, + const char *jsonfile) { g_autoptr(virNodeDeviceDef) def = NULL; virNodeDeviceObjPtr obj = NULL; @@ -67,7 +78,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, &errmsg); + cmd = mdevctl_cmd_func(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -79,10 +90,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; @@ -94,20 +105,34 @@ testMdevctlStart(const char *virt_type, } static int -testMdevctlStartHelper(const void *data) +testMdevctlStartOrDefineHelper(const void *data) { const struct startTestInfo *info = data; + const char *cmd; + MdevctlCmdFunc 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 testMdevctlStartOrDefine(info->virt_type, info->create, func, + mdevxml, cmdlinefile, jsonfile); } static int @@ -349,15 +374,18 @@ mymain(void) if (virTestRun(desc, func, info) < 0) \ ret = -1; -#define DO_TEST_START_FULL(virt_type, create, filename) \ +#define DO_TEST_CMD(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, testMdevctlStartOrDefineHelper, &info); \ } \ while (0) #define DO_TEST_START(filename) \ - DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) + DO_TEST_CMD("mdevctl start " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_START) + +#define DO_TEST_DEFINE(filename) \ + DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) @@ -380,6 +408,10 @@ mymain(void) DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + 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.3

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&gt; --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 6 +++ src/libvirt-nodedev.c | 42 ++++++++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 71 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 5 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 8 ++++ src/rpc/gendispatch.pl | 1 + 11 files changed, 156 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 77d814935e..33eb46b3cd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,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..64a0a7c473 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev); +typedef virNodeDevice* +(*virDrvNodeDeviceDefineXML)(virConnect *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 fb707b570f..cfc0c9de5b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -737,6 +737,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 + */ +virNodeDevice* +virNodeDeviceDefineXML(virConnect *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) { + virNodeDevice *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 51a3d7265a..3d8176351c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -887,6 +887,7 @@ LIBVIRT_7.1.0 { LIBVIRT_7.2.0 { global: virDomainStartDirtyRateCalc; + virNodeDeviceDefineXML; } LIBVIRT_7.1.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 abd45a6eab..418faa9fb9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -776,6 +776,26 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) } +static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlDefineCommand(def, uuid, errmsg); + if (!cmd) + return -1; + + /* an auto-generated uuid is returned via stdout if no uuid is specified in + * the mdevctl args */ + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + /* remove newline */ + *uuid = g_strstrip(*uuid); + + return 0; +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) @@ -1112,6 +1132,57 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } +virNodeDevice* +nodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virNodeDevice *device = NULL; + const char *virt_type = NULL; + g_autofree char *uuid = NULL; + g_autofree char *errmsg = 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)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + return NULL; + } + + if (!def->parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot define a mediated device without a parent")); + return NULL; + } + + if (virMdevctlDefine(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to define mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + return NULL; + } + + def->caps->data.mdev.uuid = g_strdup(uuid); + mdevGenerateDeviceName(def); + device = nodeDeviceFindNewMediatedDevice(conn, uuid); + + 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 b319990f0f..f626e9ac8a 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -102,6 +102,11 @@ nodeDeviceCreateXML(virConnectPtr conn, int nodeDeviceDestroy(virNodeDevicePtr dev); +virNodeDevice* +nodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 78144b7762..9de658cab5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2321,6 +2321,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceListCaps = nodeDeviceListCaps, /* 0.7.3 */ .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ + .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62ada08344..15c592b5b5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8696,6 +8696,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 7fdc65f029..a95ed65f12 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2145,6 +2145,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: @@ -6745,5 +6754,11 @@ enum remote_procedure { * @generate: both * @acl: domain:read */ - REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427 + REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, + + /** + * @generate: both + * @acl: node_device:write + */ + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index d13dc81a82..3488659da1 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; }; @@ -3605,4 +3612,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, REMOTE_PROC_DOMAIN_GET_MESSAGES = 426, REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 9dcd8c3e89..9f5bf0e316 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"); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:12AM -0500, Jonathon Jongsma wrote: typedef virNodeDevice * Actually, I think we should keep the Ptr variant in the public APIs. I remember the conclusion to the Ptr discussion was to keep the typedef wrt to public headers and API entrypoints. This is a new API so it's a bit different, but I think for consistency reasons we ought to keep using Ptr for public API everywhere, even new APIs (this is the case for all the APIs you added). My R-b still stands, ^this is not a show-stopper. Erik

Now that we can filter active and inactive node devices in virConnectListAllNodeDevices(), add these switches to the virsh command. Eventual output (once everything is hooked up): virsh # nodedev-list --cap mdev mdev_bd2ea955_3402_4252_8c17_7468083a0f26 virsh # nodedev-list --inactive --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c virsh # nodedev-list --all --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c mdev_bd2ea955_3402_4252_8c17_7468083a0f26 Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/virsh-nodedev.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index b9fe9b8be1..7ab5b264fc 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -378,6 +378,14 @@ static const vshCmdOptDef opts_node_list_devices[] = { .completer = virshNodeDeviceCapabilityNameCompleter, .help = N_("capability names, separated by comma") }, + {.name = "inactive", + .type = VSH_OT_BOOL, + .help = N_("list inactive devices") + }, + {.name = "all", + .type = VSH_OT_BOOL, + .help = N_("list inactive & active devices") + }, {.name = NULL} }; @@ -393,18 +401,26 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) int ncaps = 0; virshNodeDeviceListPtr list = NULL; int cap_type = -1; + bool inactive = vshCommandOptBool(cmd, "inactive"); + bool all = vshCommandOptBool(cmd, "all"); ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str)); if (cap_str) { - if (tree) { - vshError(ctl, "%s", _("Options --tree and --cap are incompatible")); - return false; - } if ((ncaps = vshStringToArray(cap_str, &caps)) < 0) return false; } + if (all && inactive) { + vshError(ctl, "%s", _("Option --all is incompatible with --inactive")); + return false; + } + + if (tree && (cap_str || inactive || all)) { + vshError(ctl, "%s", _("Option --tree is incompatible with other options")); + return false; + } + for (i = 0; i < ncaps; i++) { if ((cap_type = virNodeDevCapTypeFromString(caps[i])) < 0) { vshError(ctl, "%s", _("Invalid capability type")); @@ -481,6 +497,11 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } } + if (inactive || all) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE; + if (!inactive) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE; + if (!(list = virshNodeDeviceListCollect(ctl, caps, ncaps, flags))) { ret = false; goto cleanup; -- 2.26.3

mdevctl 'stop' and 'undefine' commands take the same uuid parameter, so refactor the test infrastructure to share common implementation for both of these commands. The 'undefine' command will be introduced in a following patch. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- tests/nodedevmdevctltest.c | 48 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 99fa328d24..7cf9fa7c67 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -12,7 +12,9 @@ typedef enum { MDEVCTL_CMD_START, + MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, + MDEVCTL_CMD_UNDEFINE } MdevctlCmd; struct startTestInfo { @@ -135,20 +137,22 @@ testMdevctlStartOrDefineHelper(const void *data) mdevxml, cmdlinefile, jsonfile); } +typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf); +struct UuidCommandTestInfo { + const char *uuid; + MdevctlCmd command; +}; + static int -testMdevctlStop(const void *data) +testMdevctlUuidCommand(const char *uuid, GetStopUndefineCmdFunc func, const char *outfile) { - const char *uuid = data; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; g_autofree char *errmsg = NULL; - g_autofree char *cmdlinefile = - g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", - abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); + cmd = func(uuid, &errmsg); if (!cmd) goto cleanup; @@ -160,7 +164,7 @@ testMdevctlStop(const void *data) if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, cmdlinefile) < 0) + if (nodedevCompareToFile(actualCmdline, outfile) < 0) goto cleanup; ret = 0; @@ -170,6 +174,27 @@ testMdevctlStop(const void *data) return ret; } +static int +testMdevctlUuidCommandHelper(const void *data) +{ + const struct UuidCommandTestInfo *info = data; + GetStopUndefineCmdFunc func; + const char *cmd; + g_autofree char *cmdlinefile = NULL; + + if (info->command == MDEVCTL_CMD_STOP) { + cmd = "stop"; + func = nodeDeviceGetMdevctlStopCommand; + } else { + return -1; + } + + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv", + abs_srcdir, cmd); + + return testMdevctlUuidCommand(info->uuid, func, cmdlinefile); +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -387,8 +412,15 @@ mymain(void) #define DO_TEST_DEFINE(filename) \ DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) +#define DO_TEST_UUID_COMMAND_FULL(desc, uuid, command) \ + do { \ + struct UuidCommandTestInfo info = { uuid, command }; \ + DO_TEST_FULL(desc, testMdevctlUuidCommandHelper, &info); \ + } \ + while (0) + #define DO_TEST_STOP(uuid) \ - DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) + DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP) #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) -- 2.26.3

This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl' Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 33eb46b3cd..623017f1fd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -135,6 +135,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags); +int virNodeDeviceUndefine(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c index 33db7752b6..d4a0c98b9b 100644 --- a/src/access/viraccessperm.c +++ b/src/access/viraccessperm.c @@ -70,7 +70,7 @@ VIR_ENUM_IMPL(virAccessPermNodeDevice, VIR_ACCESS_PERM_NODE_DEVICE_LAST, "getattr", "read", "write", "start", "stop", - "detach", + "detach", "delete", ); VIR_ENUM_IMPL(virAccessPermNWFilter, diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h index 42996b9741..051246a7b6 100644 --- a/src/access/viraccessperm.h +++ b/src/access/viraccessperm.h @@ -500,6 +500,12 @@ typedef enum { */ VIR_ACCESS_PERM_NODE_DEVICE_DETACH, + /** + * @desc: Delete node device + * @message: Deleting node device driver requires authorization + */ + VIR_ACCESS_PERM_NODE_DEVICE_DELETE, + VIR_ACCESS_PERM_NODE_DEVICE_LAST } virAccessPermNodeDevice; diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index 64a0a7c473..c352462dda 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -79,6 +79,9 @@ typedef virNodeDevice* const char *xmlDesc, unsigned int flags); +typedef int +(*virDrvNodeDeviceUndefine)(virNodeDevice *dev); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -119,4 +122,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; + virDrvNodeDeviceUndefine nodeDeviceUndefine; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index cfc0c9de5b..1d397c6610 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -779,6 +779,42 @@ virNodeDeviceDefineXML(virConnect *conn, } +/** + * virNodeDeviceUndefine: + * @dev: a device object + * + * Undefine the device object. The virtual device is removed from the host + * operating system. This function may require privileged access. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceUndefine(virNodeDevice *dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceUndefine) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceUndefine(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 3d8176351c..99b46587b9 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -888,6 +888,7 @@ LIBVIRT_7.2.0 { global: virDomainStartDirtyRateCalc; virNodeDeviceDefineXML; + virNodeDeviceUndefine; } LIBVIRT_7.1.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 418faa9fb9..48b4b1f438 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -889,6 +889,18 @@ nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) } +virCommand* +nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "undefine", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -904,6 +916,22 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +static int +virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlUndefineCommand(def->caps->data.mdev.uuid, + errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1183,6 +1211,51 @@ nodeDeviceDefineXML(virConnect *conn, } +int +nodeDeviceUndefine(virNodeDevice *device) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def; + + if (nodeDeviceWaitInit() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceUndefineEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Node device '%s' is not defined"), + def->name); + goto cleanup; + } + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlUndefine(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to undefine mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + goto cleanup; + } + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f626e9ac8a..92bb72ee5d 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -107,6 +107,9 @@ nodeDeviceDefineXML(virConnect *conn, const char *xmlDesc, unsigned int flags); +int +nodeDeviceUndefine(virNodeDevice *dev); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -132,6 +135,10 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +virCommand* +nodeDeviceGetMdevctlUndefineCommand(const char *uuid, + char **errmsg); + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9de658cab5..b870446c55 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2322,6 +2322,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 15c592b5b5..d3e21ea797 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8697,6 +8697,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a95ed65f12..10d4233d69 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2154,6 +2154,10 @@ struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6760,5 +6764,13 @@ enum remote_procedure { * @generate: both * @acl: node_device:write */ - REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428 + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, + + /** + * @generate: both + * @priority: high + * @acl: node_device:delete + */ + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429 + }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 3488659da1..792e858770 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1607,6 +1607,9 @@ struct remote_node_device_define_xml_args { struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3613,4 +3616,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_MESSAGES = 426, REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, }; diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv b/tests/nodedevmdevctldata/mdevctl-undefine.argv new file mode 100644 index 0000000000..54717455f7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-undefine.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ undefine -u d76a6b78-45ed-4149-a325-005f9abc5281 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 7cf9fa7c67..e471e2e6eb 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -185,6 +185,9 @@ testMdevctlUuidCommandHelper(const void *data) if (info->command == MDEVCTL_CMD_STOP) { cmd = "stop"; func = nodeDeviceGetMdevctlStopCommand; + } else if (info->command == MDEVCTL_CMD_UNDEFINE) { + cmd = "undefine"; + func = nodeDeviceGetMdevctlUndefineCommand; } else { return -1; } @@ -422,6 +425,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP) +#define DO_TEST_UNDEFINE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE) + #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) @@ -444,6 +450,8 @@ mymain(void) DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + done: nodedevTestDriverFree(driver); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:16AM -0500, Jonathon Jongsma wrote: This API doesn't follow our best practice which is to *always* have an "unsigned int flags" parameter, even if we don't currently think we need it. I think this needs fixing asap since it affects public API, wire protocol and language bindings, and we're not yet locked into the API design. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Fri, Apr 09, 2021 at 03:57:12PM +0200, Ján Tomko wrote: Sigh, the libvirt_public.syms file is wrong too, as it put the symbols in the previous release Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Several functions accept providing a node device by name or by wwnn,wwpn pair. Extract the logic to do this into a function that can be used by both callers. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/virsh-nodedev.c | 62 +++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 1cbf2e082b..5589a5862d 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -111,23 +111,18 @@ static const vshCmdOptDef opts_node_device_destroy[] = { {.name = NULL} }; -static bool -cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +static virNodeDevice* +vshFindNodeDevice(vshControl *ctl, const char *value) { virNodeDevicePtr dev = NULL; - bool ret = false; - const char *device_value = NULL; char **arr = NULL; int narr; virshControlPtr priv = ctl->privData; - if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) - return false; - - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); + if (strchr(value, ',')) { + narr = vshStringToArray(value, &arr); if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); + vshError(ctl, _("Malformed device value '%s'"), value); goto cleanup; } @@ -136,14 +131,33 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) dev = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); } else { - dev = virNodeDeviceLookupByName(priv->conn, device_value); + dev = virNodeDeviceLookupByName(priv->conn, value); } if (!dev) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + vshError(ctl, "%s '%s'", _("Could not find matching device"), value); goto cleanup; } + cleanup: + g_strfreev(arr); + return dev; +} + +static bool +cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevice *dev = NULL; + bool ret = false; + const char *device_value = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + dev = vshFindNodeDevice(ctl, device_value); + if (!dev) + goto cleanup; + if (virNodeDeviceDestroy(dev) == 0) { vshPrintExtra(ctl, _("Destroyed node device '%s'\n"), device_value); } else { @@ -153,7 +167,6 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); if (dev) virNodeDeviceFree(dev); return ret; @@ -578,33 +591,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device = NULL; char *xml = NULL; const char *device_value = NULL; - char **arr = NULL; - int narr; bool ret = false; - virshControlPtr priv = ctl->privData; if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) return false; - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); - if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); - goto cleanup; - } + device = vshFindNodeDevice(ctl, device_value); - if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) - goto cleanup; - - device = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); - } else { - device = virNodeDeviceLookupByName(priv->conn, device_value); - } - - if (!device) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + if (!device) goto cleanup; - } if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) goto cleanup; @@ -613,7 +608,6 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); VIR_FREE(xml); if (device) virNodeDeviceFree(device); -- 2.26.3

This new API function provides a way to start a persistently-defined mediate device that was defined by virNodeDeviceDefineXML() (or one that was defined externally via mdevctl) Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- include/libvirt/libvirt-nodedev.h | 2 + src/driver-nodedev.h | 4 ++ src/libvirt-nodedev.c | 35 ++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 68 ++++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 +++- src/remote_protocol-structs | 4 ++ tests/nodedevmdevctldata/mdevctl-create.argv | 1 + tests/nodedevmdevctltest.c | 11 +++- 12 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 623017f1fd..cf51c3d085 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -137,6 +137,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, int virNodeDeviceUndefine(virNodeDevicePtr dev); +int virNodeDeviceCreate(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index c352462dda..22fe53bfc0 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -82,6 +82,9 @@ typedef virNodeDevice* typedef int (*virDrvNodeDeviceUndefine)(virNodeDevice *dev); +typedef int +(*virDrvNodeDeviceCreate)(virNodeDevice *def); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -123,4 +126,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; virDrvNodeDeviceUndefine nodeDeviceUndefine; + virDrvNodeDeviceCreate nodeDeviceCreate; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 1d397c6610..a1b9e44ab3 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -815,6 +815,41 @@ virNodeDeviceUndefine(virNodeDevice *dev) } +/** + * virNodeDeviceCreate: + * @dev: a device object + * + * Start a defined node device: + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceCreate(virNodeDevice *dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceCreate) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceCreate(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 99b46587b9..3a8f5d8fc6 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -889,6 +889,7 @@ LIBVIRT_7.2.0 { virDomainStartDirtyRateCalc; virNodeDeviceDefineXML; virNodeDeviceUndefine; + virNodeDeviceCreate; } LIBVIRT_7.1.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 48b4b1f438..cd7677633a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -901,6 +901,18 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) return cmd; } +virCommand* +nodeDeviceGetMdevctlCreateCommand(const char *uuid, char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "start", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -932,6 +944,21 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) } +static int +virMdevctlCreate(virNodeDeviceDef *def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlCreateCommand(def->caps->data.mdev.uuid, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1257,6 +1284,47 @@ nodeDeviceUndefine(virNodeDevice *device) } +int +nodeDeviceCreate(virNodeDevice *device) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + if (virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Device is already active")); + goto cleanup; + } + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceCreateEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlCreate(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to create mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + goto cleanup; + } + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr device, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 92bb72ee5d..27e4095299 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -157,3 +157,10 @@ nodeDeviceGenerateName(virNodeDeviceDef *def, bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, virNodeDeviceDef *src); + +virCommand* +nodeDeviceGetMdevctlCreateCommand(const char *uuid, + char **errmsg); + +int +nodeDeviceCreate(virNodeDevice *dev); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b870446c55..d80e81a5a9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2323,6 +2323,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ + .nodeDeviceCreate = nodeDeviceCreate, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d3e21ea797..31b293ac4a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8696,6 +8696,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceCreate = remoteNodeDeviceCreate, /* 7.2.0 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 10d4233d69..273ae7531e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2158,6 +2158,10 @@ struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6771,6 +6775,13 @@ enum remote_procedure { * @priority: high * @acl: node_device:delete */ - REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429 + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, + + /** + * @generate: both + * @priority: high + * @acl: node_device:start + */ + REMOTE_PROC_NODE_DEVICE_CREATE = 430 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 792e858770..15bd7ae8f0 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1610,6 +1610,9 @@ struct remote_node_device_define_xml_ret { struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3617,4 +3620,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, + REMOTE_PROC_NODE_DEVICE_CREATE = 430, }; diff --git a/tests/nodedevmdevctldata/mdevctl-create.argv b/tests/nodedevmdevctldata/mdevctl-create.argv new file mode 100644 index 0000000000..802109340c --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-create.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ start -u 8a05ad83-3472-497d-8631-8142f31460e8 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index e471e2e6eb..af5e073e8b 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -14,7 +14,8 @@ typedef enum { MDEVCTL_CMD_START, MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, - MDEVCTL_CMD_UNDEFINE + MDEVCTL_CMD_UNDEFINE, + MDEVCTL_CMD_CREATE, } MdevctlCmd; struct startTestInfo { @@ -188,6 +189,9 @@ testMdevctlUuidCommandHelper(const void *data) } else if (info->command == MDEVCTL_CMD_UNDEFINE) { cmd = "undefine"; func = nodeDeviceGetMdevctlUndefineCommand; + }else if (info->command == MDEVCTL_CMD_CREATE) { + cmd = "create"; + func = nodeDeviceGetMdevctlCreateCommand; } else { return -1; } @@ -428,6 +432,9 @@ mymain(void) #define DO_TEST_UNDEFINE(uuid) \ DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE) +#define DO_TEST_CREATE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl create " uuid, uuid, MDEVCTL_CMD_CREATE) + #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) @@ -452,6 +459,8 @@ mymain(void) DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + DO_TEST_CREATE("8a05ad83-3472-497d-8631-8142f31460e8"); + done: nodedevTestDriverFree(driver); -- 2.26.3

This virsh command maps to virNodeDeviceCreate(), which starts a node device that has been previously defined by virNodeDeviceDefineXML(). This is only supported for mediated devices. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/virsh-nodedev.c | 57 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index d5b82ba285..e13c15b4b0 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1112,6 +1112,57 @@ cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } +/* + * "nodedev-start" command + */ +static const vshCmdInfo info_node_device_start[] = { + {.name = "help", + .data = N_("Start an inactive node device") + }, + {.name = "desc", + .data = N_("Starts an inactive node device that was previously defined") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_start[] = { + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceStart(vshControl *ctl, const vshCmd *cmd) +{ + const char *name = NULL; + virNodeDevice *device; + bool ret = true; + virshControl *priv = ctl->privData; + + if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) + return false; + + if (!(device = virNodeDeviceLookupByName(priv->conn, name))) { + vshError(ctl, _("Could not find matching device '%s'"), name); + return false; + } + + if (virNodeDeviceCreate(device) == 0) { + vshPrintExtra(ctl, _("Device %s started\n"), name); + } else { + vshError(ctl, _("Failed to start device %s"), name); + ret = false; + } + + virNodeDeviceFree(device); + return ret; +} + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1177,5 +1228,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_undefine, .flags = 0 }, + {.name = "nodedev-start", + .handler = cmdNodeDeviceStart, + .opts = opts_node_device_start, + .info = info_node_device_start, + .flags = 0 + }, {.name = NULL} }; -- 2.26.3

Use the new <uuid> element in the mdev caps to define and start devices with a specific UUID. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 19 ++++++++++++++++--- ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 3 ++- ...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 3 ++- ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml | 1 + 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index cd7677633a..b1b3217afb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -729,6 +729,10 @@ nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def, NULL); virCommandSetInputBuffer(cmd, json); + + if (def->caps->data.mdev.uuid) + virCommandAddArgPair(cmd, "--uuid", def->caps->data.mdev.uuid); + virCommandSetOutputBuffer(cmd, uuid_out); virCommandSetErrorBuffer(cmd, errmsg); @@ -816,7 +820,12 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; } - return nodeDeviceFindNewMediatedDevice(conn, uuid); + if (uuid && uuid[0]) { + g_free(def->caps->data.mdev.uuid); + def->caps->data.mdev.uuid = g_steal_pointer(&uuid); + } + + return nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); } @@ -1230,9 +1239,13 @@ nodeDeviceDefineXML(virConnect *conn, return NULL; } - def->caps->data.mdev.uuid = g_strdup(uuid); + if (uuid && uuid[0]) { + g_free(def->caps->data.mdev.uuid); + def->caps->data.mdev.uuid = g_steal_pointer(&uuid); + } + mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, uuid); + device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); return device; } diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv index 773e98b963..118ec7a8da 100644 --- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv @@ -1 +1,2 @@ -$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin \ +--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366 diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv index eb7262035e..129f438e4a 100644 --- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv @@ -1 +1,2 @@ -$MDEVCTL_BINARY$ start -p 0000:00:02.0 --jsonfile /dev/stdin +$MDEVCTL_BINARY$ start -p 0000:00:02.0 --jsonfile /dev/stdin \ +--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366 diff --git a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml index d6a2e99edc..605d8f63a1 100644 --- a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml +++ b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml @@ -3,5 +3,6 @@ <parent>pci_0000_00_02_0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> + <uuid>d069d019-36ea-4111-8f0a-8c9a70e21366</uuid> </capability> </device> -- 2.26.3

Calling `mdevctl stop` for a mediated device that is in use by an active domain will block until that vm exits (or the vm closes the device). Since the nodedev driver cannot query the hypervisor driver to see whether any active domains are using the device, we resort to a workaround that relies on the fact that a vfio group can only be opened by one user at a time. If we get an EBUSY error when attempting to open the group file, we assume the device is in use and refuse to try to destroy that device. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/node_device/node_device_driver.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b1b3217afb..31322e3afb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1177,8 +1177,27 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + /* If this mediated device is in use by a vm, attempting to stop it + * will block until the vm closes the device. The nodedev driver + * cannot query the hypervisor driver to determine whether the device + * is in use by any active domains, since that would introduce circular + * dependencies between daemons and add a risk of deadlocks. So we need + * to resort to a workaround. vfio only allows the group for a device + * to be opened by one user at a time. So if we get EBUSY when opening + * the group, we infer that the device is in use and therefore we + * shouldn't try to remove the device. */ + g_autofree char *vfiogroup = + virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); + VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY); g_autofree char *errmsg = NULL; + if (fd < 0 && errno == EBUSY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': device in use"), + def->name); + goto cleanup; + } + if (virMdevctlStop(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to destroy '%s': %s"), def->name, -- 2.26.3

Mention that mdev attribute order is significant. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- docs/formatnode.html.in | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 5c7286df8a..c58cd01395 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -379,7 +379,10 @@ This optional element can occur multiple times. It represents a vendor-specific attribute that is used to configure this mediated device. It has two required attributes: - <code>name</code> and <code>value</code>. + <code>name</code> and <code>value</code>. Note that the order + in which attributes are set may be important for some devices. + The order that they appear in the xml definition determines the + order that they will be written to the device. </dd> </dl> </dd> -- 2.26.3

To accomodate re-use of this functionality in a following patch, split out the processing of an individual mdev definition into a separate function. --- src/node_device/node_device_driver.c | 103 +++++++++++++++------------ 1 file changed, 57 insertions(+), 46 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 31322e3afb..35db24817a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1506,6 +1506,60 @@ removeMissingPersistentMdevs(virNodeDeviceObj *obj, } +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0; +} + + int nodeDeviceUpdateMediatedDevices(void) { @@ -1525,52 +1579,9 @@ nodeDeviceUpdateMediatedDevices(void) virNodeDeviceObjListForEachRemove(driver->devs, removeMissingPersistentMdevs, &data); - for (i = 0; i < data.ndefs; i++) { - virNodeDeviceObj *obj; - virObjectEvent *event; - g_autoptr(virNodeDeviceDef) def = defs[i]; - g_autofree char *name = g_strdup(def->name); - bool defined = false; - - def->driver = g_strdup("vfio_mdev"); - - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { - virNodeDeviceDef *d = g_steal_pointer(&def); - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { - virNodeDeviceDefFree(d); - return -1; - } - } else { - bool changed; - virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); - - defined = virNodeDeviceObjIsPersistent(obj); - /* Active devices contain some additional information (e.g. sysfs - * path) that is not provided by mdevctl, so re-use the existing - * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, def); - - if (defined && !changed) { - /* if this device was already defined and the definition - * hasn't changed, there's nothing to do for this device */ - virNodeDeviceObjEndAPI(&obj); - continue; - } - } - - /* all devices returned by virMdevctlListDefined() are persistent */ - virNodeDeviceObjSetPersistent(obj, true); - - if (!defined) - event = virNodeDeviceEventLifecycleNew(name, - VIR_NODE_DEVICE_EVENT_DEFINED, - 0); - else - event = virNodeDeviceEventUpdateNew(name); - - virNodeDeviceObjEndAPI(&obj); - virObjectEventStateQueue(driver->nodeDeviceEventState, event); - } + for (i = 0; i < data.ndefs; i++) + if (nodeDeviceUpdateMediatedDevice(defs[i]) < 0) + return -1; return 0; } -- 2.26.3

When calling virNodeDeviceDefineXML() to define a new mediated device, we call virMdevctlDefine() and then wait for the new device to appear in the driver's device list before returning. This caused long delays due to the behavior of nodeDeviceFindNewMediatedDevice(). This function checks to see if the device is in the list and then waits for 5s before checking again. Because mdevctl is relatively slow to query the list of defined devices[0], the newly-defined device was generally not in the device list when we first checked. This results in libvirt almost always taking at least 5s to complete this API call for mediated devices, which is unacceptable. In order to avoid this long delay, we resort to a workaround. If the call to virMdevctlDefine() was successful, we can assume that this new device will exist the next time we query mdevctl for new devices. So we simply add this provisional device definition directly to the nodedev driver's device list and return from the function. At some point in the future, the mdevctl handler will run and the "official" device will be processed, which will update the provisional device if any new details need to be added. The reason that this is not necessary for virNodeDeviceCreateXML() is because detecting newly-created (not defined) mdevs happens through udev instead of mdevctl. And nodeDeviceFindNewMediatedDevice() always calls 'udevadm settle' before checking to see whether the device is in the list. This allows us to wait just long enough for all udev events to be processed, so the device is almost always in the list the first time we check and so we almost never end up hitting the 5s sleep. [0] on my machine, 'mdevctl list --defined' took around 0.8s to complete for only 3 defined mdevs. --- src/node_device/node_device_driver.c | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 35db24817a..a1b79d93f7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1215,16 +1215,70 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } + +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0; +} + virNodeDevice* -nodeDeviceDefineXML(virConnect *conn, +nodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags) { g_autoptr(virNodeDeviceDef) def = NULL; - virNodeDevice *device = NULL; const char *virt_type = NULL; g_autofree char *uuid = NULL; g_autofree char *errmsg = NULL; + g_autofree char *name = NULL; virCheckFlags(0, NULL); @@ -1264,9 +1318,19 @@ nodeDeviceDefineXML(virConnect *conn, } mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); + name = g_strdup(def->name); + + /* Normally we would call nodeDeviceFindNewMediatedDevice() here to wait + * for the new device to appear. But mdevctl can take a while to query + * devices, and if nodeDeviceFindNewMediatedDevice() doesn't find the new + * device immediately it will wait at 5s before checking again. Since we + * have already received the uuid from virMdevctlDefine(), we can simply + * add the provisional device to the list and return it immediately and + * avoid this long delay. */ + if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def)) < 0) + return NULL; - return device; + return virGetNodeDevice(conn, name); } @@ -1506,60 +1570,6 @@ removeMissingPersistentMdevs(virNodeDeviceObj *obj, } -/* takes ownership of @def and potentially frees it. @def should not be used - * after returning from this function */ -static int -nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) -{ - virNodeDeviceObj *obj; - virObjectEvent *event; - bool defined = false; - g_autoptr(virNodeDeviceDef) owned = def; - g_autofree char *name = g_strdup(owned->name); - - owned->driver = g_strdup("vfio_mdev"); - - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { - virNodeDeviceDef *d = g_steal_pointer(&owned); - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { - virNodeDeviceDefFree(d); - return -1; - } - } else { - bool changed; - virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); - - defined = virNodeDeviceObjIsPersistent(obj); - /* Active devices contain some additional information (e.g. sysfs - * path) that is not provided by mdevctl, so re-use the existing - * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); - - if (defined && !changed) { - /* if this device was already defined and the definition - * hasn't changed, there's nothing to do for this device */ - virNodeDeviceObjEndAPI(&obj); - return 0; - } - } - - /* all devices returned by virMdevctlListDefined() are persistent */ - virNodeDeviceObjSetPersistent(obj, true); - - if (!defined) - event = virNodeDeviceEventLifecycleNew(name, - VIR_NODE_DEVICE_EVENT_DEFINED, - 0); - else - event = virNodeDeviceEventUpdateNew(name); - - virNodeDeviceObjEndAPI(&obj); - virObjectEventStateQueue(driver->nodeDeviceEventState, event); - - return 0; -} - - int nodeDeviceUpdateMediatedDevices(void) { -- 2.26.3