On Tue, Aug 18, 2020 at 09:47:53AM -0500, Jonathon Jongsma wrote: ... ^This produces a significantly smaller diff, but I'm not really a fan of re-defining the same macro in a different way, it can turn out being confusing in the end, since it's a different macro. I suggest renaming the original MATCH macro to something like MATCH_CAP(CAP) and defining this new one as MATCH. ACK to the rest. Erik

On Tue, Aug 18, 2020 at 09:47:55AM -0500, Jonathon Jongsma wrote: I see literally zero difference between the respective file pairs ^above. Was that intentional or they should have tested something else? I'm all for testing, but I feel like ^these single device use cases are both covered with the multiple ones introduced above, since those include devices both with attributes as well as without them. ... ^this name generator code movement should IMO be a standalone patch. ^2 blank lines between functions. given the following JSON: [ { "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" } ] } } ] } ] isn't 'n' == 'nparents'? Asking because right now I see O(n^4) complexity and I'd very much like to optimize it. ... How about we redefine the DO_TEST_FULL macro so that it doesn't pass a reference to info on its own but forces the caller to do that? That way you don't have to do ^this and simply pass NULL safely to DO_TEST_FULL. Erik

Jonathon Jongsma <jjongsma(a)redhat.com&gt; [2020-08-18, 09:47AM -0500]: I find this XML design for the an AP mdev rather unfortunate. First of all, it is really noisy and requires a lot of boiler-plate for the end user. Secondly, which I think it more important, it introduces a completely new and different nomenclature compared to what we currently already have for AP-related devices, i.e. cards virsh # nodedev-dumpxml ap_card05 <device> <name>ap_card05</name> <path>/sys/devices/ap/card05</path> <parent>computer</parent> <driver> <name>cex4card</name> </driver> <capability type='ap_card'> <ap-adapter>0x05</ap-adapter> </capability> </device> and queues virsh # nodedev-dumpxml ap_05_002a <device> <name>ap_05_002a</name> <path>/sys/devices/ap/card05/05.002a</path> <parent>ap_card05</parent> <driver> <name>cex4queue</name> </driver> <capability type='ap_queue'> <ap-adapter>0x05</ap-adapter> <ap-domain>0x002a</ap-domain> </capability> </device> This is really inconsistent and can be confusing for the end user. Can we instead propose something along the lines <device> <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> <parent>matrix</parent> <capability type='mdev'> <type id='vfio_ap-passthrough'/> <iommuGroup number='0'/> <ap-adapter>0x05</ap-adapter> <ap-adapter>0x06</ap-adapter> <ap-domain>0x002a</ap-domain> <ap-domain>0x0004</ap-domain> <ap-domain>0x00ab</ap-domain> <ap-control>0x04</ap-control> </capability> </device> plus any number of enclosing elements that we like for better readability? Best, Bjoern PS: I haven't read the whole patch series, we just have an internal discussion about AP support and the XML design came up. I know that we'd like to generically treat all mdevs as the same but we also should allow for vendor-specific attributes to be represented individually to what makes sense. -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

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 ++ src/conf/node_device_conf.h | 1 + src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 25 +++++++++++++++++++++++-- tools/virsh-nodedev.c | 4 +++- 6 files changed, 34 insertions(+), 3 deletions(-) diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c index 52caa8ffa8..c40819969c 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_STOPPED: + return "Stopped"; + case VIR_NODE_DEVICE_EVENT_STARTED: + return "Started"; case VIR_NODE_DEVICE_EVENT_LAST: break; } diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 98a972e60d..423a583d45 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -192,6 +192,8 @@ int virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, typedef enum { VIR_NODE_DEVICE_EVENT_CREATED = 0, VIR_NODE_DEVICE_EVENT_DELETED = 1, + VIR_NODE_DEVICE_EVENT_STOPPED = 2, + VIR_NODE_DEVICE_EVENT_STARTED = 3, # ifdef VIR_ENUM_SENTINELS VIR_NODE_DEVICE_EVENT_LAST diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 353dbebaf0..7e825ca6a9 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -144,6 +144,7 @@ struct _virNodeDevCapMdev { char *uuid; virMediatedDeviceAttrPtr *attributes; size_t nattributes; + bool persistent; }; typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 3b042e9a45..b47ecba10a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -894,6 +894,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; mdev = &child->caps->data.mdev; + mdev->persistent = true; mdev->uuid = g_strdup(uuid); mdev->type = g_strdup(virJSONValueObjectGetString(props, "mdev_type")); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 93b74b1f24..ac7986bc70 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1213,6 +1213,8 @@ udevRemoveOneDeviceSysPath(const char *path) virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; virObjectEventPtr event = NULL; + virNodeDevCapsDefPtr cap; + int event_type = VIR_NODE_DEVICE_EVENT_DELETED; if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { VIR_DEBUG("Failed to find device to remove that has udev path '%s'", @@ -1221,13 +1223,32 @@ udevRemoveOneDeviceSysPath(const char *path) } def = virNodeDeviceObjGetDef(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 */ + cap = def->caps; + while (cap != NULL) { + if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) { + if (cap->data.mdev.persistent) { + VIR_FREE(def->sysfs_path); + event_type = VIR_NODE_DEVICE_EVENT_STOPPED; + break; + } + } + cap = cap->next; + } + event = virNodeDeviceEventLifecycleNew(def->name, - VIR_NODE_DEVICE_EVENT_DELETED, + event_type, 0); VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + + if (event_type == VIR_NODE_DEVICE_EVENT_DELETED) + virNodeDeviceObjListRemove(driver->devs, obj); + else + virNodeDeviceObjSetActive(obj, false); virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index e87761188f..f078ce6516 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -787,7 +787,9 @@ VIR_ENUM_DECL(virshNodeDeviceEvent); VIR_ENUM_IMPL(virshNodeDeviceEvent, VIR_NODE_DEVICE_EVENT_LAST, N_("Created"), - N_("Deleted")); + N_("Deleted"), + N_("Stopped"), + N_("Started")); static const char * virshNodeDeviceEventToString(int event) -- 2.26.2

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 | 76 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 3 ++ src/node_device/node_device_udev.c | 43 ++++++++++++++++ 3 files changed, 122 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b47ecba10a..349426757e 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1088,3 +1088,79 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def, *(def->name + i) = '_'; } } + + +static int +virMdevctlListDefined(virNodeDeviceDefPtr **devs) +{ + int status; + g_autofree char *output = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (virCommandRun(cmd, &status) < 0) + return -1; + + if (!output) + return -1; + + return nodeDeviceParseMdevctlJSON(output, devs); +} + + +int +mdevctlEnumerateDevices(void) +{ + g_autofree virNodeDeviceDefPtr *devs = NULL; + int ndevs; + size_t i; + + if ((ndevs = virMdevctlListDefined(&devs)) < 0) { + virReportSystemError(errno, "%s", + _("failed to query mdevs from mdevctl")); + return -1; + } + + for (i = 0; i < ndevs; i++) { + virNodeDeviceObjPtr obj; + virObjectEventPtr event; + virNodeDeviceDefPtr dev = devs[i]; + + dev->driver = g_strdup("vfio_mdev"); + + /* If a device defined by mdevctl is already in the list, that means + * that it was found via the normal device discovery process and thus + * is already activated. Active devices contain some additional + * information (e.g. sysfs path) that is not provided by mdevctl, so + * preserve that info */ + bool new_device = true; + if ((obj = virNodeDeviceObjListFindByName(driver->devs, dev->name))) { + virNodeDeviceDefPtr olddef = virNodeDeviceObjGetDef(obj); + + /* Copy any data from the existing device */ + dev->sysfs_path = g_strdup(olddef->sysfs_path); + dev->parent_sysfs_path = g_strdup(olddef->parent_sysfs_path); + dev->driver = g_strdup(olddef->driver); + dev->devnode = g_strdup(olddef->devnode); + dev->caps->data.mdev.iommuGroupNumber = olddef->caps->data.mdev.iommuGroupNumber; + + virNodeDeviceObjEndAPI(&obj); + new_device = false; + } + + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, dev))) { + virNodeDeviceDefFree(dev); + return -1; + } + + if (new_device) + event = virNodeDeviceEventLifecycleNew(dev->name, + VIR_NODE_DEVICE_EVENT_CREATED, + 0); + else + event = virNodeDeviceEventUpdateNew(dev->name); + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + } + + return 0; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index e7317fd67a..b5c1da4ff3 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -131,6 +131,9 @@ int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDefPtr **devs); +int +mdevctlEnumerateDevices(void); + void nodeDeviceGenerateName(virNodeDeviceDefPtr def, const char *subsystem, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ac7986bc70..e06584a3dc 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1306,6 +1306,45 @@ udevSetParent(struct udev_device *device, return 0; } +static virMediatedDeviceAttrPtr * +virMediatedDeviceAttrsCopy(virMediatedDeviceAttrPtr *attrs, + size_t nattrs) +{ + size_t i; + size_t j = 0; + g_autofree virMediatedDeviceAttrPtr *ret = NULL; + + if (nattrs == 0) + return NULL; + + if (VIR_ALLOC_N(ret, nattrs) < 0) + return NULL; + + for (i = 0; i < nattrs; i++) { + virMediatedDeviceAttrPtr attr = virMediatedDeviceAttrNew(); + attr->name = g_strdup(attrs[i]->name); + attr->value = g_strdup(attrs[i]->value); + VIR_APPEND_ELEMENT_INPLACE(ret, j, attr); + } + + return g_steal_pointer(ret); +} + +/* An existing device definition may have additional info from mdevctl that is + * not available from udev. Transfer this data to the new definition */ +static void +nodeDeviceDefCopyExtraData(virNodeDeviceDefPtr dst, + virNodeDeviceDefPtr src) +{ + virNodeDevCapMdevPtr srcmdev = &src->caps->data.mdev; + virNodeDevCapMdevPtr dstmdev = &dst->caps->data.mdev; + + dstmdev->persistent = srcmdev->persistent; + dstmdev->nattributes = srcmdev->nattributes; + dstmdev->attributes = virMediatedDeviceAttrsCopy(srcmdev->attributes, + srcmdev->nattributes); +} + static int udevAddOneDevice(struct udev_device *device) @@ -1341,6 +1380,8 @@ udevAddOneDevice(struct udev_device *device) goto cleanup; if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + nodeDeviceDefCopyExtraData(def, virNodeDeviceObjGetDef(obj)); + virNodeDeviceObjEndAPI(&obj); new_device = false; } @@ -1773,6 +1814,8 @@ nodeStateInitializeEnumerate(void *opaque) /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) goto error; + if (mdevctlEnumerateDevices() != 0) + goto error; nodeDeviceLock(); driver->initialized = true; -- 2.26.2

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 | 61 ++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 349426757e..affd707a65 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1089,6 +1089,37 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def, } } +static bool +mdevMatchPersistentDevices(virConnectPtr conn G_GNUC_UNUSED, + virNodeDeviceDefPtr def) +{ + return (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV && + def->caps->data.mdev.persistent); + + +} + +/* returns a null-terminated array of strings. Free with virStringListFree() */ +static char ** +nodeDeviceGetMdevPersistentDevices(virNodeDeviceObjListPtr devlist) +{ + char **ret = NULL; + int ndevs = virNodeDeviceObjListNumOfDevices(devlist, + NULL, + "mdev", + mdevMatchPersistentDevices); + if (VIR_ALLOC_N(ret, ndevs+1) < 0) + return NULL; + + if (virNodeDeviceObjListGetNames(devlist, NULL, + mdevMatchPersistentDevices, + "mdev", + ret, + ndevs) < 0) + VIR_FREE(ret); + + return ret; +} static int virMdevctlListDefined(virNodeDeviceDefPtr **devs) @@ -1113,6 +1144,7 @@ mdevctlEnumerateDevices(void) g_autofree virNodeDeviceDefPtr *devs = NULL; int ndevs; size_t i; + char **oldmdevs = nodeDeviceGetMdevPersistentDevices(driver->devs); if ((ndevs = virMdevctlListDefined(&devs)) < 0) { virReportSystemError(errno, "%s", @@ -1160,7 +1192,36 @@ mdevctlEnumerateDevices(void) event = virNodeDeviceEventUpdateNew(dev->name); virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + virStringListRemove(&oldmdevs, dev->name); + } + + /* Any mdevs that were previously defined but were not returned by mdevctl + * this time will need to be checked to see if they should be removed from + * the device list */ + for (i = 0; i < virStringListLength((const char **)oldmdevs); i++) { + const char *name = oldmdevs[i]; + virNodeDeviceObjPtr obj = NULL; + if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) { + if (!virNodeDeviceObjIsActive(obj)) { + /* An existing device is not active and is no longer defined by + * mdevctl, so remove it */ + virObjectEventPtr event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DELETED, + 0); + + virNodeDeviceObjListRemove(driver->devs, obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + } else { + /* An existing device is active, but no longer defined. Keep + * the device in the list, but mark it as non-persistent */ + virNodeDeviceDefPtr def = virNodeDeviceObjGetDef(obj); + def->caps->data.mdev.persistent = false; + } + virNodeDeviceObjEndAPI(&obj); + } } + g_strfreev(oldmdevs); return 0; } -- 2.26.2

We need to peridocally query mdevctl for changes to device definitions since an administrator can define new devices with mdevctl outside of libvirt. In order to do this, a new thread is created to handle the querying. 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 directories for changes to files. When a change is detected, we query mdevctl and update our device list. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 182 +++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e06584a3dc..a85418e549 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -41,6 +41,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include <gio/gio.h> #include "configmake.h" @@ -66,6 +67,11 @@ struct _udevEventData { virCond threadCond; bool threadQuit; bool dataReady; + + virThread mdevctlThread; + virCond mdevctlCond; + bool mdevctlReady; + GList *mdevctl_monitors; }; static virClassPtr udevEventDataClass; @@ -85,8 +91,10 @@ udevEventDataDispose(void *obj) udev = udev_monitor_get_udev(priv->udev_monitor); udev_monitor_unref(priv->udev_monitor); udev_unref(udev); + g_list_free_full(priv->mdevctl_monitors, g_object_unref); virCondDestroy(&priv->threadCond); + virCondDestroy(&priv->mdevctlCond); } @@ -117,6 +125,11 @@ udevEventDataNew(void) return NULL; } + if (virCondInit(&ret->mdevctlCond) < 0) { + virObjectUnref(ret); + return NULL; + } + ret->watch = -1; return ret; } @@ -1520,6 +1533,7 @@ nodeStateCleanup(void) virObjectLock(priv); priv->threadQuit = true; virCondSignal(&priv->threadCond); + virCondSignal(&priv->mdevctlCond); virObjectUnlock(priv); virThreadJoin(&priv->th); } @@ -1601,6 +1615,40 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv, } +/* Thread to query mdevctl for the current state of persistent mediated device + * defintions when any changes are detected */ +static +void mdevctlThread(void *opaque G_GNUC_UNUSED) +{ + udevEventDataPtr priv = driver->privateData; + + while (1) { + virObjectLock(priv); + while (!priv->mdevctlReady && !priv->threadQuit) { + if (virCondWait(&priv->mdevctlCond, &priv->parent.lock)) { + virReportSystemError(errno, "%s", + _("handler failed to wait on condition")); + virObjectUnlock(priv); + return; + } + } + + if (priv->threadQuit) { + virObjectUnlock(priv); + return; + } + + virObjectUnlock(priv); + + mdevctlEnumerateDevices(); + + virObjectLock(priv); + priv->mdevctlReady = false; + virObjectUnlock(priv); + } +} + + /** * udevEventHandleThread * @opaque: unused @@ -1708,6 +1756,69 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, } +static int timeout_id; + +static gboolean +signalMdevctlThread(void *user_data) +{ + udevEventDataPtr priv = user_data; + + if (timeout_id) { + g_source_remove(timeout_id); + timeout_id = 0; + } + + virObjectLock(priv); + priv->mdevctlReady = true; + virCondSignal(&priv->mdevctlCond); + virObjectUnlock(priv); + + return G_SOURCE_REMOVE; +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data) +{ + udevEventDataPtr priv = user_data; + + /* if a new directory appears, monitor that directory for changes */ + if (g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) { + GFileMonitor *mon; + + if ((mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, NULL))) { + g_signal_connect(mon, "changed", + G_CALLBACK(mdevctlEventHandleCallback), + user_data); + virObjectLock(priv); + priv->mdevctl_monitors = g_list_append(priv->mdevctl_monitors, mon); + virObjectUnlock(priv); + } + } + + /* Sometimes a single configuration change results in multiple notify + * events (e.g. several CHANGED events, or a CREATED and CHANGED event + * followed by CHANGES_DONE_HINT). To avoid triggering the mdevctl thread + * multiple times for a single 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_CREATED || + event_type == G_FILE_MONITOR_EVENT_CHANGED) { + if (timeout_id) + g_source_remove(timeout_id); + timeout_id = g_timeout_add(100, signalMdevctlThread, priv); + return; + } + + signalMdevctlThread(priv); +} + + /* DMI is intel-compatible specific */ #if defined(__x86_64__) || defined(__i386__) || defined(__amd64__) static void @@ -1858,6 +1969,58 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) } +/* Recursively monitors dir and any subdirectory for file changes and returns a + * GList of GFileMonitor objects */ +static GList* +monitorDirRecursively(GFile *dir, + GCallback changed_cb, + gpointer user_data) +{ + GList *monitors = NULL; + g_autoptr(GFileInfo) dirinfo = NULL; + g_autoptr(GError) error = NULL; + g_autoptr(GFileEnumerator) children = NULL; + GFileMonitor *mon; + + if (!(children = g_file_enumerate_children(dir, "standard::*", + G_FILE_QUERY_INFO_NONE, NULL, &error))) { + if (error->code == G_IO_ERROR_NOT_DIRECTORY) + return NULL; + goto bail; + } + + if (!(mon = g_file_monitor(dir, G_FILE_MONITOR_NONE, NULL, &error))) + goto bail; + + g_signal_connect(mon, "changed", changed_cb, user_data); + monitors = g_list_append(monitors, mon); + + while (true) { + GFileInfo *info; + GFile *child; + GList *child_monitors = NULL; + + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error)) + goto bail; + if (!info) + break; + + child_monitors = monitorDirRecursively(child, changed_cb, user_data); + if (child_monitors) + monitors = g_list_concat(monitors, child_monitors); + } + + return monitors; + + bail: + if (error) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to monitor directory: %s"), error->message); + + g_list_free_full(monitors, g_object_unref); + return NULL; +} + static int nodeStateInitialize(bool privileged, const char *root, @@ -1867,6 +2030,8 @@ nodeStateInitialize(bool privileged, udevEventDataPtr priv = NULL; struct udev *udev = NULL; virThread enumThread; + g_autoptr(GError) error = NULL; + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -1969,6 +2134,23 @@ nodeStateInitialize(bool privileged, if (priv->watch == -1) goto unlock; + if (virThreadCreateFull(&priv->mdevctlThread, true, mdevctlThread, + "mdevctl-event", false, NULL) < 0) { + virReportSystemError(errno, "%s", + _("failed to create mdevctl handler thread")); + 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 */ + priv->mdevctl_monitors = monitorDirRecursively(mdevctlConfigDir, + G_CALLBACK(mdevctlEventHandleCallback), + priv); + virObjectUnlock(priv); /* Create a fictional 'computer' device to root the device tree. */ -- 2.26.2

On Tue, Aug 25, 2020 at 01:44:40PM +0100, Daniel P. Berrangé wrote: We sure don't - in that case we'd have to introduce another lock in mdevctlEnumerateDevices to serialize the threads so that the updates to the devices are guaranteed to be performed in the order the events come. Just a question, if we spawn a thread for each event, can't we by any chance theoretically DOS the host system if there's a malicious process creating/removing/stating files? Erik

On Tue, Aug 25, 2020 at 03:59:47PM +0100, Daniel P. Berrangé wrote: It's true that /etc/mdevctl.d is owned by root:root, so only a misconfiguration or a broken app like you say could exploit this with the former being the case for libvirt as well. Okay, individual threads it is then. Erik

On a Tuesday in 2020, Jonathon Jongsma wrote: ../src/node_device/node_device_udev.c:1980:26: error: unused variable 'dirinfo' [-Werror,-Wunused-variable] g_autoptr(GFileInfo) dirinfo = NULL; ^ s/bail/error/ When returning an error code, any libvirt function should either report an error in all cases or stay quiet, so something like: error ? error->message : _("unknown error") is a tiny bit frendlier for debugging. ../src/node_device/node_device_udev.c:2033:23: error: unused variable 'error' [-Werror,-Wunused-variable] g_autoptr(GError) error = NULL; ^ Jano

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 | 4 + src/node_device/node_device_driver.c | 97 +++++++++++++++++-- src/node_device/node_device_driver.h | 10 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 18 +++- src/remote_protocol-structs | 8 ++ src/rpc/gendispatch.pl | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + tests/nodedevmdevctltest.c | 68 +++++++++---- 18 files changed, 241 insertions(+), 25 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 423a583d45..02aa9d9750 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -127,6 +127,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn, int virNodeDeviceDestroy (virNodeDevicePtr dev); +virNodeDevicePtr virNodeDeviceDefineXML (virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index d0fc7f19cf..16d787f3fc 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev); +typedef virNodeDevicePtr +(*virDrvNodeDeviceDefineXML)(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -113,4 +118,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceListCaps nodeDeviceListCaps; virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; + virDrvNodeDeviceDefineXML nodeDeviceDefineXML; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 6dc61304a0..1704de929e 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -761,6 +761,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) } +/** + * virNodeDeviceDefineXML: + * @conn: pointer to the hypervisor connection + * @xmlDesc: string containing an XML description of the device to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Define a new device on the VM host machine, for example, a mediated device + * + * virNodeDeviceFree should be used to free the resources after the + * node device object is no longer needed. + * + * Returns a node device object if successful, NULL in case of failure + */ +virNodeDevicePtr +virNodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, xmlDesc=%s, flags=0x%x", conn, NULLSTR(xmlDesc), flags); + + virResetLastError(); + + virCheckConnectReturn(conn, NULL); + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(xmlDesc, error); + + if (conn->nodeDeviceDriver && + conn->nodeDeviceDriver->nodeDeviceDefineXML) { + virNodeDevicePtr dev = conn->nodeDeviceDriver->nodeDeviceDefineXML(conn, xmlDesc, flags); + if (dev == NULL) + goto error; + return dev; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return NULL; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 539d2e3943..27c637e37c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -873,4 +873,8 @@ LIBVIRT_6.0.0 { virDomainBackupGetXMLDesc; } LIBVIRT_5.10.0; +LIBVIRT_6.5.0 { + global: + virNodeDeviceDefineXML; +} LIBVIRT_6.0.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index affd707a65..16f3537776 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -658,9 +658,13 @@ nodeDeviceFindAddressByName(const char *name) } -virCommandPtr -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) +/* the mdevctl 'start' and 'define' commands accept almost the exact same + * arguments, so provide a common implementation that can be wrapped by a more + * specific function */ +static virCommandPtr +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDefPtr def, + const char *subcommand, + char **uuid_out) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -678,7 +682,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; } - cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_pci, "--jsonfile", "/dev/stdin", NULL); @@ -689,11 +693,29 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return cmd; } +virCommandPtr +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, + char **uuid_out) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out); +} + +virCommandPtr +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, + char **uuid_out) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out); +} + + + static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlDefineCommon(virNodeDeviceDefPtr def, + virCommandPtr (*func)(virNodeDeviceDefPtr, char**), + char **uuid) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = func(def, uuid); if (!cmd) return -1; @@ -709,6 +731,20 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) } +static int +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +{ + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlStartCommand, uuid); +} + + +static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid) +{ + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlDefineCommand, uuid); +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) @@ -1008,6 +1044,55 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } +virNodeDevicePtr +nodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virNodeDevicePtr device = NULL; + const char *virt_type = NULL; + + virCheckFlags(0, NULL); + + if (nodeDeviceWaitInit() < 0) + return NULL; + + virt_type = virConnectGetType(conn); + + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) + return NULL; + + if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0) + return NULL; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *uuid = NULL; + + if (!def->parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot define a mediated device without a parent")); + return NULL; + } + + if (virMdevctlDefine(def, &uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to define mediated device")); + return NULL; + } + + def->caps->data.mdev.uuid = g_strdup(uuid); + mdevGenerateDeviceName(def); + device = nodeDeviceFindNewMediatedDevice(conn, uuid); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + return device; +} + + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b5c1da4ff3..cf03e0b3cf 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -107,6 +107,11 @@ nodeDeviceCreateXML(virConnectPtr conn, int nodeDeviceDestroy(virNodeDevicePtr dev); +virNodeDevicePtr +nodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -121,6 +126,11 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, char **uuid_out); + +virCommandPtr +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, + char **uuid_out); + virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a85418e549..5101fff146 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2198,6 +2198,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceListCaps = nodeDeviceListCaps, /* 0.7.3 */ .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ + .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 6.5.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 0331060a2d..64d0d0c49b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8814,6 +8814,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 6.5.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d4393680e9..ddfdb9c084 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2139,6 +2139,15 @@ struct remote_node_device_destroy_args { remote_nonnull_string name; }; +struct remote_node_device_define_xml_args { + remote_nonnull_string xml_desc; + unsigned int flags; +}; + +struct remote_node_device_define_xml_ret { + remote_nonnull_node_device dev; +}; + /* * Events Register/Deregister: @@ -6664,5 +6673,12 @@ enum remote_procedure { * @priority: high * @acl: domain:read */ - REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422 + REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422, + + /** + * @generate: both + * @acl: node_device:write + * @acl: node_device:start + */ + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index bae0f0b545..c325923eaf 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1600,6 +1600,13 @@ struct remote_node_device_create_xml_ret { struct remote_node_device_destroy_args { remote_nonnull_string name; }; +struct remote_node_device_define_xml_args { + remote_nonnull_string xml_desc; + u_int flags; +}; +struct remote_node_device_define_xml_ret { + remote_nonnull_node_device dev; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3558,4 +3565,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_AGENT_SET_RESPONSE_TIMEOUT = 420, REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 421, REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422, + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 0b2ae59910..fa409fe3ab 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -567,6 +567,7 @@ elsif ($mode eq "server") { if ($argtype =~ m/^remote_node_device_/ and !($argtype =~ m/^remote_node_device_lookup_by_name_/) and !($argtype =~ m/^remote_node_device_create_xml_/) and + !($argtype =~ m/^remote_node_device_define_xml_/) and !($argtype =~ m/^remote_node_device_lookup_scsi_host_by_wwn_/)) { $has_node_device = 1; push(@vars_list, "virNodeDevicePtr dev = NULL"); diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json new file mode 100644 index 0000000000..bfc6dcace3 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual"} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json new file mode 100644 index 0000000000..e5b22b2c44 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute-1":"attribute-value-1"},{"example-attribute-2":"attribute-value-2"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json new file mode 100644 index 0000000000..2e03d0bd8e --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute":"attribute-value"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index cee0d913dd..f821622ff6 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -10,10 +10,16 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +typedef enum { + MDEVCTL_CMD_START, + MDEVCTL_CMD_DEFINE, +} MdevctlCmd; + struct startTestInfo { const char *virt_type; int create; const char *filename; + MdevctlCmd command; }; /* capture stdin passed to command */ @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual, return virTestCompareToFile(replacedCmdline, filename); } + +typedef virCommandPtr (*GetStartDefineCmdFunc)(virNodeDeviceDefPtr, char **); + + static int testMdevctlStart(const char *virt_type, int create, + GetStartDefineCmdFunc get_cmd_func, const char *mdevxml, - const char *startcmdfile, - const char *startjsonfile) + const char *cmdfile, + const char *jsonfile) { g_autoptr(virNodeDeviceDef) def = NULL; virNodeDeviceObjPtr obj = NULL; @@ -66,7 +77,7 @@ testMdevctlStart(const char *virt_type, /* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid); + cmd = get_cmd_func(def, &uuid); if (!cmd) goto cleanup; @@ -78,10 +89,10 @@ testMdevctlStart(const char *virt_type, if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0) + if (nodedevCompareToFile(actualCmdline, cmdfile) < 0) goto cleanup; - if (virTestCompareToFile(stdinbuf, startjsonfile) < 0) + if (virTestCompareToFile(stdinbuf, jsonfile) < 0) goto cleanup; ret = 0; @@ -96,17 +107,31 @@ static int testMdevctlStartHelper(const void *data) { const struct startTestInfo *info = data; + const char *cmd; + GetStartDefineCmdFunc func; + g_autofree char *mdevxml = NULL; + g_autofree char *cmdlinefile = NULL; + g_autofree char *jsonfile = NULL; + + if (info->command == MDEVCTL_CMD_START) { + cmd = "start"; + func = nodeDeviceGetMdevctlStartCommand; + } else if (info->command == MDEVCTL_CMD_DEFINE) { + cmd = "define"; + func = nodeDeviceGetMdevctlDefineCommand; + } else { + return -1; + } - g_autofree char *mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", - abs_srcdir, info->filename); - g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.argv", - abs_srcdir, info->filename); - g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.json", - abs_srcdir, info->filename); + mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir, + info->filename); + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.argv", + abs_srcdir, info->filename, cmd); + jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir, + info->filename, cmd); - return testMdevctlStart(info->virt_type, - info->create, mdevxml, cmdlinefile, - jsonfile); + return testMdevctlStart(info->virt_type, info->create, func, + mdevxml, cmdlinefile, jsonfile); } static int @@ -353,15 +378,18 @@ mymain(void) if (virTestRun(desc, func, &info) < 0) \ ret = -1; -#define DO_TEST_START_FULL(virt_type, create, filename) \ +#define DO_TEST_START_FULL(desc, virt_type, create, filename, command) \ do { \ - struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ + struct startTestInfo info = { virt_type, create, filename, command }; \ + DO_TEST_FULL(desc, testMdevctlStartHelper, info); \ } \ while (0) #define DO_TEST_START(filename) \ - DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) + DO_TEST_START_FULL("mdevctl start " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_START) + +#define DO_TEST_DEFINE(filename) \ + DO_TEST_START_FULL("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) @@ -390,6 +418,10 @@ mymain(void) DO_TEST_PARSE_JSON("mdevctl-list-multiple"); DO_TEST_PARSE_JSON("mdevctl-list-multiple-parents"); + DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + done: nodedevTestDriverFree(driver); -- 2.26.2

On Tue, Aug 18, 2020 at 09:48:02AM -0500, Jonathon Jongsma wrote: ... Depending on 10/16 comments, ^this would need some micro adjustments. I haven't had a deep look at the mdevctl thread yet, so I hope we won't get to a racy kind of situation where we'd try to undefine a device that doesn't exist either transient or persistent in mdevctl anymore. ... Hopefully with comments in 10/16 ^this could become testMdevctlCommand and simplify these test code bits more. Regards, Erik

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; --- tools/virsh-nodedev.c | 58 +++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 45d6c7b493..34203c0e91 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 virNodeDevicePtr +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,9 +131,30 @@ 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"), value); + goto cleanup; + } + + cleanup: + g_strfreev(arr); + return dev; +} + +static bool +cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevicePtr 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) { vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); goto cleanup; @@ -153,7 +169,6 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); if (dev) virNodeDeviceFree(dev); return ret; @@ -569,28 +584,12 @@ 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; - } - - 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); - } + device = vshFindNodeDevice(ctl, device_value); if (!device) { vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); @@ -604,7 +603,6 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); VIR_FREE(xml); if (device) virNodeDeviceFree(device); -- 2.26.2

Add a virsh command that maps to virNodeDeviceUndefine(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- tools/virsh-nodedev.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 34203c0e91..db56fbc5e9 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1002,6 +1002,65 @@ cmdNodeDeviceEvent(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-undefine" command + */ +static const vshCmdInfo info_node_device_undefine[] = { + {.name = "help", + .data = N_("Undefine an inactive node device") + }, + {.name = "desc", + .data = N_("Undefines the configuration for an inactive node device") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_undefine[] = { + {.name = "name", + .type = VSH_OT_ALIAS, + .help = "device" + }, + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceUndefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) +{ + virNodeDevicePtr 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) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + } + + if (virNodeDeviceUndefine(dev) == 0) { + vshPrintExtra(ctl, _("Undefined node device '%s'\n"), device_value); + } else { + vshError(ctl, _("Failed to undefine node device '%s'"), device_value); + goto cleanup; + } + + ret = true; + cleanup: + if (dev) + virNodeDeviceFree(dev); + return ret; +} + + /* * "nodedev-define" command */ @@ -1113,5 +1172,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_define, .flags = 0 }, + {.name = "nodedev-undefine", + .handler = cmdNodeDeviceUndefine, + .opts = opts_node_device_undefine, + .info = info_node_device_undefine, + .flags = 0 + }, {.name = NULL} }; -- 2.26.2

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; --- 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 | 64 ++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++ 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 ++ tests/nodedevmdevctldata/mdevctl-create.argv | 1 + tests/nodedevmdevctltest.c | 11 +++- 12 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index f7957499ae..cc6d4c8732 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -133,6 +133,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 e18029ab48..d00dd5845c 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -82,6 +82,9 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceUndefine)(virNodeDevicePtr dev); +typedef int +(*virDrvNodeDeviceCreate)(virNodeDevicePtr 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 eb01ceb8b0..9340ca8410 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -839,6 +839,41 @@ virNodeDeviceUndefine(virNodeDevicePtr 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(virNodeDevicePtr 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 35c88ec4b7..fa7144cec1 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -877,5 +877,6 @@ LIBVIRT_6.5.0 { global: virNodeDeviceDefineXML; virNodeDeviceUndefine; + virNodeDeviceCreate; } LIBVIRT_6.0.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 60a360e4b5..c25a77c231 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -844,6 +844,17 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid) } +virCommandPtr +nodeDeviceGetMdevctlCreateCommand(const char *uuid) +{ + return virCommandNewArgList(MDEVCTL, + "start", + "-u", + uuid, + NULL); + +} + static int virMdevctlStop(virNodeDeviceDefPtr def) { @@ -874,6 +885,21 @@ virMdevctlUndefine(virNodeDeviceDefPtr def) } +static int +virMdevctlCreate(virNodeDeviceDefPtr def) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlCreateCommand(def->caps->data.mdev.uuid); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1162,6 +1188,44 @@ nodeDeviceUndefine(virNodeDevicePtr device) } +int nodeDeviceCreate(virNodeDevicePtr device) +{ + int ret = -1; + virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def; + g_autofree char *parent = 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)) { + if (virMdevctlCreate(def) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to create mediated device")); + 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 2adb3a8073..939718effb 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -155,3 +155,9 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def, const char *subsystem, const char *sysname, const char *s); + +virCommandPtr +nodeDeviceGetMdevctlCreateCommand(const char *uuid); + +int +nodeDeviceCreate(virNodeDevicePtr dev); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 19d6642b21..6dd8ea8ac9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2200,6 +2200,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 6.5.0 */ .nodeDeviceUndefine = nodeDeviceUndefine, /* 6.5.0 */ + .nodeDeviceCreate = nodeDeviceCreate, /* 6.5.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index fc27278c4d..3264573261 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8814,6 +8814,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceCreate = remoteNodeDeviceCreate, /* 6.5.0 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 6.5.0 */ .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 6.5.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d668074204..b019f38efd 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2152,6 +2152,10 @@ struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6682,7 +6686,6 @@ enum remote_procedure { /** * @generate: both * @acl: node_device:write - * @acl: node_device:start */ REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423, @@ -6691,6 +6694,13 @@ enum remote_procedure { * @priority: high * @acl: node_device:stop */ - REMOTE_PROC_NODE_DEVICE_UNDEFINE = 424 + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 424, + + /** + * @generate: both + * @priority: high + * @acl: node_device:start + */ + REMOTE_PROC_NODE_DEVICE_CREATE = 425 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 3d450a6de9..a3cfe2572e 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; }; @@ -3570,4 +3573,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423, REMOTE_PROC_NODE_DEVICE_UNDEFINE = 424, + REMOTE_PROC_NODE_DEVICE_CREATE = 425, }; 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 216c70f1b8..a6726616dc 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 { @@ -186,6 +187,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; } @@ -432,6 +436,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 { \ if (virTestRun("mdevctl list --defined", testMdevctlListDefined, NULL) < 0) \ @@ -462,6 +469,8 @@ mymain(void) DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + DO_TEST_CREATE("8a05ad83-3472-497d-8631-8142f31460e8"); + done: nodedevTestDriverFree(driver); -- 2.26.2

On a Tuesday in 2020, Jonathon Jongsma wrote: ../src/node_device/node_device_driver.c:1128:22: error: unused variable 'parent' [-Werror,-Wunused-variable] g_autofree char *parent = NULL; ^ This change looks fishy. The last enum entry can be followed by a colon to avoid changes like these, it's just remote_protocol.x where that does not work. Jano

On Tue, Aug 18, 2020 at 09:48:06AM -0500, Jonathon Jongsma wrote: How about "Takes an inactive node device configuration and creates a device based on it" would that work as a description? Regards, Erik