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@linux.ibm.com>
---
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"));
}
--
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
On 4/19/24 16:49, Marc Hartmayer wrote:
> 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@linux.ibm.com>
> ---
> 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(-)
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
> 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@linux.ibm.com>
> ---
> 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)
While there are exceptions, the general coding style is to have a single
argument per line for function definitions.
> {
> 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)
same
> {
> 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)
same
> {
> 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,
same
> 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)
same
> {
> 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,
same
> 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)
same
> {
> 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;
unnecessary change?
>
> /* 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"));
> }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
> On 4/19/24 9:49 AM, Marc Hartmayer wrote:
>> 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.
>>
[…snip…]
>>
>>
>> static int
>> -udevProcessPCI(struct udev_device *device,
>> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device,
>> virNodeDeviceDef *def)
>
> While there are exceptions, the general coding style is to have a single
> argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for
Libvirt? :/
[…snip…]
>> 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;
>
> unnecessary change?
Will remove.
>
>>
>> /* 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"));
>> }
>
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Thanks.
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote: > On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote: > > On 4/19/24 9:49 AM, Marc Hartmayer wrote: > >> 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. > >> > > […snip…] > > >> > >> > >> static int > >> -udevProcessPCI(struct udev_device *device, > >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, > >> virNodeDeviceDef *def) > > > > While there are exceptions, the general coding style is to have a single > > argument per line for function definitions. > > Okay. BTW, why is there no .clangformat configuration available for > Libvirt? :/ There is no combination of clangformat settings that can match libvirt code style. If we were startnig again from scratch we would of course want to match a defined clangformat style, but unless we're willing to bulk reformat the codebase we are stuck with what we have. With 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 :| _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote: >> On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote: >> > On 4/19/24 9:49 AM, Marc Hartmayer wrote: >> >> 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. >> >> >> >> […snip…] >> >> >> >> >> >> >> static int >> >> -udevProcessPCI(struct udev_device *device, >> >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, >> >> virNodeDeviceDef *def) >> > >> > While there are exceptions, the general coding style is to have a single >> > argument per line for function definitions. >> >> Okay. BTW, why is there no .clangformat configuration available for >> Libvirt? :/ > > There is no combination of clangformat settings that can match > libvirt code style. If we were startnig again from scratch we > would of course want to match a defined clangformat style, > but unless we're willing to bulk reformat the codebase we are > stuck with what we have. Hmm, is the downside of doing a full codebase reformat greater than always doing the code formatting manually? Probably it is, otherwise it would have already be done :) If we would have such a big commit we could list it in a `.git-blame-ignore-revs` file so it will ignored by git blames. > > > With 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote: > On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote: > >> On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote: > >> > On 4/19/24 9:49 AM, Marc Hartmayer wrote: > >> >> 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. > >> >> > >> > >> […snip…] > >> > >> >> > >> >> > >> >> static int > >> >> -udevProcessPCI(struct udev_device *device, > >> >> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, > >> >> virNodeDeviceDef *def) > >> > > >> > While there are exceptions, the general coding style is to have a single > >> > argument per line for function definitions. > >> > >> Okay. BTW, why is there no .clangformat configuration available for > >> Libvirt? :/ > > > > There is no combination of clangformat settings that can match > > libvirt code style. If we were startnig again from scratch we > > would of course want to match a defined clangformat style, > > but unless we're willing to bulk reformat the codebase we are > > stuck with what we have. > > Hmm, is the downside of doing a full codebase reformat greater than > always doing the code formatting manually? Probably it is, otherwise it > would have already be done :) > > If we would have such a big commit we could list it in a > `.git-blame-ignore-revs` file so it will ignored by git blames. Yes, that's great for git blame. The bigger problem though is that a bulk reformat will immediately kill the ability of distro maintainers to cleanly cherry-pick patches across the big reformat commit. Cherry-picking the reformat likely won't be clean either, so they'll be faced with many patches needing manual editting. The tricky question is whether it is none the less worthwhile doing it. The distro maintainer pain will be very real, but also somewhat timelimited, as the need to backport fixes to a given release as it ages. With 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 :| _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote: >> On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: >> > On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote: >> >> On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote: >> >> > On 4/19/24 9:49 AM, Marc Hartmayer wrote: >> >> >> It's better practice for all functions called by the threads to >> >> >pass the driver […snip…] >> >> Hmm, is the downside of doing a full codebase reformat greater than >> always doing the code formatting manually? Probably it is, otherwise it >> would have already be done :) >> >> If we would have such a big commit we could list it in a >> `.git-blame-ignore-revs` file so it will ignored by git blames. > > Yes, that's great for git blame. The bigger problem though is that > a bulk reformat will immediately kill the ability of distro > maintainers to cleanly cherry-pick patches across the big reformat > commit. Cherry-picking the reformat likely won't be clean either, > so they'll be faced with many patches needing manual editting. Yes, but isn't that already the case most of the time? But since I do not backport libvirt fixes, I cannot answer this for sure :) Anyhow, we shouldn't misuse this mail thread for the side discussion :) Is it worth starting a separate thread for it or is the answer a clear NACK? > > The tricky question is whether it is none the less worthwhile doing > it. The distro maintainer pain will be very real, but also somewhat > timelimited, as the need to backport fixes to a given release > as it ages. If the people doing the backporting are more or less libvirt developers, it might be a good trade for them in the long run. > > With 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 :| > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 4/23/24 3:41 AM, Marc Hartmayer wrote: > On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: >> On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote: >>> On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote: >>>> On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote: >>>>> On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote: >>>>>> On 4/19/24 9:49 AM, Marc Hartmayer wrote: >>>>>>> It's better practice for all functions called by the threads to >>>>>> pass the driver > > […snip…] > >>> >>> Hmm, is the downside of doing a full codebase reformat greater than >>> always doing the code formatting manually? Probably it is, otherwise it >>> would have already be done :) >>> >>> If we would have such a big commit we could list it in a >>> `.git-blame-ignore-revs` file so it will ignored by git blames. >> >> Yes, that's great for git blame. The bigger problem though is that >> a bulk reformat will immediately kill the ability of distro >> maintainers to cleanly cherry-pick patches across the big reformat >> commit. Cherry-picking the reformat likely won't be clean either, >> so they'll be faced with many patches needing manual editting. > > Yes, but isn't that already the case most of the time? But since I do > not backport libvirt fixes, I cannot answer this for sure :) As a downstream package maintainer, I always shake my fist at these types of reformat changes when they break clean cherry-picks. But I also understand progress, modernization, etc can't be hamstrung by downstream convenience :-). Regards, Jim > > Anyhow, we shouldn't misuse this mail thread for the side discussion :) > Is it worth starting a separate thread for it or is the answer a clear > NACK? > >> >> The tricky question is whether it is none the less worthwhile doing >> it. The distro maintainer pain will be very real, but also somewhat >> timelimited, as the need to backport fixes to a given release >> as it ages. > > If the people doing the backporting are more or less libvirt developers, > it might be a good trade for them in the long run. > >> >> With 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 :| >> _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2026 Red Hat, Inc.