On a Tuesday in 2020, Jonathon Jongsma wrote:

This adds some internal API to query for persistent mediated devices that are defined by mdevctl. Following commits will make use of this information. This just provides the infrastructure and tests for this feature. One test verifies that we are executing mdevctl with the proper arguments, and other tests verify that we can parse the returned JSON and convert it to our own internal node device representations.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 156 ++++++++++++++++++ src/node_device/node_device_driver.h | 13 ++ src/node_device/node_device_udev.c | 19 +-- .../mdevctl-list-defined.argv | 1 + .../mdevctl-list-multiple-parents.json | 59 +++++++ .../mdevctl-list-multiple-parents.out.xml | 39 +++++ .../mdevctl-list-multiple.json | 59 +++++++ .../mdevctl-list-multiple.out.xml | 39 +++++ .../mdevctl-list-single-noattr.json | 11 ++ .../mdevctl-list-single-noattr.out.xml | 8 + .../mdevctl-list-single.json | 31 ++++ .../mdevctl-list-single.out.xml | 14 ++ tests/nodedevmdevctltest.c | 98 +++++++++++ 13 files changed, 531 insertions(+), 16 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple-parents.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple-parents.out.xml create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml create mode 100644 tests/nodedevmdevctldata/mdevctl-list-single-noattr.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-single-noattr.out.xml create mode 100644 tests/nodedevmdevctldata/mdevctl-list-single.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-single.out.xml

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index f766fd9f32..3b042e9a45 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -812,6 +812,137 @@ virMdevctlStop(virNodeDeviceDefPtr def) }

+virCommandPtr +nodeDeviceGetMdevctlListCommand(bool defined, + char **output) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "list", + "--dumpjson", + NULL); + + if (defined) + virCommandAddArg(cmd, "--defined"); + + virCommandSetOutputBuffer(cmd, output); + + return cmd; +} + + +static void mdevGenerateDeviceName(virNodeDeviceDefPtr dev) +{ + nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); +} + +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDefPtr **devs) +{ + int n; + g_autoptr(virJSONValue) json_devicelist = NULL; + virNodeDeviceDefPtr *outdevs = NULL; + size_t noutdevs = 0; + size_t i, j, k, m; + + json_devicelist = virJSONValueFromString(jsonstring); + + if (!json_devicelist) + goto parsefailure; + + if (!virJSONValueIsArray(json_devicelist)) + goto parsefailure; + + n = virJSONValueArraySize(json_devicelist); + + for (i = 0; i < n; i++) { + virJSONValuePtr obj = virJSONValueArrayGet(json_devicelist, i); + int nparents; + + if (!virJSONValueIsObject(obj)) + goto parsefailure; + + nparents = virJSONValueObjectKeysNumber(obj); + + for (j = 0; j < nparents; j++) { + const char *parent = virJSONValueObjectGetKey(obj, j); + virJSONValuePtr child_array = virJSONValueObjectGetValue(obj, j); + int nchildren; + + if (!virJSONValueIsArray(child_array)) + goto parsefailure; + + nchildren = virJSONValueArraySize(child_array); + + for (k = 0; k < nchildren; k++) {

This is nested quite deep. Maybe a helper function like 'nodeDeviceParseMdevctlSingleDevJSON' would help get rid of it.

+ virNodeDevCapMdevPtr mdev; + const char *uuid; + virJSONValuePtr props; + g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); + virJSONValuePtr child_obj = virJSONValueArrayGet(child_array, k); + + /* the child object should have a single key equal to its uuid. + * The value is an object describing the properties of the mdev */ + if (virJSONValueObjectKeysNumber(child_obj) != 1) + goto parsefailure; + + uuid = virJSONValueObjectGetKey(child_obj, 0); + props = virJSONValueObjectGetValue(child_obj, 0); + + child->parent = g_strdup(parent); + child->caps = g_new0(virNodeDevCapsDef, 1); + child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; + + mdev = &child->caps->data.mdev; + mdev->uuid = g_strdup(uuid); + mdev->type = + g_strdup(virJSONValueObjectGetString(props, "mdev_type")); + + virJSONValuePtr attrs = virJSONValueObjectGet(props, "attrs"); + + if (attrs && virJSONValueIsArray(attrs)) { + int nattrs = virJSONValueArraySize(attrs); + + mdev->attributes = g_new0(virMediatedDeviceAttrPtr, nattrs); + mdev->nattributes = nattrs; + + for (m = 0; m < nattrs; m++) { + virJSONValuePtr attr = virJSONValueArrayGet(attrs, m); + virMediatedDeviceAttrPtr attribute; + + if (!virJSONValueIsObject(attr) || + virJSONValueObjectKeysNumber(attr) != 1) + goto parsefailure; + + attribute = g_new0(virMediatedDeviceAttr, 1); + attribute->name = g_strdup(virJSONValueObjectGetKey(attr, 0)); + virJSONValuePtr value = virJSONValueObjectGetValue(attr, 0); + attribute->value = g_strdup(virJSONValueGetString(value)); + mdev->attributes[m] = attribute; + } + } + mdevGenerateDeviceName(child); +

+ if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) + child = NULL;

The indentation is off here. Note that VIR_APPEND_ELEMENT aborts on failure and NULL's the element on success, so the assignment here is pointless.

+ } + } + } + + *devs = outdevs; + return noutdevs; + + parsefailure:

s/parsefailure/error/

+ for (i = 0; i < noutdevs; i++) + virNodeDeviceDefFree(outdevs[i]); + VIR_FREE(outdevs); + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to parse JSON response")); + return -1; +} + + int nodeDeviceDestroy(virNodeDevicePtr device) { @@ -931,3 +1062,28 @@ nodedevRegister(void) # endif #endif } + + +void +nodeDeviceGenerateName(virNodeDeviceDefPtr def, + const char *subsystem, + const char *sysname, + const char *s) +{ + size_t i; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s_%s", + subsystem, + sysname); + + if (s != NULL) + virBufferAsprintf(&buf, "_%s", s); + + def->name = virBufferContentAndReset(&buf); + + for (i = 0; i < strlen(def->name); i++) { + if (!(g_ascii_isalnum(*(def->name + i)))) + *(def->name + i) = '_'; + } +}

+static int +testMdevctlParse(const void *data) +{ + g_autofree char *buf = NULL; + const char *filename = data; + g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s.json", + abs_srcdir, filename); + g_autofree char *xmloutfile = g_strdup_printf("%s/nodedevmdevctldata/%s.out.xml", + abs_srcdir, filename); + g_autofree char *actualxml = NULL;

Fails to compile with clang: ../tests/nodedevmdevctltest.c:190:22: error: unused variable 'actualxml' [-Werror,-Wunused-variable] g_autofree char *actualxml = NULL; ^ 1 error generated. Jano

+ int ret = -1; + int nmdevs = 0; + virNodeDeviceDefPtr *mdevs = NULL; + virBuffer xmloutbuf = VIR_BUFFER_INITIALIZER; + size_t i;

Bjoern Walk <bwalk@linux.ibm.com> [2020-10-15, 09:41PM +0200]:

Jonathon Jongsma <jjongsma@redhat.com> [2020-08-18, 09:47AM -0500]:

...

diff --git a/tests/nodedevmdevctldata/mdevctl-list-single.out.xml b/tests/nodedevmdevctldata/mdevctl-list-single.out.xml new file mode 100644 index 0000000000..cb443346ef --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-single.out.xml @@ -0,0 +1,14 @@ +<device> + <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> + <parent>matrix</parent> + <capability type='mdev'> + <type id='vfio_ap-passthrough'/> + <iommuGroup number='0'/> + <attr name='assign_adapter' value='5'/> + <attr name='assign_adapter' value='6'/> + <attr name='assign_domain' value='0xab'/> + <attr name='assign_control_domain' value='0xab'/> + <attr name='assign_domain' value='4'/> + <attr name='assign_control_domain' value='4'/> + </capability> +</device>

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>

I just figured that patches that implement that were not even sent upstream... We will do so shortly. I would however welcome a discussion on a sane and consistent XML design for both series.

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

-- 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

On Tue, Aug 18, 2020 at 09:47:56AM -0500, Jonathon Jongsma wrote:

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.

I think this patch should come a bit later where there already are the necessary backing functionality present.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- 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;

^This attribute should live in the object instead...

};

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;

...and be set outside of the JSON parsing.

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);

Once we move the persistent attribute to the 'obj' instead, we can then simplify ^this whole bit to merely: if (obj->persitent) { VIR_FREE(def->sysfs_path); virNodeDeviceObjSetActive(obj, false); } I'd also like to see the commentary above enhanced with an explanation that we only need to free the sysfs path, since that part is the only one that is dynamic, so the rest doesn't change per the definition.

+ 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);

^This 'else' branch could be moved to the "persistence" check above as demonstrated. Erik

On Tue, Aug 18, 2020 at 09:47:57AM -0500, Jonathon Jongsma wrote:

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@redhat.com> --- ...

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));

I'm a tiny bit lost here, what scenario does ^this address? Erik

On Tue, Aug 18, 2020 at 09:47:58AM -0500, Jonathon Jongsma wrote:

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@redhat.com> --- 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);

Indentation is off here.

+ + +} + +/* 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; +}

This suggestion will result in more code, but in the long run I think it is the better solution and is definitely consistent with other areas of libvirt where we iterate obj lists and have to apply modifications. Time complexity wise, honestly it's hard to tell whether it would actually be more efficient (there are a number of factors in play), I guess it would be about the same. What IMO should be done instead is to extend the API set for the node device obj list by virNodeDeviceObjListForEach and virNodeDeviceObjListRemoveLocked. Then, modify the mdevctl JSON parser to return a hash table of devices instead of a linked list. Then, use the nodedev list "ForEach" iterator, pass the mdevctl hash table defined devices as payload and then use RemoveLocked to remove elements (which passed the MDEV cap + persistence filter) not found in the mdevctl hash table from driver->devs.

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);

If nothing else, ^this would deserve a helper. Regards, Erik

On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote:

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@redhat.com> --- 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;

Please keep driver state in the driver state struct.

+ +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; +}

I don't see why we need to have a timer that then signals a persistent thread. Why cant this just spawn a throw-away thread to do the work and avoid all the conditional signaling complexity.

+ + +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:

error: is our naming convention

+ if (error) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to monitor directory: %s"), error->message);

This looks questionable - every codepath sets 'error' and we don't want to return NULL without setting an error.

+ + 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

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 Tue, Aug 25, 2020 at 04:55:06PM +0200, Erik Skultety wrote:

On Tue, Aug 25, 2020 at 01:44:40PM +0100, Daniel P. Berrangé wrote:

...

On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote:

...

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@redhat.com> --- 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;

Please keep driver state in the driver state struct.

...

+ +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; +}

I don't see why we need to have a timer that then signals a persistent thread. Why cant this just spawn a throw-away thread to do the work and avoid all the conditional signaling complexity.

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?

Yes, it can but the code is already delaying and merging processing of the events. Also the only person able to inflict such a DoS is root themselves, so I don't think there's risk from malicious attacks here, just broken apps. 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 Wed, 26 Aug 2020 07:48:15 +0200 Erik Skultety <eskultet@redhat.com> wrote:

On Tue, Aug 25, 2020 at 03:59:47PM +0100, Daniel P. Berrangé wrote:

...

On Tue, Aug 25, 2020 at 04:55:06PM +0200, Erik Skultety wrote:

...

On Tue, Aug 25, 2020 at 01:44:40PM +0100, Daniel P. Berrangé wrote:

...

On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote:

...

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@redhat.com> --- 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;

Please keep driver state in the driver state struct.

...

+ +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; +}

I don't see why we need to have a timer that then signals a persistent thread. Why cant this just spawn a throw-away thread to do the work and avoid all the conditional signaling complexity.

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?

Yes, it can but the code is already delaying and merging processing of the events. Also the only person able to inflict such a DoS is root themselves, so I don't think there's risk from malicious attacks here, just broken apps.

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.

Sorry it took a little while to get back to this patch, but I don't really see how spawning throw-away threads makes this much simpler. As Erik mentions, if we spawn new threads for each event, we will need a mechanism to serialize these threads and maintain coherent ordering of events, etc. I agree that the DoS issue isn't something that we need to protect against, since it require root access to exploit, but the ordering issue does seem important. There is already a persistent thread to handle udev events. What if I just add the mdevctl functionality to this existing thread? Jonathon

On Tue, Aug 18, 2020 at 09:47:59AM -0500, Jonathon Jongsma wrote:

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@redhat.com> --- 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

Isn't this a glib bug that it sends multiple (even different) events for the same change? Same for CHANGES_DONE_HINT, how come it may not arrive at the end of all the changes? Regards, Erik

+ * 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 18, 2020 at 09:48:00AM -0500, Jonathon Jongsma wrote:

With mediated devices, we can now define persistent node devices that can be started and stopped. In order to take advantage of this, we need an API to define new node devices.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 + src/driver-nodedev.h | 6 ++ src/libvirt-nodedev.c | 42 ++++++++ src/libvirt_public.syms | 4 + src/node_device/node_device_driver.c | 97 +++++++++++++++++-- src/node_device/node_device_driver.h | 10 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 18 +++- src/remote_protocol-structs | 8 ++ src/rpc/gendispatch.pl | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + tests/nodedevmdevctltest.c | 68 +++++++++---- 18 files changed, 241 insertions(+), 25 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 423a583d45..02aa9d9750 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -127,6 +127,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn,

int virNodeDeviceDestroy (virNodeDevicePtr dev);

+virNodeDevicePtr virNodeDeviceDefineXML (virConnectPtr conn, + const char *xmlDesc, + unsigned int flags);

Pre-existing, but this forced space-padded alignment feels weird nowadays and we should use the same policy as for the internal headers - no padding, 1 blank line separating the function declarations. In fact at the end of the header, we're breaking the padded "consistency" anyway. So, I'd suggest to abandon the padding, use a single space as a delimiter and we can send a follow-up to fix the rest. the public side of things looks good to me ...

+LIBVIRT_6.5.0 { + global: + virNodeDeviceDefineXML; +} LIBVIRT_6.0.0;

6.8.0 - this is just a reminder as we've managed to push new symbols referencing an old release once IIRC :)

# .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index affd707a65..16f3537776 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -658,9 +658,13 @@ nodeDeviceFindAddressByName(const char *name) }

-virCommandPtr -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) +/* the mdevctl 'start' and 'define' commands accept almost the exact same + * arguments, so provide a common implementation that can be wrapped by a more + * specific function */ +static virCommandPtr +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDefPtr def, + const char *subcommand, + char **uuid_out) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -678,7 +682,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; }

- cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_pci, "--jsonfile", "/dev/stdin", NULL);

Okay, but could we actually make ^this function the internal "public" gateway to all the mdevctl commands accepting a larger set of arguments and...

@@ -689,11 +693,29 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return cmd; }

+virCommandPtr +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, + char **uuid_out)

...make ^these the static implementations handling the subcommand nuances?

+{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out); +} + +virCommandPtr +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, + char **uuid_out) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out); +} + + + static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlDefineCommon(virNodeDeviceDefPtr def, + virCommandPtr (*func)(virNodeDeviceDefPtr, char**), + char **uuid) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = func(def, uuid); if (!cmd) return -1;

I'm not sure whether this standalone function brings that much value in terms of abstracting code.

@@ -709,6 +731,20 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) }

+static int +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +{ + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlStartCommand, uuid);

I wouldn't mind doing what virMdevctlDefineCommon does right here. In a later patch you're introducing a virMdevctlCreate command which I think should actually be part of this function with a bool selector whether we're starting a transient or a persistent device.

+} + + +static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid) +{ + return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlDefineCommand, uuid);

here too.. [snip]

diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index cee0d913dd..f821622ff6 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -10,10 +10,16 @@

#define VIR_FROM_THIS VIR_FROM_NODEDEV

+typedef enum { + MDEVCTL_CMD_START, + MDEVCTL_CMD_DEFINE, +} MdevctlCmd;

Hmm, with the suggestion I outlined above, we could define all the commands we're going to use as an enum in node_device_driver.h like VIR_ENUM_DECL.

+ struct startTestInfo { const char *virt_type; int create; const char *filename; + MdevctlCmd command; };

/* capture stdin passed to command */ @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual, return virTestCompareToFile(replacedCmdline, filename); }

+ +typedef virCommandPtr (*GetStartDefineCmdFunc)(virNodeDeviceDefPtr, char **); + + static int testMdevctlStart(const char *virt_type, int create, + GetStartDefineCmdFunc get_cmd_func, const char *mdevxml, - const char *startcmdfile, - const char *startjsonfile) + const char *cmdfile, + const char *jsonfile) { g_autoptr(virNodeDeviceDef) def = NULL; virNodeDeviceObjPtr obj = NULL; @@ -66,7 +77,7 @@ testMdevctlStart(const char *virt_type,

/* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid); + cmd = get_cmd_func(def, &uuid);

if (!cmd) goto cleanup; @@ -78,10 +89,10 @@ testMdevctlStart(const char *virt_type, if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup;

- if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0) + if (nodedevCompareToFile(actualCmdline, cmdfile) < 0) goto cleanup;

- if (virTestCompareToFile(stdinbuf, startjsonfile) < 0) + if (virTestCompareToFile(stdinbuf, jsonfile) < 0) goto cleanup;

ret = 0; @@ -96,17 +107,31 @@ static int testMdevctlStartHelper(const void *data) { const struct startTestInfo *info = data; + const char *cmd; + GetStartDefineCmdFunc func; + g_autofree char *mdevxml = NULL; + g_autofree char *cmdlinefile = NULL; + g_autofree char *jsonfile = NULL; + + if (info->command == MDEVCTL_CMD_START) { + cmd = "start"; + func = nodeDeviceGetMdevctlStartCommand; + } else if (info->command == MDEVCTL_CMD_DEFINE) { + cmd = "define"; + func = nodeDeviceGetMdevctlDefineCommand; + } else {

^This would then not be needed and we'd simply call virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)

On a Tuesday in 2020, Jonathon Jongsma wrote:

This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/driver-nodedev.h | 4 ++ src/libvirt-nodedev.c | 36 ++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 68 +++++++++++++++++++ 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 ++ .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 56 ++++++++++++--- 12 files changed, 185 insertions(+), 9 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 02aa9d9750..f7957499ae 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,8 @@ virNodeDevicePtr virNodeDeviceDefineXML (virConnectPtr conn, const char *xmlDesc, unsigned int flags);

+int virNodeDeviceUndefine (virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index 16d787f3fc..e18029ab48 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -79,6 +79,9 @@ typedef virNodeDevicePtr const char *xmlDesc, unsigned int flags);

+typedef int +(*virDrvNodeDeviceUndefine)(virNodeDevicePtr dev); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -119,4 +122,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; + virDrvNodeDeviceUndefine nodeDeviceUndefine; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 1704de929e..eb01ceb8b0 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -803,6 +803,42 @@ virNodeDeviceDefineXML(virConnectPtr conn, }

+/** + * virNodeDeviceUndefine: + * @dev: a device object + * + * Undefine the device object. The virtual device is removed from the host + * operating system. This function may require privileged access. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceUndefine(virNodeDevicePtr dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceUndefine) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceUndefine(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 27c637e37c..35c88ec4b7 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -876,5 +876,6 @@ LIBVIRT_6.0.0 { LIBVIRT_6.5.0 { global: virNodeDeviceDefineXML; + virNodeDeviceUndefine; } 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 16f3537776..60a360e4b5 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -833,6 +833,17 @@ nodeDeviceGetMdevctlStopCommand(const char *uuid)

}

+virCommandPtr +nodeDeviceGetMdevctlUndefineCommand(const char *uuid) +{ + return virCommandNewArgList(MDEVCTL, + "undefine", + "-u", + uuid, + NULL); + +} + static int virMdevctlStop(virNodeDeviceDefPtr def) { @@ -848,6 +859,21 @@ virMdevctlStop(virNodeDeviceDefPtr def) }

+static int +virMdevctlUndefine(virNodeDeviceDefPtr def) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlUndefineCommand(def->caps->data.mdev.uuid); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1093,6 +1119,48 @@ nodeDeviceDefineXML(virConnectPtr conn, }

+int +nodeDeviceUndefine(virNodeDevicePtr device) +{ + int ret = -1; + virNodeDeviceObjPtr obj = NULL; + virNodeDeviceDefPtr def; + g_autofree char *parent = NULL; +

../src/node_device/node_device_driver.c:1128:22: error: unused variable 'parent' [-Werror,-Wunused-variable] g_autofree char *parent = NULL; ^

+ if (nodeDeviceWaitInit() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceUndefineEnsureACL(device->conn, def) < 0) + goto cleanup; +

An incremental build fails for me here: ../src/node_device/node_device_driver.c:1137:9: error: implicit declaration of function 'virNodeDeviceUndefineEnsureACL' is invalid in C99 [-Werror,-Wimplicit-function-declaration] if (virNodeDeviceUndefineEnsureACL(device->conn, def) < 0) ^ ../src/node_device/node_device_driver.c:1137:9: note: did you mean 'virNodeDeviceDefineXMLEnsureACL'? src/access/viraccessapicheck.h:340:12: note: 'virNodeDeviceDefineXMLEnsureACL' declared here extern int virNodeDeviceDefineXMLEnsureACL(virConnectPtr conn, virNodeDeviceDefPtr device); ^ Maybe there's a missing dependency in meson.build?

+ if (virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("node device '%s' is still active"), + def->name); + goto cleanup; + } + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + if (virMdevctlUndefine(def) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to undefine 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, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index cf03e0b3cf..2adb3a8073 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -112,6 +112,9 @@ nodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags);

+int +nodeDeviceUndefine(virNodeDevicePtr dev); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -134,6 +137,9 @@ nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def, virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid);

+virCommandPtr +nodeDeviceGetMdevctlUndefineCommand(const char *uuid); + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output);

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5101fff146..19d6642b21 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2199,6 +2199,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 6.5.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 6.5.0 */ };

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 64d0d0c49b..fc27278c4d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8815,6 +8815,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .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 ddfdb9c084..d668074204 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2148,6 +2148,10 @@ struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; };

+struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; +

/* * Events Register/Deregister: @@ -6680,5 +6684,13 @@ enum remote_procedure { * @acl: node_device:write * @acl: node_device:start */ - REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423 + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423, + + /** + * @generate: both + * @priority: high + * @acl: node_device:stop

For domains, we use domain:delete as the acl for undefine

+ */ + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 424 + }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index c325923eaf..3d450a6de9 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1607,6 +1607,9 @@ struct remote_node_device_define_xml_args { struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3566,4 +3569,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 421, REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 423, + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 424, }; diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv b/tests/nodedevmdevctldata/mdevctl-undefine.argv new file mode 100644 index 0000000000..54717455f7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-undefine.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ undefine -u d76a6b78-45ed-4149-a325-005f9abc5281 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index f821622ff6..216c70f1b8 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,30 @@ testMdevctlStop(const void *data) return ret; }

+static int +testMdevctlUuidCommandHelper(const void *data)

Please separate the test refactors from the API addition. Jano

+{ + 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 if (info->command == MDEVCTL_CMD_UNDEFINE) { + cmd = "undefine"; + func = nodeDeviceGetMdevctlUndefineCommand; + } 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) { @@ -391,8 +419,18 @@ 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_UNDEFINE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE)

#define DO_TEST_LIST_DEFINED() \ do { \ @@ -422,6 +460,8 @@ mymain(void) DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");

+ DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + done: nodedevTestDriverFree(driver);

-- 2.26.2