Re: [PATCH v1 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit

It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_driver.h | 2 +- src/node_device/node_device_driver.c | 6 +-- src/node_device/node_device_udev.c | 72 ++++++++++++++-------------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f195cfef9d49..2781ad136d68 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, bool defined); int -nodeDeviceUpdateMediatedDevices(void); +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver); void nodeDeviceGenerateName(virNodeDeviceDef *def, diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index f623339dc973..59c5f9b417a4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj, int -nodeDeviceUpdateMediatedDevices(void) +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver) { g_autofree virNodeDeviceDef **defs = NULL; g_autofree virNodeDeviceDef **act_defs = NULL; @@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void) /* Any mdevs that were previously defined but were not returned in the * latest mdevctl query should be removed from the device list */ data.defs = defs; - virNodeDeviceObjListForEachRemove(driver->devs, + virNodeDeviceObjListForEachRemove(node_driver->devs, removeMissingPersistentMdev, &data); for (i = 0; i < data.ndefs; i++) @@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device, cleanup: virNodeDeviceObjEndAPI(&obj); if (updated) - nodeDeviceUpdateMediatedDevices(); + nodeDeviceUpdateMediatedDevices(driver); return ret; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 38740033a66e..e4b1532dc385 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -361,7 +361,7 @@ udevTranslatePCIIds(unsigned int vendor, static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)

{ virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev; @@ -372,8 +372,8 @@ udevProcessPCI(struct udev_device *device, char *p; bool privileged = false; - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - privileged = driver->privileged; + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + privileged = driver_state->privileged; } pci_dev->klass = -1; @@ -1391,12 +1391,12 @@ udevGetDeviceType(struct udev_device *device, static int -udevGetDeviceDetails(struct udev_device *device, +udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)

{ switch (def->caps->data.type) { case VIR_NODE_DEV_CAP_PCI_DEV: - return udevProcessPCI(device, def); + return udevProcessPCI(driver_state, device, def); case VIR_NODE_DEV_CAP_USB_DEV: return udevProcessUSBDevice(device, def); case VIR_NODE_DEV_CAP_USB_INTERFACE: @@ -1447,13 +1447,13 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force); static int -udevRemoveOneDeviceSysPath(const char *path) +udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, const char *path)

{ virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; virObjectEvent *event = NULL; - if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, path))) { VIR_DEBUG("Failed to find device to remove that has udev path '%s'", path); return -1; @@ -1474,21 +1474,21 @@ udevRemoveOneDeviceSysPath(const char *path) } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver_state->devs, obj); } virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + scheduleMdevctlUpdate(driver_state->privateData, false); } - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; } static int -udevSetParent(struct udev_device *device, +udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device,

virNodeDeviceDef *def) { struct udev_device *parent_device = NULL; @@ -1511,7 +1511,7 @@ udevSetParent(struct udev_device *device, return -1; } - if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); def->parent = g_strdup(objdef->name); @@ -1529,7 +1529,7 @@ udevSetParent(struct udev_device *device, } static int -udevAddOneDevice(struct udev_device *device) +udevAddOneDevice(virNodeDeviceDriverState *driver_state, struct udev_device *device)

{ g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1560,15 +1560,15 @@ udevAddOneDevice(struct udev_device *device) if (udevGetDeviceNodes(device, def) != 0) goto cleanup; - if (udevGetDeviceDetails(device, def) != 0) + if (udevGetDeviceDetails(driver_state, device, def) != 0) goto cleanup; - if (udevSetParent(device, def) != 0) + if (udevSetParent(driver_state, device, def) != 0) goto cleanup; is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV; - if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(driver_state->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj); if (is_mdev) @@ -1586,7 +1586,7 @@ udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver_state->devs, def))) goto cleanup; /* @def is now owned by @obj */ def = NULL; @@ -1606,15 +1606,15 @@ udevAddOneDevice(struct udev_device *device) virNodeDeviceObjEndAPI(&obj); if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { + scheduleMdevctlUpdate(driver_state->privateData, false); } } /* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that * the 'created' event is emitted. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } @@ -1622,7 +1622,7 @@ udevAddOneDevice(struct udev_device *device) ret = 0; cleanup: - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); if (ret != 0) { VIR_DEBUG("Discarding device %d %p %s", ret, def, @@ -1635,7 +1635,7 @@ udevAddOneDevice(struct udev_device *device) static int -udevProcessDeviceListEntry(struct udev *udev, +udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev *udev,

struct udev_list_entry *list_entry) { struct udev_device *device; @@ -1647,7 +1647,7 @@ udevProcessDeviceListEntry(struct udev *udev, device = udev_device_new_from_syspath(udev, name); if (device != NULL) { - if (udevAddOneDevice(device) != 0) { + if (udevAddOneDevice(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1685,7 +1685,7 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) static int -udevEnumerateDevices(struct udev *udev) +udevEnumerateDevices(virNodeDeviceDriverState *driver_state, struct udev *udev)

{ struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1701,7 +1701,7 @@ udevEnumerateDevices(struct udev *udev) udev_list_entry_foreach(list_entry, udev_enumerate_get_list_entry(udev_enumerate)) { - udevProcessDeviceListEntry(udev, list_entry); + udevProcessDeviceListEntry(driver_state, udev, list_entry); } ret = 0; @@ -1756,12 +1756,12 @@ udevHandleOneDevice(struct udev_device *device) VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + return udevAddOneDevice(driver, device); if (STREQ(action, "remove")) { const char *path = udev_device_get_syspath(device); - return udevRemoveOneDeviceSysPath(path); + return udevRemoveOneDeviceSysPath(driver, path); } if (STREQ(action, "move")) { @@ -1770,10 +1770,10 @@ udevHandleOneDevice(struct udev_device *device) if (devpath_old) { g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - udevRemoveOneDeviceSysPath(devpath_old_fixed); + udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); } - return udevAddOneDevice(device); + return udevAddOneDevice(driver, device); } return 0; @@ -1995,15 +1995,15 @@ udevSetupSystemDev(void) static void nodeStateInitializeEnumerate(void *opaque) { - struct udev *udev = opaque; udevEventData *priv = driver->privateData; + struct udev *udev = opaque;

/* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) + if (udevEnumerateDevices(driver, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices() != 0) + if (nodeDeviceUpdateMediatedDevices(driver) != 0) goto error; cleanup: @@ -2051,9 +2051,11 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) +mdevctlUpdateThreadFunc(void *opaque) { - if (nodeDeviceUpdateMediatedDevices() < 0) + virNodeDeviceDriverState *driver_state = opaque; + + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) VIR_WARN("mdevctl failed to update mediated devices"); } @@ -2070,7 +2072,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) } if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, NULL) < 0) { + "mdevctl-thread", false, driver) < 0) { virReportSystemError(errno, "%s", _("failed to create mdevctl thread")); }