On Thu, Dec 24, 2020 at 08:14:29AM -0600, Jonathon Jongsma wrote: At this point I think we could split this even further and add the parser code first and then introduce listing support. ... One declaration per line please (I've seen another one, but not sure if it was in this patch or some later one). As pointed out in v2: s/parsefailure/error ... Why are there 2 of each? Does that indicate to mdevctl that when creating an mdev on the matrix device it can pick any of the adapters for the mdev? Are ^these the real life attribute names mdevctl uses for vfio_ap-passthrough? ... Bjoern was right in his response to v2 when he said this adds quite some noise for the end user and is not consistent at all with how we represent the AP scheme. On the other hand, at the time of posting the v2 we already had introduced and documented the <attr> element, so I still think what you have here is the right approach because should we have gone with vendor specific elements the end users might get the impression that libvirt owns the attributes or IOW is somehow responsible for whether the mdev is going to be created successfully with these attributes or not - whilst we're just the middle man and we're merely passing these attributes to mdevctl and we also document that it's a generic mechanism to allow vendor-specific attributes to be specified along with the mdev. However, the end users know nothing about what the names of the attributes are and what mdevctl accepts, so maybe we could, for XML purposes, name the attributes the same way as we name the AP elements, i.e. ap-adapter, ap-control (yeah, it should have been "ap_xyz" I overlooked this during review, so that is on me). The fact we're using mdevctl underneath is completely transparent to the end users, so we should strive for at least some kind of consistency so anyone can tell how the individual node devices relate to each other. Code-wise I don't see any outstanding problems anymore, so once we come to an agreement on what the XML needs to look like to make most (ideally all) parties happy, you have my: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;

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 f164e825e1..d6eec648ec 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 d304283871..a473563857 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -197,6 +197,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 3d7872fd6e..bbc28cf2b9 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -157,6 +157,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 bbd373e32e..5309b8abd5 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -899,6 +899,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, 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 be59e6c6bc..632413d046 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1401,6 +1401,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'", @@ -1409,13 +1411,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 69422e20f5..35117585ff 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_("Stopped"), + N_("Started")); static const char * virshNodeDeviceEventToString(int event) -- 2.26.2

On Tue, 5 Jan 2021 17:25:50 +0100 Erik Skultety <eskultet(a)redhat.com&gt; wrote: hmm, I apparently missed that comment. But I guess the reason that I added it to the mdev caps rather than the object is because it really only applies to mdev devices. For instance, we don't really have a mechanism to keep around the definition of e.g. a PCI device that has been removed from the machine. And I don't really see any use case for doing so. I suppose we could just set this value to 'false' for all non-mdev node devices, but that doesn't seem very elegant to me.

On Thu, Dec 24, 2020 at 08:14:30AM -0600, Jonathon Jongsma wrote: This doesn't feel right to me. STARTED/STOPPED are esstentially the same as CREATED/DELETED semantically. None of the other APIs emit different events for persistent vs transient objects. We're adding the ability to define/undefine inactive mdev configs, so what is missing is the DEFINED/UNDEFINED events I believe. 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 Thu, Dec 24, 2020 at 08:14:31AM -0600, Jonathon Jongsma wrote: ^This was called mdevctlEnumerateDevices in v2, so I'm wondering why did you change the name? I think it was okay the way it was (I double checked that I didn't suggest this change in v2 by accident). So, what data other than attributes are coming from mdevctl that we have to copy the udev provided data from the old def ^this way? When I look at virNodeDeviceDef, If I'm not missing anything, the added value from mdevctl are the caps, more specifically, attributes right? If that is true I think we should revert the logic and use the function that you introduce at the end of the patch that copies extra data and use it here as well. To describe my thoughts into more details, at the end of the patch you add calls to udevAddOneDevice and nodeStateInitializeEnumerate. In the former case, we already enumerated the devices, including mdevctl, so now when we start an mdev, we get an event from udev and what do we do? We take that info and copy the attributes from the existing definition to it, so the data flows more or less like udev <- mdevctl. In the latter case though, we first enumerated udev devices, then asked mdevctl which created a new def ptr for us and we copied the data from udev and then redefined the device, so mdevctl <- udev. If I'm not missing some crucial information I think the logic can be reverted in one of those cases so that we always copy from the same source to the same destination to apply a new definition. ^This name is too vague, we're likely going to only use it with mdevs (and if the time comes we need to make it generic, we can easily create a wrapper). I'd suggest nodeDeviceDefCopyFromMdevctl or something similar to make it clear at first glance what we're trying to copy, because extra data is basically "void *". I think ^this check would be better on the caller side since we're tailoring it to mdevs. ...the check mentioned above should come here, it'll help the code readability instantly to signal that we're not trying to change/copy anything unless the device for which udev generated an event is an mdev. Erik

On Wed, Jan 06, 2021 at 11:27:09AM +0100, Erik Skultety wrote: In patch 9 I see the reason and it's indeed better to be named this way, so you can ignore ^this comment, sorry, I was too hasty. Erik

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 virNodeDeviceObjListForEach() (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 | 62 ++++++++++++++++++++++++++++++++++--- src/conf/virnodedeviceobj.h | 12 +++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index 6e9291264a..5f0b21f8bf 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -506,23 +506,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(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr dev) +{ + virHashRemoveEntry(devs->objs, dev->def->name); +} + + /* * Return the NPIV dev's parent device name */ @@ -1000,3 +1006,51 @@ virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, { obj->active = active; } + +struct _virNodeDeviceObjListForEachData { + virNodeDeviceObjListIterator iter; + const void *opaque; +}; + +static int +virNodeDeviceObjListForEachCb(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + virNodeDeviceObjPtr obj = payload; + struct _virNodeDeviceObjListForEachData *data = opaque; + + /* Grab a reference so that we don't rely only on references grabbed by + * hash table earlier. Remember, an iterator can remove object from the + * hash table. */ + virObjectRef(obj); + virObjectLock(obj); + data->iter(obj, data->opaque); + virNodeDeviceObjEndAPI(&obj); + + return 0; +} + +/** + * virNodeDeviceObjListForEach + * @devs: Pointer to object list + * @iter: Callback iteration helper + * @opaque: Opaque data to use as argument to helper + * + * For each object in @devs, call the @iter helper using @opaque as + * an argument. + */ +void +virNodeDeviceObjListForEach(virNodeDeviceObjListPtr devs, + virNodeDeviceObjListIterator iter, + const void *opaque) +{ + struct _virNodeDeviceObjListForEachData data = { + .iter = iter, + .opaque = opaque + }; + + virObjectRWLockWrite(devs); + virHashForEach(devs->objs, virNodeDeviceObjListForEachCb, &data); + virObjectRWUnlock(devs); +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index c119f4c51f..e672d8af61 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -80,6 +80,10 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr dev); +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjListPtr devs, + virNodeDeviceObjPtr dev); + int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); @@ -128,3 +132,11 @@ virNodeDeviceObjIsActive(virNodeDeviceObjPtr obj); void virNodeDeviceObjSetActive(virNodeDeviceObjPtr obj, bool active); + +typedef void +(*virNodeDeviceObjListIterator)(virNodeDeviceObjPtr obj, + const void *opaque); + +void virNodeDeviceObjListForEach(virNodeDeviceObjListPtr devs, + virNodeDeviceObjListIterator iter, + const void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fecd012300..181d91792e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1210,12 +1210,14 @@ virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFindMediatedDeviceByUUID; virNodeDeviceObjListFindSCSIHostByWWNs; +virNodeDeviceObjListForEach; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; -- 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 | 67 +++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0267005af1..0bebd534d0 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1163,23 +1163,73 @@ virMdevctlListDefined(virNodeDeviceDefPtr **devs) } +typedef struct _virMdevctlForEachData virMdevctlForEachData; +struct _virMdevctlForEachData { + int ndevs; + virNodeDeviceDefPtr *devs; +}; + + +/* 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, remove it from the driver as well. */ +static void +removeMissingPersistentMdevs(virNodeDeviceObjPtr obj, + const void *opaque) +{ + const virMdevctlForEachData *data = opaque; + size_t i; + virNodeDeviceDefPtr def = virNodeDeviceObjGetDef(obj); + virObjectEventPtr event; + + /* 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 (!(def->caps->data.type == VIR_NODE_DEV_CAP_MDEV && + def->caps->data.mdev.persistent)) + return; + + for (i = 0; i < data->ndevs; i++) { + /* OK, this mdev is still defined by mdevctl */ + if (STREQ(data->devs[i]->name, def->name)) + return; + } + + if (virNodeDeviceObjIsActive(obj)) { + /* The device is active, but no longer defined by mdevctl. Keep + * the device in the list, but mark it as non-persistent */ + def->caps->data.mdev.persistent = false; + return; + } + + /* the device is not active, and it is no longer defined by mdevctl, so + * remove it. */ + event = virNodeDeviceEventLifecycleNew(def->name, + VIR_NODE_DEVICE_EVENT_DELETED, + 0); + + virNodeDeviceObjListRemoveLocked(driver->devs, obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); +} + + int nodeDeviceUpdateMediatedDevices(void) { - g_autofree virNodeDeviceDefPtr *devs = NULL; - int ndevs; size_t i; + virMdevctlForEachData data = { 0, }; - if ((ndevs = virMdevctlListDefined(&devs)) < 0) { + if ((data.ndevs = virMdevctlListDefined(&data.devs)) < 0) { virReportSystemError(errno, "%s", _("failed to query mdevs from mdevctl")); return -1; } - for (i = 0; i < ndevs; i++) { + for (i = 0; i < data.ndevs; i++) { virNodeDeviceObjPtr obj; virObjectEventPtr event; - virNodeDeviceDefPtr dev = devs[i]; + virNodeDeviceDefPtr dev = data.devs[i]; bool new_device = true; dev->driver = g_strdup("vfio_mdev"); @@ -1218,5 +1268,12 @@ nodeDeviceUpdateMediatedDevices(void) virObjectEventStateQueue(driver->nodeDeviceEventState, event); } + /* 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 */ + virNodeDeviceObjListForEach(driver->devs, removeMissingPersistentMdevs, &data); + + g_free(data.devs); + return 0; } -- 2.26.2

The udev thread handles received udev events. This is accomplished by setting dataReady to 'true' and signalling the thread. We also want to use this thread to handle mdev events, so we'll need to add another variable to indicate which event has woken the thread. To prepare for this, rename 'dataReady' so that it is clear that when this variable is set, the udev event needs to be handled. Mdevctl events will use a different variable name. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 223ee5a2ff..d7f7ab4370 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -65,7 +65,7 @@ struct _udevEventData { virThread th; virCond threadCond; bool threadQuit; - bool dataReady; + bool udevReady; }; static virClassPtr udevEventDataClass; @@ -1809,7 +1809,7 @@ udevEventMonitorSanityCheck(udevEventDataPtr priv, * NB: Some older distros, such as CentOS 6, libudev opens sockets * without the NONBLOCK flag which might cause issues with event * based algorithm. Although the issue can be mitigated by resetting - * priv->dataReady for each event found; however, the scheduler issues + * priv->udevReady for each event found; however, the scheduler issues * would still come into play. */ static void @@ -1821,7 +1821,7 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) /* continue rather than break from the loop on non-fatal errors */ while (1) { virObjectLock(priv); - while (!priv->dataReady && !priv->threadQuit) { + while (!priv->udevReady && !priv->threadQuit) { if (virCondWait(&priv->threadCond, &priv->parent.lock)) { virReportSystemError(errno, "%s", _("handler failed to wait on condition")); @@ -1858,11 +1858,11 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) return; } - /* Trying to move the reset of the @priv->dataReady flag to + /* Trying to move the reset of the @priv->udevReady flag to * after the udev_monitor_receive_device wouldn't help much * due to event mgmt and scheduler timing. */ virObjectLock(priv); - priv->dataReady = false; + priv->udevReady = false; virObjectUnlock(priv); continue; @@ -1892,7 +1892,7 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, if (!udevEventMonitorSanityCheck(priv, fd)) priv->threadQuit = true; else - priv->dataReady = true; + priv->udevReady = true; virCondSignal(&priv->threadCond); virObjectUnlock(priv); -- 2.26.2

We need to periodically query mdevctl for changes to device definitions since an administrator can define new devices with 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 directories for changes to files. When a change is detected, we query mdevctl and update our device list. The mdevctl querying is handled by the existing udev thread. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 204 ++++++++++++++++++++++++----- 1 file changed, 171 insertions(+), 33 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d7f7ab4370..5729fea264 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,10 @@ struct _udevEventData { virCond threadCond; bool threadQuit; bool udevReady; + bool mdevReady; + + GList *mdevctlMonitors; + int mdevctlTimeout; }; static virClassPtr udevEventDataClass; @@ -85,6 +90,7 @@ 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->mdevctlMonitors, g_object_unref); virCondDestroy(&priv->threadCond); } @@ -1821,7 +1827,7 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) /* continue rather than break from the loop on non-fatal errors */ while (1) { virObjectLock(priv); - while (!priv->udevReady && !priv->threadQuit) { + while (!priv->udevReady && !priv->mdevReady && !priv->threadQuit) { if (virCondWait(&priv->threadCond, &priv->parent.lock)) { virReportSystemError(errno, "%s", _("handler failed to wait on condition")); @@ -1835,46 +1841,58 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) return; } - errno = 0; - device = udev_monitor_receive_device(priv->udev_monitor); - virObjectUnlock(priv); + if (priv->udevReady) { + errno = 0; + device = udev_monitor_receive_device(priv->udev_monitor); + virObjectUnlock(priv); - if (!device) { - if (errno == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to receive device from udev monitor")); - return; - } + if (!device) { + if (errno == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to receive device from udev monitor")); + return; + } + + /* POSIX allows both EAGAIN and EWOULDBLOCK to be used + * interchangeably when the read would block or timeout was fired + */ + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR + if (errno != EAGAIN && errno != EWOULDBLOCK) { + VIR_WARNINGS_RESET + virReportSystemError(errno, "%s", + _("failed to receive device from udev " + "monitor")); + return; + } + + /* Trying to move the reset of the @priv->udevReady flag to + * after the udev_monitor_receive_device wouldn't help much + * due to event mgmt and scheduler timing. */ + virObjectLock(priv); + priv->udevReady = false; + virObjectUnlock(priv); - /* POSIX allows both EAGAIN and EWOULDBLOCK to be used - * interchangeably when the read would block or timeout was fired - */ - VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR - if (errno != EAGAIN && errno != EWOULDBLOCK) { - VIR_WARNINGS_RESET - virReportSystemError(errno, "%s", - _("failed to receive device from udev " - "monitor")); - return; + continue; } - /* Trying to move the reset of the @priv->udevReady flag to - * after the udev_monitor_receive_device wouldn't help much - * due to event mgmt and scheduler timing. */ - virObjectLock(priv); - priv->udevReady = false; - virObjectUnlock(priv); + udevHandleOneDevice(device); + udev_device_unref(device); - continue; + /* Instead of waiting for the next event after processing @device + * data, let's keep reading from the udev monitor and only wait + * for the next event once either a EAGAIN or a EWOULDBLOCK error + * is encountered. */ } - udevHandleOneDevice(device); - udev_device_unref(device); + if (priv->mdevReady) { + virObjectUnlock(priv); + if (nodeDeviceUpdateMediatedDevices() < 0) + VIR_WARN("mdevctl failed to updated mediated devices"); - /* Instead of waiting for the next event after processing @device - * data, let's keep reading from the udev monitor and only wait - * for the next event once either a EAGAIN or a EWOULDBLOCK error - * is encountered. */ + virObjectLock(priv); + priv->mdevReady = false; + virObjectUnlock(priv); + } } } @@ -1899,6 +1917,117 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, } +static void +scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque) +{ + udevEventDataPtr priv = opaque; + + if (priv->mdevctlTimeout > 0) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + virObjectLock(priv); + priv->mdevReady = true; + virCondSignal(&priv->threadCond); + virObjectUnlock(priv); +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor, + GFile *file, + GFile *other_file, + 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(udevEventDataPtr 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))) { + if (error->code == G_IO_ERROR_NOT_DIRECTORY) + return NULL; + 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; + GFile *child; + GList *child_monitors = NULL; + + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error)) + goto error; + if (!info) + break; + + 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); + return NULL; +} + + +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 (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); + priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors); + } + + /* 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); +} + + /* DMI is intel-compatible specific */ #if defined(__x86_64__) || defined(__i386__) || defined(__amd64__) static void @@ -2052,6 +2181,7 @@ nodeStateInitialize(bool privileged, udevEventDataPtr priv = NULL; struct udev *udev = NULL; virThread enumThread; + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2153,6 +2283,14 @@ 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 */ + priv->mdevctlMonitors = monitorFileRecursively(priv, mdevctlConfigDir); + virObjectUnlock(priv); /* Create a fictional 'computer' device to root the device tree. */ -- 2.26.2

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 | 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 | 17 +++- 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, 239 insertions(+), 23 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 a473563857..3e57e9522a 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -132,6 +132,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 375b907852..15eb70218a 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -765,6 +765,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 cf31f937d5..8fae4352ff 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -877,6 +877,7 @@ LIBVIRT_6.10.0 { global: virDomainAuthorizedSSHKeysGet; virDomainAuthorizedSSHKeysSet; + 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 0bebd534d0..4fbe8743b4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -698,9 +698,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; @@ -718,7 +722,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; } - cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL); @@ -729,6 +733,22 @@ 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) { @@ -749,6 +769,26 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) } +static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid) +{ + int status; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlDefineCommand(def, uuid); + 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) @@ -1068,6 +1108,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; + g_autofree char *uuid = 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) < 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); + + 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 4315f6d6ed..e58bb0f124 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); +virNodeDevicePtr +nodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -116,6 +121,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 5729fea264..2532c3189e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2338,6 +2338,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 b0af3ee88e..f826b6f73e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8665,6 +8665,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 2df38cef77..d2250502b4 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2142,6 +2142,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: @@ -6714,5 +6723,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425 + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, + + /** + * @generate: both + * @acl: node_device:write + */ + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 426 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 9bcd14603d..565c1673f1 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; }; @@ -3588,4 +3595,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 426, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 0020273d9e..30108272f1 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 8bc464d59f..981276d180 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 @@ -347,15 +372,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) @@ -378,6 +406,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.2

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 --inactive --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c virsh # nodedev-list --active --cap mdev mdev_bd2ea955_3402_4252_8c17_7468083a0f26 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; --- tools/virsh-nodedev.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 35117585ff..e3261747e3 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,27 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) int ncaps = 0; virshNodeDeviceListPtr list = NULL; int cap_type = -1; + bool inactive, all; + inactive = vshCommandOptBool(cmd, "inactive"); + 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)) { + 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 +498,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.2

Add a virsh command that maps to virNodeDeviceDefineXML(). Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- tools/virsh-nodedev.c | 58 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index e3261747e3..07d48bbfbe 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1014,6 +1014,58 @@ cmdNodeDeviceEvent(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-define" command + */ +static const vshCmdInfo info_node_device_define[] = { + {.name = "help", + .data = N_("Define a device by an xml file on a node") + }, + {.name = "desc", + .data = N_("Defines a persistent device on the node that can be " + "assigned to a domain. The device must be started before " + "it can be assigned to a domain.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_define[] = { + VIRSH_COMMON_OPT_FILE(N_("file containing an XML description " + "of the device")), + {.name = NULL} +}; + +static bool +cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) +{ + virNodeDevicePtr dev = NULL; + const char *from = NULL; + bool ret = true; + char *buffer; + virshControlPtr priv = ctl->privData; + + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) + return false; + + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) + return false; + + dev = virNodeDeviceDefineXML(priv->conn, buffer, 0); + VIR_FREE(buffer); + + if (dev != NULL) { + vshPrintExtra(ctl, _("Node device %s defined from %s\n"), + virNodeDeviceGetName(dev), from); + virNodeDeviceFree(dev); + } else { + vshError(ctl, _("Failed to define node device from %s"), from); + ret = false; + } + + return ret; +} + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1067,5 +1119,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_event, .flags = 0 }, + {.name = "nodedev-define", + .handler = cmdNodeDeviceDefine, + .opts = opts_node_device_define, + .info = info_node_device_define, + .flags = 0 + }, {.name = NULL} }; -- 2.26.2

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 981276d180..56265cfaea 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 { @@ -134,19 +136,21 @@ testMdevctlStartHelper(const void *data) mdevxml, cmdlinefile, jsonfile); } +typedef virCommandPtr (*GetStopUndefineCmdFunc)(const char *uuid); +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 *cmdlinefile = - g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", - abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid); + cmd = func(uuid); if (!cmd) goto cleanup; @@ -158,7 +162,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; @@ -168,6 +172,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) { @@ -385,8 +410,15 @@ mymain(void) #define DO_TEST_DEFINE(filename) \ DO_TEST_START_FULL("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.2

On Thu, Dec 24, 2020 at 08:14:42AM -0600, Jonathon Jongsma wrote: ^This is a new command, so there should not introduce any alias. We tend to add aliases when at some point we realize the design was a mistake and we need the virsh argument names, e.g. for virt-admin commands Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;

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; --- tools/virsh-nodedev.c | 61 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 97d2a34056..861c3259f1 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1123,6 +1123,61 @@ 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 = "name", + .type = VSH_OT_ALIAS, + .help = "device" + }, + {.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; + virNodeDevicePtr device; + bool ret = true; + virshControlPtr 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, @@ -1188,5 +1243,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.2

Most of the header files no longer use the space-padded function name and parameter alignment. Update the style to be more consistent with other headers. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- include/libvirt/libvirt-nodedev.h | 79 ++++++++++++++++--------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 71626bd837..663e8a9e5b 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -48,15 +48,14 @@ typedef struct _virNodeDevice virNodeDevice; typedef virNodeDevice *virNodeDevicePtr; -int virNodeNumOfDevices (virConnectPtr conn, - const char *cap, - unsigned int flags); - -int virNodeListDevices (virConnectPtr conn, - const char *cap, - char **const names, - int maxnames, - unsigned int flags); +int virNodeNumOfDevices(virConnectPtr conn, + const char *cap, + unsigned int flags); + +int virNodeListDevices(virConnectPtr conn, + const char *cap, + char **const names, + int maxnames, unsigned int flags); /* * virConnectListAllNodeDevices: * @@ -91,46 +90,50 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE = 1 << 30, /* Active devices */ } virConnectListAllNodeDeviceFlags; -int virConnectListAllNodeDevices (virConnectPtr conn, - virNodeDevicePtr **devices, - unsigned int flags); +int virConnectListAllNodeDevices(virConnectPtr conn, + virNodeDevicePtr **devices, + unsigned int flags); -virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn, - const char *name); +virNodeDevicePtr virNodeDeviceLookupByName(virConnectPtr conn, + const char *name); -virNodeDevicePtr virNodeDeviceLookupSCSIHostByWWN (virConnectPtr conn, - const char *wwnn, - const char *wwpn, - unsigned int flags); +virNodeDevicePtr virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); -const char * virNodeDeviceGetName (virNodeDevicePtr dev); +const char *virNodeDeviceGetName(virNodeDevicePtr dev); -const char * virNodeDeviceGetParent (virNodeDevicePtr dev); +const char *virNodeDeviceGetParent(virNodeDevicePtr dev); -int virNodeDeviceNumOfCaps (virNodeDevicePtr dev); +int virNodeDeviceNumOfCaps(virNodeDevicePtr dev); -int virNodeDeviceListCaps (virNodeDevicePtr dev, - char **const names, - int maxnames); +int virNodeDeviceListCaps(virNodeDevicePtr dev, + char **const names, + int maxnames); -char * virNodeDeviceGetXMLDesc (virNodeDevicePtr dev, - unsigned int flags); +char * virNodeDeviceGetXMLDesc(virNodeDevicePtr dev, + unsigned int flags); -int virNodeDeviceRef (virNodeDevicePtr dev); -int virNodeDeviceFree (virNodeDevicePtr dev); +int virNodeDeviceRef(virNodeDevicePtr dev); -int virNodeDeviceDettach (virNodeDevicePtr dev); -int virNodeDeviceDetachFlags(virNodeDevicePtr dev, - const char *driverName, - unsigned int flags); -int virNodeDeviceReAttach (virNodeDevicePtr dev); -int virNodeDeviceReset (virNodeDevicePtr dev); +int virNodeDeviceFree(virNodeDevicePtr dev); -virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn, - const char *xmlDesc, - unsigned int flags); +int virNodeDeviceDettach(virNodeDevicePtr dev); -int virNodeDeviceDestroy (virNodeDevicePtr dev); +int virNodeDeviceDetachFlags(virNodeDevicePtr dev, + const char *driverName, + unsigned int flags); + +int virNodeDeviceReAttach(virNodeDevicePtr dev); + +int virNodeDeviceReset(virNodeDevicePtr dev); + +virNodeDevicePtr virNodeDeviceCreateXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + +int virNodeDeviceDestroy(virNodeDevicePtr dev); virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, -- 2.26.2