[libvirt] [PATCH 00/32] node_device_udev: cleanups
Mostly style changes and refactors. Ján Tomko (32): Initialize ret to -1 in nodeStateInitialize Split out pciaccess (de)initialization node_device_udev: initialize libpciaccess after the driver lock Reformat nodeStateCleanup Do not call nodeStateCleanup on early initialization error Assign node device driver private data earlier udevGetDMIData: remove unused variable udevProcessSCSIHost: use STRSKIP Rewrite usage of StrToLong_ui in udevProcess{PCI,SCSI} Remove udevStrToLong_ull Remove udevStrToLong_ui Remove udevStrToLong_i Do not VIR_STRDUP the string in udevGetDeviceProperty Move udevHasDeviceProperty earlier udevProcessFloppy; remove unnecessary allocation Fix the return value in udevKludgeStorageType Rewrite disk type checking in udevProcessStorage Only return two values in udevGetStringProperty Only return two values in udevGetUintProperty Remove extra allocation in udevGetDeviceSysfsAttr Only return two values in udevGetStringSysfsAttr Only return two values in udevGetIntSysfsAttr Only return two values in udevGetUintSysfsAttr Remove PROPERTY_* constants node_device_udev: switch to using virReportError udevProcessStorage: trim all whitespace from model and vendor Reformat udevProcessRemoveableMedia udevSetupSystemDev: return if allocation fails node_device_udev: remove yoda condition node_device_udev: remove unnecessary ret variables node_device_udev: rename labels to cleanup Add nomatch filters when enumerating udev devices src/node_device/node_device_udev.c | 1156 ++++++++++++++---------------------- src/node_device/node_device_udev.h | 3 - 2 files changed, 437 insertions(+), 722 deletions(-) -- 2.7.3
Most of the code paths had to reset it to -1 and returning 0 was only possible if we made it to the end of the function. Initialize it to -1 and only set it to 0 if we reach the end, as we do in most of libvirt code. --- src/node_device/node_device_udev.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6bff5ba..86862d6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1680,7 +1680,7 @@ static int nodeStateInitialize(bool privileged, { udevPrivate *priv = NULL; struct udev *udev = NULL; - int ret = 0; + int ret = -1; #if defined __s390__ || defined __s390x_ /* On s390(x) system there is no PCI bus. @@ -1696,23 +1696,19 @@ static int nodeStateInitialize(bool privileged, char ebuf[256]; VIR_ERROR(_("Failed to initialize libpciaccess: %s"), virStrerror(pciret, ebuf, sizeof(ebuf))); - ret = -1; goto out; } } #endif - if (VIR_ALLOC(priv) < 0) { - ret = -1; + if (VIR_ALLOC(priv) < 0) goto out; - } priv->watch = -1; priv->privileged = privileged; if (VIR_ALLOC(driver) < 0) { VIR_FREE(priv); - ret = -1; goto out; } @@ -1720,7 +1716,6 @@ static int nodeStateInitialize(bool privileged, VIR_ERROR(_("Failed to initialize mutex for driver")); VIR_FREE(priv); VIR_FREE(driver); - ret = -1; goto out; } @@ -1742,7 +1737,6 @@ static int nodeStateInitialize(bool privileged, if (priv->udev_monitor == NULL) { VIR_FREE(priv); VIR_ERROR(_("udev_monitor_new_from_netlink returned NULL")); - ret = -1; goto out_unlock; } @@ -1762,23 +1756,19 @@ static int nodeStateInitialize(bool privileged, priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, NULL, NULL); - if (priv->watch == -1) { - ret = -1; + if (priv->watch == -1) goto out_unlock; - } /* Create a fictional 'computer' device to root the device tree. */ - if (udevSetupSystemDev() != 0) { - ret = -1; + if (udevSetupSystemDev() != 0) goto out_unlock; - } /* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) { - ret = -1; + if (udevEnumerateDevices(udev) != 0) goto out_unlock; - } + + ret = 0; out_unlock: nodeDeviceUnlock(); -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:38 +0200, Ján Tomko wrote:
Most of the code paths had to reset it to -1 and returning 0 was only possible if we made it to the end of the function.
Initialize it to -1 and only set it to 0 if we reach the end, as we do in most of libvirt code. --- src/node_device/node_device_udev.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)
ACK
Move pci_system_init and pci_system_cleanup into separate functions, to make the conditional compilation easier to read. --- src/node_device/node_device_udev.c | 43 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 86862d6..ebf5145 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1474,6 +1474,18 @@ static int udevEnumerateDevices(struct udev *udev) } +static void udevPCITranslateDeinit(void) +{ +#if defined __s390__ || defined __s390x_ + /* Nothing was initialized, nothing needs to be cleaned up */ +#else + /* pci_system_cleanup returns void */ + pci_system_cleanup(); +#endif + return; +} + + static int nodeStateCleanup(void) { int ret = 0; @@ -1509,13 +1521,7 @@ static int nodeStateCleanup(void) ret = -1; } -#if defined __s390__ || defined __s390x_ - /* Nothing was initialized, nothing needs to be cleaned up */ -#else - /* pci_system_cleanup returns void */ - pci_system_cleanup(); -#endif - + udevPCITranslateDeinit(); return ret; } @@ -1674,14 +1680,8 @@ static int udevSetupSystemDev(void) return ret; } -static int nodeStateInitialize(bool privileged, - virStateInhibitCallback callback ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) +static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) { - udevPrivate *priv = NULL; - struct udev *udev = NULL; - int ret = -1; - #if defined __s390__ || defined __s390x_ /* On s390(x) system there is no PCI bus. * Therefore there is nothing to initialize here. */ @@ -1696,10 +1696,23 @@ static int nodeStateInitialize(bool privileged, char ebuf[256]; VIR_ERROR(_("Failed to initialize libpciaccess: %s"), virStrerror(pciret, ebuf, sizeof(ebuf))); - goto out; + return -1; } } #endif + return 0; +} + +static int nodeStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + udevPrivate *priv = NULL; + struct udev *udev = NULL; + int ret = -1; + + if (udevPCITranslateInit(privileged) < 0) + goto out; if (VIR_ALLOC(priv) < 0) goto out; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:39 +0200, Ján Tomko wrote:
Move pci_system_init and pci_system_cleanup into separate functions, to make the conditional compilation easier to read. --- src/node_device/node_device_udev.c | 43 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 15 deletions(-)
ACK
This will simplify cleanup. --- src/node_device/node_device_udev.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ebf5145..ef8d7af 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1517,11 +1517,12 @@ static int nodeStateCleanup(void) virMutexDestroy(&driver->lock); VIR_FREE(driver); VIR_FREE(priv); + + udevPCITranslateDeinit(); } else { ret = -1; } - udevPCITranslateDeinit(); return ret; } @@ -1711,9 +1712,6 @@ static int nodeStateInitialize(bool privileged, struct udev *udev = NULL; int ret = -1; - if (udevPCITranslateInit(privileged) < 0) - goto out; - if (VIR_ALLOC(priv) < 0) goto out; @@ -1734,6 +1732,11 @@ static int nodeStateInitialize(bool privileged, nodeDeviceLock(); + if (udevPCITranslateInit(privileged) < 0) { + VIR_FREE(priv); + goto out_unlock; + } + /* * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... * -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:40 +0200, Ján Tomko wrote:
This will simplify cleanup. --- src/node_device/node_device_udev.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
ACK. I had to look forward in the series but it indeed helps.
Remove the ret variable and return early if there is no driver. --- src/node_device/node_device_udev.c | 44 +++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ef8d7af..1e11afe 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1488,42 +1488,38 @@ static void udevPCITranslateDeinit(void) static int nodeStateCleanup(void) { - int ret = 0; - udevPrivate *priv = NULL; struct udev_monitor *udev_monitor = NULL; struct udev *udev = NULL; - if (driver) { - nodeDeviceLock(); + if (!driver) + return -1; - priv = driver->privateData; + nodeDeviceLock(); - if (priv->watch != -1) - virEventRemoveHandle(priv->watch); + priv = driver->privateData; - udev_monitor = DRV_STATE_UDEV_MONITOR(driver); + if (priv->watch != -1) + virEventRemoveHandle(priv->watch); - if (udev_monitor != NULL) { - udev = udev_monitor_get_udev(udev_monitor); - udev_monitor_unref(udev_monitor); - } + udev_monitor = DRV_STATE_UDEV_MONITOR(driver); - if (udev != NULL) - udev_unref(udev); + if (udev_monitor != NULL) { + udev = udev_monitor_get_udev(udev_monitor); + udev_monitor_unref(udev_monitor); + } - virNodeDeviceObjListFree(&driver->devs); - nodeDeviceUnlock(); - virMutexDestroy(&driver->lock); - VIR_FREE(driver); - VIR_FREE(priv); + if (udev != NULL) + udev_unref(udev); - udevPCITranslateDeinit(); - } else { - ret = -1; - } + virNodeDeviceObjListFree(&driver->devs); + nodeDeviceUnlock(); + virMutexDestroy(&driver->lock); + VIR_FREE(driver); + VIR_FREE(priv); - return ret; + udevPCITranslateDeinit(); + return 0; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:41 +0200, Ján Tomko wrote:
Remove the ret variable and return early if there is no driver. --- src/node_device/node_device_udev.c | 44 +++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 24 deletions(-)
ACK
If we have not allocated driver yet, there is nothing to cleanup. --- src/node_device/node_device_udev.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1e11afe..c2f503d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1709,21 +1709,21 @@ static int nodeStateInitialize(bool privileged, int ret = -1; if (VIR_ALLOC(priv) < 0) - goto out; + return -1; priv->watch = -1; priv->privileged = privileged; if (VIR_ALLOC(driver) < 0) { VIR_FREE(priv); - goto out; + return -1; } if (virMutexInit(&driver->lock) < 0) { VIR_ERROR(_("Failed to initialize mutex for driver")); VIR_FREE(priv); VIR_FREE(driver); - goto out; + return -1; } nodeDeviceLock(); @@ -1785,7 +1785,6 @@ static int nodeStateInitialize(bool privileged, out_unlock: nodeDeviceUnlock(); - out: if (ret == -1) nodeStateCleanup(); return ret; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:42 +0200, Ján Tomko wrote:
If we have not allocated driver yet, there is nothing to cleanup. --- src/node_device/node_device_udev.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
ACK
We do not need it to track if priv->udev_monitor is non-NULL. --- src/node_device/node_device_udev.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index c2f503d..f443d58 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1499,14 +1499,16 @@ static int nodeStateCleanup(void) priv = driver->privateData; - if (priv->watch != -1) - virEventRemoveHandle(priv->watch); + if (priv) { + if (priv->watch != -1) + virEventRemoveHandle(priv->watch); - udev_monitor = DRV_STATE_UDEV_MONITOR(driver); + udev_monitor = DRV_STATE_UDEV_MONITOR(driver); - if (udev_monitor != NULL) { - udev = udev_monitor_get_udev(udev_monitor); - udev_monitor_unref(udev_monitor); + if (udev_monitor != NULL) { + udev = udev_monitor_get_udev(udev_monitor); + udev_monitor_unref(udev_monitor); + } } if (udev != NULL) @@ -1726,12 +1728,11 @@ static int nodeStateInitialize(bool privileged, return -1; } + driver->privateData = priv; nodeDeviceLock(); - if (udevPCITranslateInit(privileged) < 0) { - VIR_FREE(priv); + if (udevPCITranslateInit(privileged) < 0) goto out_unlock; - } /* * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... @@ -1747,16 +1748,12 @@ static int nodeStateInitialize(bool privileged, priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { - VIR_FREE(priv); VIR_ERROR(_("udev_monitor_new_from_netlink returned NULL")); goto out_unlock; } udev_monitor_enable_receiving(priv->udev_monitor); - /* udev can be retrieved from udev_monitor */ - driver->privateData = priv; - /* We register the monitor with the event callback so we are * notified by udev of device changes before we enumerate existing * devices because libvirt will simply recreate the device if we -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:43 +0200, Ján Tomko wrote:
We do not need it to track if priv->udev_monitor is non-NULL.
I don't quite understand what you meant to say with this.
--- src/node_device/node_device_udev.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
ACK.
A variable without use is pointless. Remove it, since we have no use for it. --- src/node_device/node_device_udev.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f443d58..59bc452 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1576,7 +1576,6 @@ udevGetDMIData(virNodeDevCapDataPtr data) { struct udev *udev = NULL; struct udev_device *device = NULL; - char *tmp = NULL; udev = udev_monitor_get_udev(DRV_STATE_UDEV_MONITOR(driver)); @@ -1637,7 +1636,6 @@ udevGetDMIData(virNodeDevCapDataPtr data) } out: - VIR_FREE(tmp); if (device != NULL) udev_device_unref(device); return; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:44 +0200, Ján Tomko wrote:
A variable without use is pointless.
Remove it, since we have no use for it. --- src/node_device/node_device_udev.c | 2 -- 1 file changed, 2 deletions(-)
ACK
Instead of separating it into STRPEFIX and str + strlen. --- src/node_device/node_device_udev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 59bc452..7d111c4 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -712,16 +712,17 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, int ret = -1; virNodeDevCapDataPtr data = &def->caps->data; char *filename = NULL; + char *str; filename = last_component(def->sysfs_path); - if (!STRPREFIX(filename, "host")) { + if (!(str = STRSKIP(filename, "host"))) { VIR_ERROR(_("SCSI host found, but its udev name '%s' does " "not begin with 'host'"), filename); goto out; } - if (udevStrToLong_ui(filename + strlen("host"), + if (udevStrToLong_ui(str, NULL, 0, &data->scsi_host.host) == -1) { -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:45 +0200, Ján Tomko wrote:
Instead of separating it into STRPEFIX and str + strlen. --- src/node_device/node_device_udev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
ACK
Use virStrToLong_ui instead of udevStrToLong_ui, reformat the code and report a more specific error message. --- src/node_device/node_device_udev.c | 80 ++++++++++---------------------------- 1 file changed, 21 insertions(+), 59 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7d111c4..2e2ed96 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -429,33 +429,14 @@ static int udevProcessPCI(struct udev_device *device, goto out; } - p = strrchr(syspath, '/'); - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 16, - &data->pci_dev.domain) == -1)) { - goto out; - } - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 16, - &data->pci_dev.bus) == -1)) { - goto out; - } - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 16, - &data->pci_dev.slot) == -1)) { - goto out; - } - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 16, - &data->pci_dev.function) == -1)) { + if ((p = strrchr(syspath, '/')) == NULL || + virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.domain) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.bus) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.slot) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.function) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse the PCI address from sysfs path: '%s'"), + syspath); goto out; } @@ -716,16 +697,11 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, filename = last_component(def->sysfs_path); - if (!(str = STRSKIP(filename, "host"))) { - VIR_ERROR(_("SCSI host found, but its udev name '%s' does " - "not begin with 'host'"), filename); - goto out; - } - - if (udevStrToLong_ui(str, - NULL, - 0, - &data->scsi_host.host) == -1) { + if (!(str = STRSKIP(filename, "host")) || + virStrToLong_ui(str, NULL, 0, &data->scsi_host.host) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse SCSI host '%s'"), + filename); goto out; } @@ -831,28 +807,14 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, filename = last_component(def->sysfs_path); - if (udevStrToLong_ui(filename, &p, 10, &data->scsi.host) == -1) - goto out; - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 10, - &data->scsi.bus) == -1)) { - goto out; - } - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 10, - &data->scsi.target) == -1)) { - goto out; - } - - if ((p == NULL) || (udevStrToLong_ui(p+1, - &p, - 10, - &data->scsi.lun) == -1)) { - goto out; + if (virStrToLong_ui(filename, &p, 10, &data->scsi.host) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 10, &data->scsi.bus) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 10, &data->scsi.target) < 0 || p == NULL || + virStrToLong_ui(p + 1, &p, 10, &data->scsi.lun) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse the SCSI address from filename: '%s'"), + filename); + return -1; } switch (udevGetUintSysfsAttr(device, "type", &tmp, 0)) { -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:46 +0200, Ján Tomko wrote:
Use virStrToLong_ui instead of udevStrToLong_ui, reformat the code and report a more specific error message. --- src/node_device/node_device_udev.c | 80 ++++++++++---------------------------- 1 file changed, 21 insertions(+), 59 deletions(-)
ACK
The wrapper adds an error message or a debug log. Since we already log the properties we get from udev as strings, there is no much use for the debug logs. Open code the error message and delete the function. --- src/node_device/node_device_udev.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 2e2ed96..ff9668c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -57,23 +57,6 @@ struct _udevPrivate { bool privileged; }; -static int udevStrToLong_ull(char const *s, - char **end_ptr, - int base, - unsigned long long *result) -{ - int ret = 0; - - ret = virStrToLong_ull(s, end_ptr, base, result); - if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), s); - } else { - VIR_DEBUG("Converted '%s' to unsigned long %llu", s, *result); - } - - return ret; -} - static int udevStrToLong_ui(char const *s, char **end_ptr, @@ -300,8 +283,10 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); if (ret == PROPERTY_FOUND) { - if (udevStrToLong_ull(udev_value, NULL, 0, value) != 0) + if (virStrToLong_ull(udev_value, NULL, 0, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), udev_value); ret = PROPERTY_ERROR; + } } VIR_FREE(udev_value); -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:47 +0200, Ján Tomko wrote:
The wrapper adds an error message or a debug log.
Since we already log the properties we get from udev as strings, there is no much use for the debug logs.
Open code the error message and delete the function. --- src/node_device/node_device_udev.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-)
ACK
Remove the debug message, open code the error in the two udevGetUint callers and use a more specific error in SCSI and PCI processing. --- src/node_device/node_device_udev.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ff9668c..5b341d9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -58,23 +58,6 @@ struct _udevPrivate { }; -static int udevStrToLong_ui(char const *s, - char **end_ptr, - int base, - unsigned int *result) -{ - int ret = 0; - - ret = virStrToLong_ui(s, end_ptr, base, result); - if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); - } else { - VIR_DEBUG("Converted '%s' to unsigned int %u", s, *result); - } - - return ret; -} - static int udevStrToLong_i(char const *s, char **end_ptr, int base, @@ -165,8 +148,10 @@ static int udevGetUintProperty(struct udev_device *udev_device, ret = udevGetDeviceProperty(udev_device, property_key, &udev_value); if (ret == PROPERTY_FOUND) { - if (udevStrToLong_ui(udev_value, NULL, base, value) != 0) + if (virStrToLong_ui(udev_value, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to unsigned int"), udev_value); ret = PROPERTY_ERROR; + } } VIR_FREE(udev_value); @@ -264,8 +249,10 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); if (ret == PROPERTY_FOUND) { - if (udevStrToLong_ui(udev_value, NULL, base, value) != 0) + if (virStrToLong_ui(udev_value, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to unsigned int"), udev_value); ret = PROPERTY_ERROR; + } } VIR_FREE(udev_value); -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:48 +0200, Ján Tomko wrote:
Remove the debug message, open code the error in the two udevGetUint callers and use a more specific error in SCSI and PCI processing. --- src/node_device/node_device_udev.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
ACK
Open code the error message. --- src/node_device/node_device_udev.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5b341d9..b1e733b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -58,23 +58,6 @@ struct _udevPrivate { }; -static int udevStrToLong_i(char const *s, - char **end_ptr, - int base, - int *result) -{ - int ret = 0; - - ret = virStrToLong_i(s, end_ptr, base, result); - if (ret != 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), s); - } else { - VIR_DEBUG("Converted '%s' to int %u", s, *result); - } - - return ret; -} - /* This function allocates memory from the heap for the property * value. That memory must be later freed by some other code. */ static int udevGetDeviceProperty(struct udev_device *udev_device, @@ -128,8 +111,10 @@ static int udevGetIntProperty(struct udev_device *udev_device, ret = udevGetDeviceProperty(udev_device, property_key, &udev_value); if (ret == PROPERTY_FOUND) { - if (udevStrToLong_i(udev_value, NULL, base, value) != 0) + if (virStrToLong_i(udev_value, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to int"), udev_value); ret = PROPERTY_ERROR; + } } VIR_FREE(udev_value); @@ -229,8 +214,10 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); if (ret == PROPERTY_FOUND) { - if (udevStrToLong_i(udev_value, NULL, base, value) != 0) + if (virStrToLong_i(udev_value, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to int"), udev_value); ret = PROPERTY_ERROR; + } } VIR_FREE(udev_value); -- 2.7.3
Two out of three callers free it right after converting it to a number. Also change the comment at the beginning of the function, because the comment inside the function told me to. --- src/node_device/node_device_udev.c | 72 ++++++++++++-------------------------- 1 file changed, 23 insertions(+), 49 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b1e733b..5d78527 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -58,36 +58,16 @@ struct _udevPrivate { }; -/* This function allocates memory from the heap for the property - * value. That memory must be later freed by some other code. */ -static int udevGetDeviceProperty(struct udev_device *udev_device, - const char *property_key, - char **property_value) +static const char *udevGetDeviceProperty(struct udev_device *udev_device, + const char *property_key) { - const char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *ret = NULL; - udev_value = udev_device_get_property_value(udev_device, property_key); - if (udev_value == NULL) { - VIR_DEBUG("udev reports device '%s' does not have property '%s'", - udev_device_get_sysname(udev_device), property_key); - ret = PROPERTY_MISSING; - goto out; - } + ret = udev_device_get_property_value(udev_device, property_key); - /* If this allocation is changed, the comment at the beginning - * of the function must also be changed. */ - if (VIR_STRDUP(*property_value, udev_value) < 0) { - ret = PROPERTY_ERROR; - goto out; - } + VIR_DEBUG("Found property key '%s' value '%s' for device with sysname '%s'", + property_key, NULLSTR(ret), udev_device_get_sysname(udev_device)); - VIR_DEBUG("Found property key '%s' value '%s' " - "for device with sysname '%s'", - property_key, *property_value, - udev_device_get_sysname(udev_device)); - - out: return ret; } @@ -96,7 +76,11 @@ static int udevGetStringProperty(struct udev_device *udev_device, const char *property_key, char **value) { - return udevGetDeviceProperty(udev_device, property_key, value); + if (VIR_STRDUP(*value, + udevGetDeviceProperty(udev_device, property_key)) < 0) + return PROPERTY_ERROR; + + return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } @@ -105,20 +89,15 @@ static int udevGetIntProperty(struct udev_device *udev_device, int *value, int base) { - char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *str = NULL; - ret = udevGetDeviceProperty(udev_device, property_key, &udev_value); + str = udevGetDeviceProperty(udev_device, property_key); - if (ret == PROPERTY_FOUND) { - if (virStrToLong_i(udev_value, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), udev_value); - ret = PROPERTY_ERROR; - } + if (str && virStrToLong_i(str, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to int"), str); + return PROPERTY_ERROR; } - - VIR_FREE(udev_value); - return ret; + return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } @@ -127,20 +106,15 @@ static int udevGetUintProperty(struct udev_device *udev_device, unsigned int *value, int base) { - char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *str = NULL; - ret = udevGetDeviceProperty(udev_device, property_key, &udev_value); + str = udevGetDeviceProperty(udev_device, property_key); - if (ret == PROPERTY_FOUND) { - if (virStrToLong_ui(udev_value, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), udev_value); - ret = PROPERTY_ERROR; - } + if (str && virStrToLong_ui(str, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to int"), str); + return PROPERTY_ERROR; } - - VIR_FREE(udev_value); - return ret; + return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:50 +0200, Ján Tomko wrote:
Two out of three callers free it right after converting it to a number.
Also change the comment at the beginning of the function, because the comment inside the function told me to. --- src/node_device/node_device_udev.c | 72 ++++++++++++-------------------------- 1 file changed, 23 insertions(+), 49 deletions(-)
ACK
--- src/node_device/node_device_udev.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5d78527..d7a49f8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -58,6 +58,18 @@ struct _udevPrivate { }; +static bool +udevHasDeviceProperty(struct udev_device *dev, + const char *key) +{ + if (udev_device_get_property_value(dev, key)) + return true; + + return false; +} + + + static const char *udevGetDeviceProperty(struct udev_device *udev_device, const char *property_key) { @@ -1084,16 +1096,6 @@ udevProcessSCSIGeneric(struct udev_device *dev, return 0; } -static bool -udevHasDeviceProperty(struct udev_device *dev, - const char *key) -{ - if (udev_device_get_property_value(dev, key)) - return true; - - return false; -} - static int udevGetDeviceType(struct udev_device *device, virNodeDevCapType *type) -- 2.7.3
Use udevHasDeviceProperty instead of udevGetStringProperty. We do not need to copy the string since we do not need it. Also add braces around the if body, since the change made syntax check complain. --- src/node_device/node_device_udev.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d7a49f8..46e48df 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -892,17 +892,14 @@ static int udevProcessFloppy(struct udev_device *device, { int tmp_int = 0; int has_media = 0; - char *tmp_str = NULL; if ((udevGetIntProperty(device, "DKD_MEDIA_AVAILABLE", - &tmp_int, 0) == PROPERTY_FOUND)) + &tmp_int, 0) == PROPERTY_FOUND)) { /* USB floppy */ has_media = tmp_int; - else if (udevGetStringProperty(device, "ID_FS_LABEL", - &tmp_str) == PROPERTY_FOUND) { + } else if (udevHasDeviceProperty(device, "ID_FS_LABEL")) { /* Legacy floppy */ has_media = 1; - VIR_FREE(tmp_str); } return udevProcessRemoveableMedia(device, def, has_media); -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:52 +0200, Ján Tomko wrote:
Use udevHasDeviceProperty instead of udevGetStringProperty. We do not need to copy the string since we do not need it.
Also add braces around the if body, since the change made syntax check complain. --- src/node_device/node_device_udev.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
ACK
Since the switch to VIR_STRDUP this function returns 1 on success, but the caller treats any non-zero value as failure. --- src/node_device/node_device_udev.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 46e48df..4506ff8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -940,28 +940,22 @@ static int udevProcessSD(struct udev_device *device, * storage device it is from other information that is provided. */ static int udevKludgeStorageType(virNodeDeviceDefPtr def) { - int ret = -1; - VIR_DEBUG("Could not find definitive storage type for device " "with sysfs path '%s', trying to guess it", def->sysfs_path); - if (STRPREFIX(def->caps->data.storage.block, "/dev/vd")) { - /* virtio disk */ - ret = VIR_STRDUP(def->caps->data.storage.drive_type, "disk"); - } - - if (ret != 0) { - VIR_DEBUG("Could not determine storage type for device " - "with sysfs path '%s'", def->sysfs_path); - } else { + /* virtio disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/vd") && + VIR_STRDUP(def->caps->data.storage.drive_type, "disk") > 0) { VIR_DEBUG("Found storage type '%s' for device " "with sysfs path '%s'", def->caps->data.storage.drive_type, def->sysfs_path); + return 0; } - - return ret; + VIR_DEBUG("Could not determine storage type " + "for device with sysfs path '%s'", def->sysfs_path); + return -1; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:53 +0200, Ján Tomko wrote:
Since the switch to VIR_STRDUP this function returns 1 on success, but the caller treats any non-zero value as failure. --- src/node_device/node_device_udev.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
ACK
Error out on parsing errors and use a local const char pointer instead of chained ifs to check whether we found a match. --- src/node_device/node_device_udev.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4506ff8..35d0dc7 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1020,30 +1020,34 @@ static int udevProcessStorage(struct udev_device *device, "ID_TYPE", &data->storage.drive_type) != PROPERTY_FOUND || STREQ(def->caps->data.storage.drive_type, "generic")) { - int tmp_int = 0; + int val = 0; + const char *str = NULL; /* All floppy drives have the ID_DRIVE_FLOPPY prop. This is * needed since legacy floppies don't have a drive_type */ - if (udevGetIntProperty(device, "ID_DRIVE_FLOPPY", - &tmp_int, 0) == PROPERTY_FOUND && - tmp_int == 1) { + if (udevGetIntProperty(device, "ID_DRIVE_FLOPPY", &val, 0) == PROPERTY_ERROR) + goto out; + else if (val == 1) + str = "floppy"; - if (VIR_STRDUP(data->storage.drive_type, "floppy") < 0) + if (!str) { + if (udevGetIntProperty(device, "ID_CDROM", &val, 0) == PROPERTY_ERROR) goto out; - } else if (udevGetIntProperty(device, "ID_CDROM", - &tmp_int, 0) == PROPERTY_FOUND && - tmp_int == 1) { + else if (val == 1) + str = "cd"; + } - if (VIR_STRDUP(data->storage.drive_type, "cd") < 0) + if (!str) { + if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) == PROPERTY_ERROR) goto out; - } else if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", - &tmp_int, 0) == PROPERTY_FOUND && - tmp_int == 1) { + if (val == 1) + str = "sd"; + } - if (VIR_STRDUP(data->storage.drive_type, "sd") < 0) + if (str) { + if (VIR_STRDUP(data->storage.drive_type, str) < 0) goto out; } else { - /* If udev doesn't have it, perhaps we can guess it. */ if (udevKludgeStorageType(def) != 0) goto out; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:54 +0200, Ján Tomko wrote:
Error out on parsing errors and use a local const char pointer instead of chained ifs to check whether we found a match. --- src/node_device/node_device_udev.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
You didn't help in making the code more clear but didn't make it worse either. ACK
There is no need to differentiate between PROPERTY_FOUND and PROPERTY_MISSING - we can just look if the string is non-NULL. --- src/node_device/node_device_udev.c | 87 ++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 51 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 35d0dc7..876fca1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -90,9 +90,8 @@ static int udevGetStringProperty(struct udev_device *udev_device, { if (VIR_STRDUP(*value, udevGetDeviceProperty(udev_device, property_key)) < 0) - return PROPERTY_ERROR; - - return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return -1; + return 0; } @@ -474,7 +473,6 @@ static int udevProcessUSBDevice(struct udev_device *device, { virNodeDevCapDataPtr data = &def->caps->data; int ret = -1; - int err; if (udevGetUintProperty(device, "BUSNUM", @@ -497,18 +495,16 @@ static int udevProcessUSBDevice(struct udev_device *device, goto out; } - err = udevGetStringProperty(device, - "ID_VENDOR_FROM_DATABASE", - &data->usb_dev.vendor_name); - if (err == PROPERTY_ERROR) + if (udevGetStringProperty(device, + "ID_VENDOR_FROM_DATABASE", + &data->usb_dev.vendor_name) < 0) + goto out; + + if (!data->usb_dev.vendor_name && + udevGetStringSysfsAttr(device, + "manufacturer", + &data->usb_dev.vendor_name) == PROPERTY_ERROR) goto out; - if (err == PROPERTY_MISSING) { - if (udevGetStringSysfsAttr(device, - "manufacturer", - &data->usb_dev.vendor_name) == PROPERTY_ERROR) { - goto out; - } - } if (udevGetUintProperty(device, "ID_MODEL_ID", @@ -517,18 +513,16 @@ static int udevProcessUSBDevice(struct udev_device *device, goto out; } - err = udevGetStringProperty(device, - "ID_MODEL_FROM_DATABASE", - &data->usb_dev.product_name); - if (err == PROPERTY_ERROR) + if (udevGetStringProperty(device, + "ID_MODEL_FROM_DATABASE", + &data->usb_dev.product_name) < 0) + goto out; + + if (!data->usb_dev.product_name && + udevGetStringSysfsAttr(device, + "product", + &data->usb_dev.product_name) == PROPERTY_ERROR) goto out; - if (err == PROPERTY_MISSING) { - if (udevGetStringSysfsAttr(device, - "product", - &data->usb_dev.product_name) == PROPERTY_ERROR) { - goto out; - } - } if (udevGenerateDeviceName(device, def, NULL) != 0) goto out; @@ -599,9 +593,8 @@ static int udevProcessNetworkInterface(struct udev_device *device, if (udevGetStringProperty(device, "INTERFACE", - &data->net.ifname) == PROPERTY_ERROR) { + &data->net.ifname) < 0) goto out; - } if (udevGetStringSysfsAttr(device, "address", @@ -834,9 +827,8 @@ static int udevProcessRemoveableMedia(struct udev_device *device, VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; if (udevGetStringProperty(device, "ID_FS_LABEL", - &data->storage.media_label) == PROPERTY_ERROR) { + &data->storage.media_label) < 0) goto out; - } if (udevGetUint64SysfsAttr(device, "size", @@ -989,16 +981,11 @@ static int udevProcessStorage(struct udev_device *device, if (VIR_STRDUP(data->storage.block, devnode) < 0) goto out; - if (udevGetStringProperty(device, - "ID_BUS", - &data->storage.bus) == PROPERTY_ERROR) { + if (udevGetStringProperty(device, "ID_BUS", &data->storage.bus) < 0) goto out; - } - if (udevGetStringProperty(device, - "ID_SERIAL", - &data->storage.serial) == PROPERTY_ERROR) { + if (udevGetStringProperty(device, "ID_SERIAL", &data->storage.serial) < 0) goto out; - } + if (udevGetStringSysfsAttr(device, "device/vendor", &data->storage.vendor) == PROPERTY_ERROR) { @@ -1016,9 +1003,10 @@ static int udevProcessStorage(struct udev_device *device, * expected, so I don't see a problem with not having a property * for it. */ - if (udevGetStringProperty(device, - "ID_TYPE", - &data->storage.drive_type) != PROPERTY_FOUND || + if (udevGetStringProperty(device, "ID_TYPE", &data->storage.drive_type) < 0) + goto out; + + if (!data->storage.drive_type || STREQ(def->caps->data.storage.drive_type, "generic")) { int val = 0; const char *str = NULL; @@ -1080,9 +1068,8 @@ static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDefPtr def) { - if (udevGetStringProperty(dev, - "DEVNAME", - &def->caps->data.sg.path) != PROPERTY_FOUND) + if (udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path) < 0 || + !def->caps->data.sg.path) return -1; if (udevGenerateDeviceName(dev, def, NULL) != 0) @@ -1130,9 +1117,10 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_NET; /* SCSI generic device doesn't set DEVTYPE property */ - if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) == - PROPERTY_FOUND && - STREQ(subsystem, "scsi_generic")) + if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) < 0) + return -1; + + if (STREQ_NULLABLE(subsystem, "scsi_generic")) *type = VIR_NODE_DEV_CAP_SCSI_GENERIC; VIR_FREE(subsystem); } @@ -1277,11 +1265,8 @@ static int udevAddOneDevice(struct udev_device *device) if (VIR_STRDUP(def->sysfs_path, udev_device_get_syspath(device)) < 0) goto out; - if (udevGetStringProperty(device, - "DRIVER", - &def->driver) == PROPERTY_ERROR) { + if (udevGetStringProperty(device, "DRIVER", &def->driver) < 0) goto out; - } if (VIR_ALLOC(def->caps) != 0) goto out; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:55 +0200, Ján Tomko wrote:
There is no need to differentiate between PROPERTY_FOUND and PROPERTY_MISSING - we can just look if the string is non-NULL. --- src/node_device/node_device_udev.c | 87 ++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 51 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 35d0dc7..876fca1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -90,9 +90,8 @@ static int udevGetStringProperty(struct udev_device *udev_device, { if (VIR_STRDUP(*value, udevGetDeviceProperty(udev_device, property_key)) < 0) - return PROPERTY_ERROR; - - return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return -1;
Add a newline here.
+ return 0; }
ACK
We only care about the failure, not a missing property. --- src/node_device/node_device_udev.c | 60 ++++++++++++-------------------------- 1 file changed, 18 insertions(+), 42 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 876fca1..d30a450 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -106,9 +106,9 @@ static int udevGetIntProperty(struct udev_device *udev_device, if (str && virStrToLong_i(str, NULL, base, value) < 0) { VIR_ERROR(_("Failed to convert '%s' to int"), str); - return PROPERTY_ERROR; + return -1; } - return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -123,9 +123,9 @@ static int udevGetUintProperty(struct udev_device *udev_device, if (str && virStrToLong_ui(str, NULL, base, value) < 0) { VIR_ERROR(_("Failed to convert '%s' to int"), str); - return PROPERTY_ERROR; + return -1; } - return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -366,12 +366,8 @@ static int udevProcessPCI(struct udev_device *device, syspath = udev_device_get_syspath(device); - if (udevGetUintProperty(device, - "PCI_CLASS", - &data->pci_dev.class, - 16) == PROPERTY_ERROR) { + if (udevGetUintProperty(device, "PCI_CLASS", &data->pci_dev.class, 16) < 0) goto out; - } if ((p = strrchr(syspath, '/')) == NULL || virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.domain) < 0 || p == NULL || @@ -474,26 +470,12 @@ static int udevProcessUSBDevice(struct udev_device *device, virNodeDevCapDataPtr data = &def->caps->data; int ret = -1; - if (udevGetUintProperty(device, - "BUSNUM", - &data->usb_dev.bus, - 10) == PROPERTY_ERROR) { + if (udevGetUintProperty(device, "BUSNUM", &data->usb_dev.bus, 10) < 0) goto out; - } - - if (udevGetUintProperty(device, - "DEVNUM", - &data->usb_dev.device, - 10) == PROPERTY_ERROR) { + if (udevGetUintProperty(device, "DEVNUM", &data->usb_dev.device, 10) < 0) goto out; - } - - if (udevGetUintProperty(device, - "ID_VENDOR_ID", - &data->usb_dev.vendor, - 16) == PROPERTY_ERROR) { + if (udevGetUintProperty(device, "ID_VENDOR_ID", &data->usb_dev.vendor, 16) < 0) goto out; - } if (udevGetStringProperty(device, "ID_VENDOR_FROM_DATABASE", @@ -506,12 +488,8 @@ static int udevProcessUSBDevice(struct udev_device *device, &data->usb_dev.vendor_name) == PROPERTY_ERROR) goto out; - if (udevGetUintProperty(device, - "ID_MODEL_ID", - &data->usb_dev.product, - 16) == PROPERTY_ERROR) { + if (udevGetUintProperty(device, "ID_MODEL_ID", &data->usb_dev.product, 16) < 0) goto out; - } if (udevGetStringProperty(device, "ID_MODEL_FROM_DATABASE", @@ -859,7 +837,6 @@ static int udevProcessCDROM(struct udev_device *device, virNodeDeviceDefPtr def) { int ret = -1; - int tmp_int = 0; int has_media = 0; /* NB: the drive_type string provided by udev is different from @@ -870,9 +847,9 @@ static int udevProcessCDROM(struct udev_device *device, if (VIR_STRDUP(def->caps->data.storage.drive_type, "cdrom") < 0) goto out; - if ((udevGetIntProperty(device, "ID_CDROM_MEDIA", - &tmp_int, 0) == PROPERTY_FOUND)) - has_media = tmp_int; + if (udevHasDeviceProperty(device, "ID_CDROM_MEDIA") && + udevGetIntProperty(device, "ID_CDROM_MEDIA", &has_media, 0) < 0) + goto out; ret = udevProcessRemoveableMedia(device, def, has_media); out: @@ -882,13 +859,12 @@ static int udevProcessCDROM(struct udev_device *device, static int udevProcessFloppy(struct udev_device *device, virNodeDeviceDefPtr def) { - int tmp_int = 0; int has_media = 0; - if ((udevGetIntProperty(device, "DKD_MEDIA_AVAILABLE", - &tmp_int, 0) == PROPERTY_FOUND)) { + if (udevHasDeviceProperty(device, "ID_CDROM_MEDIA")) { /* USB floppy */ - has_media = tmp_int; + if (udevGetIntProperty(device, "DKD_MEDIA_AVAILABLE", &has_media, 0) < 0) + return -1; } else if (udevHasDeviceProperty(device, "ID_FS_LABEL")) { /* Legacy floppy */ has_media = 1; @@ -1013,20 +989,20 @@ static int udevProcessStorage(struct udev_device *device, /* All floppy drives have the ID_DRIVE_FLOPPY prop. This is * needed since legacy floppies don't have a drive_type */ - if (udevGetIntProperty(device, "ID_DRIVE_FLOPPY", &val, 0) == PROPERTY_ERROR) + if (udevGetIntProperty(device, "ID_DRIVE_FLOPPY", &val, 0) < 0) goto out; else if (val == 1) str = "floppy"; if (!str) { - if (udevGetIntProperty(device, "ID_CDROM", &val, 0) == PROPERTY_ERROR) + if (udevGetIntProperty(device, "ID_CDROM", &val, 0) < 0) goto out; else if (val == 1) str = "cd"; } if (!str) { - if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) == PROPERTY_ERROR) + if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) < 0) goto out; if (val == 1) str = "sd"; -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:56 +0200, Ján Tomko wrote:
We only care about the failure, not a missing property.
Well, you made us not care about the missing property in two instances here :).
--- src/node_device/node_device_udev.c | 60 ++++++++++++-------------------------- 1 file changed, 18 insertions(+), 42 deletions(-)
ACK
Most of the code paths free it right after converting it to an integer. --- src/node_device/node_device_udev.c | 98 +++++++++++--------------------------- 1 file changed, 29 insertions(+), 69 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d30a450..e2aeb32 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -129,38 +129,17 @@ static int udevGetUintProperty(struct udev_device *udev_device, } -/* This function allocates memory from the heap for the property - * value. That memory must be later freed by some other code. - * Any control characters that cannot be printed in the XML are stripped - * from the string */ -static int udevGetDeviceSysfsAttr(struct udev_device *udev_device, - const char *attr_name, - char **attr_value) +static const char *udevGetDeviceSysfsAttr(struct udev_device *udev_device, + const char *attr_name) { - const char *udev_value = NULL; - int ret = PROPERTY_FOUND; - - udev_value = udev_device_get_sysattr_value(udev_device, attr_name); - if (udev_value == NULL) { - VIR_DEBUG("udev reports device '%s' does not have sysfs attr '%s'", - udev_device_get_sysname(udev_device), attr_name); - ret = PROPERTY_MISSING; - goto out; - } + const char *ret = NULL; - /* If this allocation is changed, the comment at the beginning - * of the function must also be changed. */ - if (VIR_STRDUP(*attr_value, udev_value) < 0) { - ret = PROPERTY_ERROR; - goto out; - } + ret = udev_device_get_sysattr_value(udev_device, attr_name); VIR_DEBUG("Found sysfs attribute '%s' value '%s' " "for device with sysname '%s'", - attr_name, *attr_value, + attr_name, NULLSTR(ret), udev_device_get_sysname(udev_device)); - - out: return ret; } @@ -169,22 +148,15 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device, const char *attr_name, char **value) { - char *tmp = NULL; - int ret = PROPERTY_MISSING; - - ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &tmp); + if (VIR_STRDUP(*value, udevGetDeviceSysfsAttr(udev_device, attr_name)) < 0) + return PROPERTY_ERROR; - virStringStripControlChars(tmp); + virStringStripControlChars(*value); - if (tmp != NULL && (STREQ(tmp, ""))) { - VIR_FREE(tmp); - tmp = NULL; - ret = PROPERTY_MISSING; - } - - *value = tmp; + if (*value != NULL && (STREQ(*value, ""))) + VIR_FREE(*value); - return ret; + return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } @@ -193,20 +165,16 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, int *value, int base) { - char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *str = NULL; - ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); + str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (ret == PROPERTY_FOUND) { - if (virStrToLong_i(udev_value, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), udev_value); - ret = PROPERTY_ERROR; - } + if (str && virStrToLong_i(str, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to int"), str); + return PROPERTY_ERROR; } - VIR_FREE(udev_value); - return ret; + return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } @@ -215,20 +183,16 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, unsigned int *value, int base) { - char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *str = NULL; - ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); + str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (ret == PROPERTY_FOUND) { - if (virStrToLong_ui(udev_value, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), udev_value); - ret = PROPERTY_ERROR; - } + if (str && virStrToLong_ui(str, NULL, base, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to unsigned int"), str); + return PROPERTY_ERROR; } - VIR_FREE(udev_value); - return ret; + return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } @@ -236,20 +200,16 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, const char *attr_name, unsigned long long *value) { - char *udev_value = NULL; - int ret = PROPERTY_FOUND; + const char *str = NULL; - ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &udev_value); + str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (ret == PROPERTY_FOUND) { - if (virStrToLong_ull(udev_value, NULL, 0, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), udev_value); - ret = PROPERTY_ERROR; - } + if (str && virStrToLong_ull(str, NULL, 0, value) < 0) { + VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), str); + return PROPERTY_ERROR; } - VIR_FREE(udev_value); - return ret; + return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:57 +0200, Ján Tomko wrote:
Most of the code paths free it right after converting it to an integer. --- src/node_device/node_device_udev.c | 98 +++++++++++--------------------------- 1 file changed, 29 insertions(+), 69 deletions(-)
I guess it makes sense to keep the wrapper to log the debug message. ACK
The callers only care for an error, and a missing attribute is simply NULL. --- src/node_device/node_device_udev.c | 78 ++++++++++++-------------------------- 1 file changed, 24 insertions(+), 54 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e2aeb32..6e97ac5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -149,14 +149,14 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device, char **value) { if (VIR_STRDUP(*value, udevGetDeviceSysfsAttr(udev_device, attr_name)) < 0) - return PROPERTY_ERROR; + return -1; virStringStripControlChars(*value); if (*value != NULL && (STREQ(*value, ""))) VIR_FREE(*value); - return *value == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -443,9 +443,8 @@ static int udevProcessUSBDevice(struct udev_device *device, goto out; if (!data->usb_dev.vendor_name && - udevGetStringSysfsAttr(device, - "manufacturer", - &data->usb_dev.vendor_name) == PROPERTY_ERROR) + udevGetStringSysfsAttr(device, "manufacturer", + &data->usb_dev.vendor_name) < 0) goto out; if (udevGetUintProperty(device, "ID_MODEL_ID", &data->usb_dev.product, 16) < 0) @@ -457,9 +456,8 @@ static int udevProcessUSBDevice(struct udev_device *device, goto out; if (!data->usb_dev.product_name && - udevGetStringSysfsAttr(device, - "product", - &data->usb_dev.product_name) == PROPERTY_ERROR) + udevGetStringSysfsAttr(device, "product", + &data->usb_dev.product_name) < 0) goto out; if (udevGenerateDeviceName(device, def, NULL) != 0) @@ -534,11 +532,9 @@ static int udevProcessNetworkInterface(struct udev_device *device, &data->net.ifname) < 0) goto out; - if (udevGetStringSysfsAttr(device, - "address", - &data->net.address) == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "address", + &data->net.address) < 0) goto out; - } if (udevGetUintSysfsAttr(device, "addr_len", @@ -922,17 +918,11 @@ static int udevProcessStorage(struct udev_device *device, if (udevGetStringProperty(device, "ID_SERIAL", &data->storage.serial) < 0) goto out; - if (udevGetStringSysfsAttr(device, - "device/vendor", - &data->storage.vendor) == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "device/vendor", &data->storage.vendor) < 0) goto out; - } udevStripSpaces(def->caps->data.storage.vendor); - if (udevGetStringSysfsAttr(device, - "device/model", - &data->storage.model) == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "device/model", &data->storage.model) < 0) goto out; - } udevStripSpaces(def->caps->data.storage.model); /* There is no equivalent of the hotpluggable property in libudev, * but storage is going toward a world in which hotpluggable is @@ -1403,51 +1393,31 @@ udevGetDMIData(virNodeDevCapDataPtr data) } } - if (udevGetStringSysfsAttr(device, - "product_name", - &data->system.product_name) == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "product_name", + &data->system.product_name) < 0) goto out; - } - if (udevGetStringSysfsAttr(device, - "sys_vendor", - &data->system.hardware.vendor_name) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "sys_vendor", + &data->system.hardware.vendor_name) < 0) goto out; - } - if (udevGetStringSysfsAttr(device, - "product_version", - &data->system.hardware.version) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "product_version", + &data->system.hardware.version) < 0) goto out; - } - if (udevGetStringSysfsAttr(device, - "product_serial", - &data->system.hardware.serial) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "product_serial", + &data->system.hardware.serial) < 0) goto out; - } if (virGetHostUUID(data->system.hardware.uuid)) goto out; - if (udevGetStringSysfsAttr(device, - "bios_vendor", - &data->system.firmware.vendor_name) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "bios_vendor", + &data->system.firmware.vendor_name) < 0) goto out; - } - if (udevGetStringSysfsAttr(device, - "bios_version", - &data->system.firmware.version) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "bios_version", + &data->system.firmware.version) < 0) goto out; - } - if (udevGetStringSysfsAttr(device, - "bios_date", - &data->system.firmware.release_date) - == PROPERTY_ERROR) { + if (udevGetStringSysfsAttr(device, "bios_date", + &data->system.firmware.release_date) < 0) goto out; - } out: if (device != NULL) -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:58 +0200, Ján Tomko wrote:
The callers only care for an error, and a missing attribute is simply NULL. --- src/node_device/node_device_udev.c | 78 ++++++++++++-------------------------- 1 file changed, 24 insertions(+), 54 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e2aeb32..6e97ac5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
ACK
Callers only check for an error or a specific integer value. --- src/node_device/node_device_udev.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6e97ac5..f2310ac 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -171,10 +171,10 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, if (str && virStrToLong_i(str, NULL, base, value) < 0) { VIR_ERROR(_("Failed to convert '%s' to int"), str); - return PROPERTY_ERROR; + return -1; } - return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -322,7 +322,6 @@ static int udevProcessPCI(struct udev_device *device, udevPrivate *priv = driver->privateData; int ret = -1; char *p; - int rc; syspath = udev_device_get_syspath(device); @@ -364,17 +363,12 @@ static int udevProcessPCI(struct udev_device *device, if (udevGenerateDeviceName(device, def, NULL) != 0) goto out; - rc = udevGetIntSysfsAttr(device, - "numa_node", - &data->pci_dev.numa_node, - 10); - if (rc == PROPERTY_ERROR) { + /* The default value is -1, because it can't be 0 + * as zero is valid node number. */ + data->pci_dev.numa_node = -1; + if (udevGetIntSysfsAttr(device, "numa_node", + &data->pci_dev.numa_node, 10) < 0) goto out; - } else if (rc == PROPERTY_MISSING) { - /* The default value is -1, because it can't be 0 - * as zero is valid node number. */ - data->pci_dev.numa_node = -1; - } if (nodeDeviceSysfsGetPCIRelatedDevCaps(syspath, data) < 0) goto out; @@ -748,12 +742,12 @@ static int udevProcessRemoveableMedia(struct udev_device *device, int has_media) { virNodeDevCapDataPtr data = &def->caps->data; - int tmp_int = 0, ret = 0; + int is_removable = 0, ret = 0; - if ((udevGetIntSysfsAttr(device, "removable", &tmp_int, 0) == PROPERTY_FOUND) && - (tmp_int == 1)) { + if (udevGetIntSysfsAttr(device, "removable", &is_removable, 0) < 0) + return -1; + if (is_removable == 1) def->caps->data.storage.flags |= VIR_NODE_DEV_CAP_STORAGE_REMOVABLE; - } if (has_media) { -- 2.7.3
On Mon, Jun 06, 2016 at 11:01:59 +0200, Ján Tomko wrote:
Callers only check for an error or a specific integer value. --- src/node_device/node_device_udev.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
ACK
Open code the call to udev_device_get_sysattr_value in the one place where it's needed. --- src/node_device/node_device_udev.c | 65 +++++++++++--------------------------- 1 file changed, 18 insertions(+), 47 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f2310ac..f97f1eb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -189,10 +189,10 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, if (str && virStrToLong_ui(str, NULL, base, value) < 0) { VIR_ERROR(_("Failed to convert '%s' to unsigned int"), str); - return PROPERTY_ERROR; + return -1; } - return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -339,19 +339,11 @@ static int udevProcessPCI(struct udev_device *device, goto out; } - if (udevGetUintSysfsAttr(device, - "vendor", - &data->pci_dev.vendor, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "vendor", &data->pci_dev.vendor, 16) < 0) goto out; - } - if (udevGetUintSysfsAttr(device, - "device", - &data->pci_dev.product, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "device", &data->pci_dev.product, 16) < 0) goto out; - } if (udevTranslatePCIIds(data->pci_dev.vendor, data->pci_dev.product, @@ -470,33 +462,21 @@ static int udevProcessUSBInterface(struct udev_device *device, int ret = -1; virNodeDevCapDataPtr data = &def->caps->data; - if (udevGetUintSysfsAttr(device, - "bInterfaceNumber", - &data->usb_if.number, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "bInterfaceNumber", + &data->usb_if.number, 16) < 0) goto out; - } - if (udevGetUintSysfsAttr(device, - "bInterfaceClass", - &data->usb_if._class, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "bInterfaceClass", + &data->usb_if._class, 16) < 0) goto out; - } - if (udevGetUintSysfsAttr(device, - "bInterfaceSubClass", - &data->usb_if.subclass, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "bInterfaceSubClass", + &data->usb_if.subclass, 16) < 0) goto out; - } - if (udevGetUintSysfsAttr(device, - "bInterfaceProtocol", - &data->usb_if.protocol, - 16) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "bInterfaceProtocol", + &data->usb_if.protocol, 16) < 0) goto out; - } if (udevGenerateDeviceName(device, def, NULL) != 0) goto out; @@ -530,12 +510,8 @@ static int udevProcessNetworkInterface(struct udev_device *device, &data->net.address) < 0) goto out; - if (udevGetUintSysfsAttr(device, - "addr_len", - &data->net.address_len, - 0) == PROPERTY_ERROR) { + if (udevGetUintSysfsAttr(device, "addr_len", &data->net.address_len, 0) < 0) goto out; - } if (udevGenerateDeviceName(device, def, data->net.address) != 0) goto out; @@ -683,17 +659,12 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, return -1; } - switch (udevGetUintSysfsAttr(device, "type", &tmp, 0)) { - case PROPERTY_FOUND: - if (udevGetSCSIType(def, tmp, &data->scsi.type) == -1) + if (udev_device_get_sysattr_value(device, "type")) { + if (udevGetUintSysfsAttr(device, "type", &tmp, 0) < 0) + goto out; + + if (udevGetSCSIType(def, tmp, &data->scsi.type) < 0) goto out; - break; - case PROPERTY_MISSING: - break; /* No type is not an error */ - case PROPERTY_ERROR: - default: - goto out; - break; } if (udevGenerateDeviceName(device, def, NULL) != 0) -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:00 +0200, Ján Tomko wrote:
Open code the call to udev_device_get_sysattr_value in the one place where it's needed. --- src/node_device/node_device_udev.c | 65 +++++++++++--------------------------- 1 file changed, 18 insertions(+), 47 deletions(-)
ACK
They are no longer used. --- src/node_device/node_device_udev.c | 41 ++++++++++++-------------------------- src/node_device/node_device_udev.h | 3 --- 2 files changed, 13 insertions(+), 31 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f97f1eb..54eb319 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -206,10 +206,10 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, if (str && virStrToLong_ull(str, NULL, 0, value) < 0) { VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), str); - return PROPERTY_ERROR; + return -1; } - return str == NULL ? PROPERTY_MISSING : PROPERTY_FOUND; + return 0; } @@ -687,18 +687,12 @@ static int udevProcessDisk(struct udev_device *device, virNodeDevCapDataPtr data = &def->caps->data; int ret = 0; - if (udevGetUint64SysfsAttr(device, - "size", - &data->storage.num_blocks) == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "size", &data->storage.num_blocks) < 0) goto out; - } - if (udevGetUint64SysfsAttr(device, - "queue/logical_block_size", - &data->storage.logical_block_size) - == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", + &data->storage.logical_block_size) < 0) goto out; - } data->storage.size = data->storage.num_blocks * data->storage.logical_block_size; @@ -729,17 +723,13 @@ static int udevProcessRemoveableMedia(struct udev_device *device, &data->storage.media_label) < 0) goto out; - if (udevGetUint64SysfsAttr(device, - "size", - &data->storage.num_blocks) == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "size", + &data->storage.num_blocks) < 0) goto out; - } - if (udevGetUint64SysfsAttr(device, - "queue/logical_block_size", - &data->storage.logical_block_size) == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", + &data->storage.logical_block_size) < 0) goto out; - } /* XXX This calculation is wrong for the qemu virtual cdrom * which reports the size in 512 byte blocks, but the logical @@ -801,18 +791,13 @@ static int udevProcessSD(struct udev_device *device, virNodeDevCapDataPtr data = &def->caps->data; int ret = 0; - if (udevGetUint64SysfsAttr(device, - "size", - &data->storage.num_blocks) == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "size", + &data->storage.num_blocks) < 0) goto out; - } - if (udevGetUint64SysfsAttr(device, - "queue/logical_block_size", - &data->storage.logical_block_size) - == PROPERTY_ERROR) { + if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", + &data->storage.logical_block_size) < 0) goto out; - } data->storage.size = data->storage.num_blocks * data->storage.logical_block_size; diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index 47eeb1f..9a07ab7 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -29,6 +29,3 @@ typedef struct _udevPrivate udevPrivate; #define DRV_STATE_UDEV_MONITOR(ds) (((udevPrivate *)((ds)->privateData))->udev_monitor) #define DMI_DEVPATH "/sys/devices/virtual/dmi/id" #define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" -#define PROPERTY_FOUND 0 -#define PROPERTY_MISSING 1 -#define PROPERTY_ERROR -1 -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:01 +0200, Ján Tomko wrote:
They are no longer used. --- src/node_device/node_device_udev.c | 41 ++++++++++++-------------------------- src/node_device/node_device_udev.h | 3 --- 2 files changed, 13 insertions(+), 31 deletions(-)
ACK
Also use the more common "Unable to initialize mutex" string and virReportSystemError instead of virStrerror. --- src/node_device/node_device_udev.c | 47 +++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 54eb319..8307b80 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -105,7 +105,8 @@ static int udevGetIntProperty(struct udev_device *udev_device, str = udevGetDeviceProperty(udev_device, property_key); if (str && virStrToLong_i(str, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), str); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to int"), str); return -1; } return 0; @@ -122,7 +123,8 @@ static int udevGetUintProperty(struct udev_device *udev_device, str = udevGetDeviceProperty(udev_device, property_key); if (str && virStrToLong_ui(str, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), str); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to int"), str); return -1; } return 0; @@ -170,7 +172,8 @@ static int udevGetIntSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); if (str && virStrToLong_i(str, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to int"), str); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to int"), str); return -1; } @@ -188,7 +191,8 @@ static int udevGetUintSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); if (str && virStrToLong_ui(str, NULL, base, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), str); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to unsigned int"), str); return -1; } @@ -205,7 +209,8 @@ static int udevGetUint64SysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); if (str && virStrToLong_ull(str, NULL, 0, value) < 0) { - VIR_ERROR(_("Failed to convert '%s' to unsigned long long"), str); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert '%s' to unsigned long long"), str); return -1; } @@ -674,8 +679,9 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, out: if (ret != 0) { - VIR_ERROR(_("Failed to process SCSI device with sysfs path '%s'"), - def->sysfs_path); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to process SCSI device with sysfs path '%s'"), + def->sysfs_path); } return ret; } @@ -1049,7 +1055,8 @@ static int udevGetDeviceDetails(struct udev_device *device, ret = udevProcessSCSIGeneric(device, def); break; default: - VIR_ERROR(_("Unknown device type %d"), def->caps->data.type); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown device type %d"), def->caps->data.type); ret = -1; break; } @@ -1212,7 +1219,7 @@ static int udevEnumerateDevices(struct udev *udev) ret = udev_enumerate_scan_devices(udev_enumerate); if (0 != ret) { - VIR_ERROR(_("udev scan devices returned %d"), ret); + virReportError(VIR_ERR_INTERNAL_ERROR, _("udev scan devices returned %d"), ret); goto out; } @@ -1292,14 +1299,15 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, nodeDeviceLock(); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { - VIR_ERROR(_("File descriptor returned by udev %d does not " + virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned by udev %d does not " "match node device file descriptor %d"), fd, udev_fd); goto out; } device = udev_monitor_receive_device(udev_monitor); if (device == NULL) { - VIR_ERROR(_("udev_monitor_receive_device returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_receive_device returned NULL")); goto out; } @@ -1337,7 +1345,7 @@ udevGetDMIData(virNodeDevCapDataPtr data) if (device == NULL) { device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK); if (device == NULL) { - VIR_ERROR(_("Failed to get udev device for syspath '%s' or '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get udev device for syspath '%s' or '%s'"), DMI_DEVPATH, DMI_DEVPATH_FALLBACK); goto out; } @@ -1417,16 +1425,15 @@ static int udevPCITranslateInit(bool privileged ATTRIBUTE_UNUSED) /* On s390(x) system there is no PCI bus. * Therefore there is nothing to initialize here. */ #else - int pciret; + int rc; - if ((pciret = pci_system_init()) != 0) { + if ((rc = pci_system_init()) != 0) { /* Ignore failure as non-root; udev is not as helpful in that * situation, but a non-privileged user won't benefit much * from udev in the first place. */ if (errno != ENOENT && (privileged || errno != EACCES)) { - char ebuf[256]; - VIR_ERROR(_("Failed to initialize libpciaccess: %s"), - virStrerror(pciret, ebuf, sizeof(ebuf))); + virReportSystemError(rc, "%s", + _("Failed to initialize libpciaccess")); return -1; } } @@ -1454,7 +1461,8 @@ static int nodeStateInitialize(bool privileged, } if (virMutexInit(&driver->lock) < 0) { - VIR_ERROR(_("Failed to initialize mutex for driver")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); VIR_FREE(priv); VIR_FREE(driver); return -1; @@ -1480,7 +1488,8 @@ static int nodeStateInitialize(bool privileged, priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { - VIR_ERROR(_("udev_monitor_new_from_netlink returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_new_from_netlink returned NULL")); goto out_unlock; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:02 +0200, Ján Tomko wrote:
Also use the more common "Unable to initialize mutex" string and virReportSystemError instead of virStrerror. --- src/node_device/node_device_udev.c | 47 +++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 54eb319..8307b80 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
[...]
@@ -1212,7 +1219,7 @@ static int udevEnumerateDevices(struct udev *udev)
ret = udev_enumerate_scan_devices(udev_enumerate); if (0 != ret) { - VIR_ERROR(_("udev scan devices returned %d"), ret); + virReportError(VIR_ERR_INTERNAL_ERROR, _("udev scan devices returned %d"), ret);
Resulting line is too long.
goto out; }
@@ -1292,14 +1299,15 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, nodeDeviceLock(); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { - VIR_ERROR(_("File descriptor returned by udev %d does not " + virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned by udev %d does not " "match node device file descriptor %d"), fd, udev_fd);
Resulting line is too long and formatting is broken.
goto out; }
device = udev_monitor_receive_device(udev_monitor); if (device == NULL) { - VIR_ERROR(_("udev_monitor_receive_device returned NULL")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("udev_monitor_receive_device returned NULL")); goto out; }
@@ -1337,7 +1345,7 @@ udevGetDMIData(virNodeDevCapDataPtr data) if (device == NULL) { device = udev_device_new_from_syspath(udev, DMI_DEVPATH_FALLBACK); if (device == NULL) { - VIR_ERROR(_("Failed to get udev device for syspath '%s' or '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get udev device for syspath '%s' or '%s'"), DMI_DEVPATH, DMI_DEVPATH_FALLBACK);
Too long and broken too.
goto out; }
ACK with the defects fixed.
Use virTrimSpaces instead of a custom implementation. --- src/node_device/node_device_udev.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 8307b80..893c782 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -839,20 +839,6 @@ static int udevKludgeStorageType(virNodeDeviceDefPtr def) } -static void udevStripSpaces(char *s) -{ - if (s == NULL) - return; - - while (virFileStripSuffix(s, " ")) { - /* do nothing */ - ; - } - - return; -} - - static int udevProcessStorage(struct udev_device *device, virNodeDeviceDefPtr def) { @@ -876,10 +862,13 @@ static int udevProcessStorage(struct udev_device *device, if (udevGetStringSysfsAttr(device, "device/vendor", &data->storage.vendor) < 0) goto out; - udevStripSpaces(def->caps->data.storage.vendor); + if (def->caps->data.storage.vendor) + virTrimSpaces(def->caps->data.storage.vendor, NULL); + if (udevGetStringSysfsAttr(device, "device/model", &data->storage.model) < 0) goto out; - udevStripSpaces(def->caps->data.storage.model); + if (def->caps->data.storage.model) + virTrimSpaces(def->caps->data.storage.model, NULL); /* There is no equivalent of the hotpluggable property in libudev, * but storage is going toward a world in which hotpluggable is * expected, so I don't see a problem with not having a property -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:03 +0200, Ján Tomko wrote:
Use virTrimSpaces instead of a custom implementation. --- src/node_device/node_device_udev.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
ACK
Remove unnecessary ret variable and return early if we have no media to save on indentation. --- src/node_device/node_device_udev.c | 45 +++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 893c782..1e96154 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -713,41 +713,40 @@ static int udevProcessRemoveableMedia(struct udev_device *device, int has_media) { virNodeDevCapDataPtr data = &def->caps->data; - int is_removable = 0, ret = 0; + int is_removable = 0; if (udevGetIntSysfsAttr(device, "removable", &is_removable, 0) < 0) return -1; if (is_removable == 1) def->caps->data.storage.flags |= VIR_NODE_DEV_CAP_STORAGE_REMOVABLE; - if (has_media) { + if (!has_media) + return 0; - def->caps->data.storage.flags |= - VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; + def->caps->data.storage.flags |= + VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; - if (udevGetStringProperty(device, "ID_FS_LABEL", - &data->storage.media_label) < 0) - goto out; + if (udevGetStringProperty(device, "ID_FS_LABEL", + &data->storage.media_label) < 0) + return -1; - if (udevGetUint64SysfsAttr(device, "size", - &data->storage.num_blocks) < 0) - goto out; + if (udevGetUint64SysfsAttr(device, "size", + &data->storage.num_blocks) < 0) + return -1; - if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", - &data->storage.logical_block_size) < 0) - goto out; + if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", + &data->storage.logical_block_size) < 0) + return -1; - /* XXX This calculation is wrong for the qemu virtual cdrom - * which reports the size in 512 byte blocks, but the logical - * block size as 2048. I don't have a physical cdrom on a - * devel system to see how they behave. */ - def->caps->data.storage.removable_media_size = - def->caps->data.storage.num_blocks * - def->caps->data.storage.logical_block_size; - } + /* XXX This calculation is wrong for the qemu virtual cdrom + * which reports the size in 512 byte blocks, but the logical + * block size as 2048. I don't have a physical cdrom on a + * devel system to see how they behave. */ + def->caps->data.storage.removable_media_size = + def->caps->data.storage.num_blocks * + def->caps->data.storage.logical_block_size; - out: - return ret; + return 0; } static int udevProcessCDROM(struct udev_device *device, -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:04 +0200, Ján Tomko wrote:
Remove unnecessary ret variable and return early if we have no media to save on indentation. --- src/node_device/node_device_udev.c | 45 +++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 23 deletions(-)
ACK
There is no cleanup to be done. --- src/node_device/node_device_udev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1e96154..484aa5c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1379,8 +1379,8 @@ static int udevSetupSystemDev(void) virNodeDeviceObjPtr dev = NULL; int ret = -1; - if (VIR_ALLOC(def) != 0) - goto out; + if (VIR_ALLOC(def) < 0) + return -1; if (VIR_STRDUP(def->name, "computer") < 0) goto out; -- 2.7.3
--- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 484aa5c..aa4c36e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1206,7 +1206,7 @@ static int udevEnumerateDevices(struct udev *udev) udev_enumerate = udev_enumerate_new(udev); ret = udev_enumerate_scan_devices(udev_enumerate); - if (0 != ret) { + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("udev scan devices returned %d"), ret); goto out; } -- 2.7.3
Remove ret variables and labels from functions where there is no cleanup to be done. --- src/node_device/node_device_udev.c | 108 ++++++++++++++----------------------- 1 file changed, 39 insertions(+), 69 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index aa4c36e..88a333e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -222,7 +222,6 @@ static int udevGenerateDeviceName(struct udev_device *device, virNodeDeviceDefPtr def, const char *s) { - int ret = 0; size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -243,7 +242,7 @@ static int udevGenerateDeviceName(struct udev_device *device, *(def->name + i) = '_'; } - return ret; + return 0; } #if HAVE_UDEV_LOGGING @@ -287,7 +286,6 @@ static int udevTranslatePCIIds(unsigned int vendor, char **vendor_string, char **product_string) { - int ret = -1; struct pci_id_match m; const char *vendor_name = NULL, *device_name = NULL; @@ -306,14 +304,11 @@ static int udevTranslatePCIIds(unsigned int vendor, NULL, NULL); - if (VIR_STRDUP(*vendor_string, vendor_name) < 0|| + if (VIR_STRDUP(*vendor_string, vendor_name) < 0 || VIR_STRDUP(*product_string, device_name) < 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } @@ -419,84 +414,75 @@ static int udevProcessUSBDevice(struct udev_device *device, virNodeDeviceDefPtr def) { virNodeDevCapDataPtr data = &def->caps->data; - int ret = -1; if (udevGetUintProperty(device, "BUSNUM", &data->usb_dev.bus, 10) < 0) - goto out; + return -1; if (udevGetUintProperty(device, "DEVNUM", &data->usb_dev.device, 10) < 0) - goto out; + return -1; if (udevGetUintProperty(device, "ID_VENDOR_ID", &data->usb_dev.vendor, 16) < 0) - goto out; + return -1; if (udevGetStringProperty(device, "ID_VENDOR_FROM_DATABASE", &data->usb_dev.vendor_name) < 0) - goto out; + return -1; if (!data->usb_dev.vendor_name && udevGetStringSysfsAttr(device, "manufacturer", &data->usb_dev.vendor_name) < 0) - goto out; + return -1; if (udevGetUintProperty(device, "ID_MODEL_ID", &data->usb_dev.product, 16) < 0) - goto out; + return -1; if (udevGetStringProperty(device, "ID_MODEL_FROM_DATABASE", &data->usb_dev.product_name) < 0) - goto out; + return -1; if (!data->usb_dev.product_name && udevGetStringSysfsAttr(device, "product", &data->usb_dev.product_name) < 0) - goto out; + return -1; if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } static int udevProcessUSBInterface(struct udev_device *device, virNodeDeviceDefPtr def) { - int ret = -1; virNodeDevCapDataPtr data = &def->caps->data; if (udevGetUintSysfsAttr(device, "bInterfaceNumber", &data->usb_if.number, 16) < 0) - goto out; + return -1; if (udevGetUintSysfsAttr(device, "bInterfaceClass", &data->usb_if._class, 16) < 0) - goto out; + return -1; if (udevGetUintSysfsAttr(device, "bInterfaceSubClass", &data->usb_if.subclass, 16) < 0) - goto out; + return -1; if (udevGetUintSysfsAttr(device, "bInterfaceProtocol", &data->usb_if.protocol, 16) < 0) - goto out; + return -1; if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } static int udevProcessNetworkInterface(struct udev_device *device, virNodeDeviceDefPtr def) { - int ret = -1; const char *devtype = udev_device_get_devtype(device); virNodeDevCapDataPtr data = &def->caps->data; @@ -509,35 +495,31 @@ static int udevProcessNetworkInterface(struct udev_device *device, if (udevGetStringProperty(device, "INTERFACE", &data->net.ifname) < 0) - goto out; + return -1; if (udevGetStringSysfsAttr(device, "address", &data->net.address) < 0) - goto out; + return -1; if (udevGetUintSysfsAttr(device, "addr_len", &data->net.address_len, 0) < 0) - goto out; + return -1; if (udevGenerateDeviceName(device, def, data->net.address) != 0) - goto out; + return -1; if (virNetDevGetLinkInfo(data->net.ifname, &data->net.lnk) < 0) - goto out; + return -1; if (virNetDevGetFeatures(data->net.ifname, &data->net.features) < 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, virNodeDeviceDefPtr def) { - int ret = -1; virNodeDevCapDataPtr data = &def->caps->data; char *filename = NULL; char *str; @@ -549,40 +531,33 @@ static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse SCSI host '%s'"), filename); - goto out; + return -1; } nodeDeviceSysfsGetSCSIHostCaps(&def->caps->data); if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } static int udevProcessSCSITarget(struct udev_device *device ATTRIBUTE_UNUSED, virNodeDeviceDefPtr def) { - int ret = -1; const char *sysname = NULL; virNodeDevCapDataPtr data = &def->caps->data; sysname = udev_device_get_sysname(device); if (VIR_STRDUP(data->scsi_target.name, sysname) < 0) - goto out; + return -1; if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; - - ret = 0; + return -1; - out: - return ret; + return 0; } @@ -752,7 +727,6 @@ static int udevProcessRemoveableMedia(struct udev_device *device, static int udevProcessCDROM(struct udev_device *device, virNodeDeviceDefPtr def) { - int ret = -1; int has_media = 0; /* NB: the drive_type string provided by udev is different from @@ -761,15 +735,13 @@ static int udevProcessCDROM(struct udev_device *device, * versions of libvirt. */ VIR_FREE(def->caps->data.storage.drive_type); if (VIR_STRDUP(def->caps->data.storage.drive_type, "cdrom") < 0) - goto out; + return -1; if (udevHasDeviceProperty(device, "ID_CDROM_MEDIA") && udevGetIntProperty(device, "ID_CDROM_MEDIA", &has_media, 0) < 0) - goto out; + return -1; - ret = udevProcessRemoveableMedia(device, def, has_media); - out: - return ret; + return udevProcessRemoveableMedia(device, def, has_media); } static int udevProcessFloppy(struct udev_device *device, @@ -794,21 +766,19 @@ static int udevProcessSD(struct udev_device *device, virNodeDeviceDefPtr def) { virNodeDevCapDataPtr data = &def->caps->data; - int ret = 0; if (udevGetUint64SysfsAttr(device, "size", &data->storage.num_blocks) < 0) - goto out; + return -1; if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", &data->storage.logical_block_size) < 0) - goto out; + return -1; data->storage.size = data->storage.num_blocks * data->storage.logical_block_size; - out: - return ret; + return 0; } -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:07 +0200, Ján Tomko wrote:
Remove ret variables and labels from functions where there is no cleanup to be done. --- src/node_device/node_device_udev.c | 108 ++++++++++++++----------------------- 1 file changed, 39 insertions(+), 69 deletions(-)
ACK
On Mon, Jun 06, 2016 at 11:02:07 +0200, Ján Tomko wrote:
Remove ret variables and labels from functions where there is no cleanup to be done. --- src/node_device/node_device_udev.c | 108 ++++++++++++++----------------------- 1 file changed, 39 insertions(+), 69 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index aa4c36e..88a333e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
udevProcessDisk needs the same treatment. ACK with that added.
Instead of the custom out and out_unlock. --- src/node_device/node_device_udev.c | 154 ++++++++++++++++++------------------- 1 file changed, 77 insertions(+), 77 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 88a333e..b46fec6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -326,7 +326,7 @@ static int udevProcessPCI(struct udev_device *device, syspath = udev_device_get_syspath(device); if (udevGetUintProperty(device, "PCI_CLASS", &data->pci_dev.class, 16) < 0) - goto out; + goto cleanup; if ((p = strrchr(syspath, '/')) == NULL || virStrToLong_ui(p + 1, &p, 16, &data->pci_dev.domain) < 0 || p == NULL || @@ -336,54 +336,54 @@ static int udevProcessPCI(struct udev_device *device, virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the PCI address from sysfs path: '%s'"), syspath); - goto out; + goto cleanup; } if (udevGetUintSysfsAttr(device, "vendor", &data->pci_dev.vendor, 16) < 0) - goto out; + goto cleanup; if (udevGetUintSysfsAttr(device, "device", &data->pci_dev.product, 16) < 0) - goto out; + goto cleanup; if (udevTranslatePCIIds(data->pci_dev.vendor, data->pci_dev.product, &data->pci_dev.vendor_name, &data->pci_dev.product_name) != 0) { - goto out; + goto cleanup; } if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; + goto cleanup; /* The default value is -1, because it can't be 0 * as zero is valid node number. */ data->pci_dev.numa_node = -1; if (udevGetIntSysfsAttr(device, "numa_node", &data->pci_dev.numa_node, 10) < 0) - goto out; + goto cleanup; if (nodeDeviceSysfsGetPCIRelatedDevCaps(syspath, data) < 0) - goto out; + goto cleanup; if (!(pciDev = virPCIDeviceNew(data->pci_dev.domain, data->pci_dev.bus, data->pci_dev.slot, data->pci_dev.function))) - goto out; + goto cleanup; /* We need to be root to read PCI device configs */ if (priv->privileged) { if (virPCIGetHeaderType(pciDev, &data->pci_dev.hdrType) < 0) - goto out; + goto cleanup; if (virPCIDeviceIsPCIExpress(pciDev) > 0) { if (VIR_ALLOC(pci_express) < 0) - goto out; + goto cleanup; if (virPCIDeviceHasPCIExpressLink(pciDev) > 0) { if (VIR_ALLOC(pci_express->link_cap) < 0 || VIR_ALLOC(pci_express->link_sta) < 0) - goto out; + goto cleanup; if (virPCIDeviceGetLinkCapSta(pciDev, &pci_express->link_cap->port, @@ -391,7 +391,7 @@ static int udevProcessPCI(struct udev_device *device, &pci_express->link_cap->width, &pci_express->link_sta->speed, &pci_express->link_sta->width) < 0) - goto out; + goto cleanup; pci_express->link_sta->port = -1; /* PCIe can't negotiate port. Yet :) */ } @@ -403,7 +403,7 @@ static int udevProcessPCI(struct udev_device *device, ret = 0; - out: + cleanup: virPCIDeviceFree(pciDev); virPCIEDeviceInfoFree(pci_express); return ret; @@ -641,18 +641,18 @@ static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED, if (udev_device_get_sysattr_value(device, "type")) { if (udevGetUintSysfsAttr(device, "type", &tmp, 0) < 0) - goto out; + goto cleanup; if (udevGetSCSIType(def, tmp, &data->scsi.type) < 0) - goto out; + goto cleanup; } if (udevGenerateDeviceName(device, def, NULL) != 0) - goto out; + goto cleanup; ret = 0; - out: + cleanup: if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to process SCSI device with sysfs path '%s'"), @@ -669,16 +669,16 @@ static int udevProcessDisk(struct udev_device *device, int ret = 0; if (udevGetUint64SysfsAttr(device, "size", &data->storage.num_blocks) < 0) - goto out; + goto cleanup; if (udevGetUint64SysfsAttr(device, "queue/logical_block_size", &data->storage.logical_block_size) < 0) - goto out; + goto cleanup; data->storage.size = data->storage.num_blocks * data->storage.logical_block_size; - out: + cleanup: return ret; } @@ -818,24 +818,24 @@ static int udevProcessStorage(struct udev_device *device, devnode = udev_device_get_devnode(device); if (!devnode) { VIR_DEBUG("No devnode for '%s'", udev_device_get_devpath(device)); - goto out; + goto cleanup; } if (VIR_STRDUP(data->storage.block, devnode) < 0) - goto out; + goto cleanup; if (udevGetStringProperty(device, "ID_BUS", &data->storage.bus) < 0) - goto out; + goto cleanup; if (udevGetStringProperty(device, "ID_SERIAL", &data->storage.serial) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "device/vendor", &data->storage.vendor) < 0) - goto out; + goto cleanup; if (def->caps->data.storage.vendor) virTrimSpaces(def->caps->data.storage.vendor, NULL); if (udevGetStringSysfsAttr(device, "device/model", &data->storage.model) < 0) - goto out; + goto cleanup; if (def->caps->data.storage.model) virTrimSpaces(def->caps->data.storage.model, NULL); /* There is no equivalent of the hotpluggable property in libudev, @@ -844,7 +844,7 @@ static int udevProcessStorage(struct udev_device *device, * for it. */ if (udevGetStringProperty(device, "ID_TYPE", &data->storage.drive_type) < 0) - goto out; + goto cleanup; if (!data->storage.drive_type || STREQ(def->caps->data.storage.drive_type, "generic")) { @@ -854,31 +854,31 @@ static int udevProcessStorage(struct udev_device *device, /* All floppy drives have the ID_DRIVE_FLOPPY prop. This is * needed since legacy floppies don't have a drive_type */ if (udevGetIntProperty(device, "ID_DRIVE_FLOPPY", &val, 0) < 0) - goto out; + goto cleanup; else if (val == 1) str = "floppy"; if (!str) { if (udevGetIntProperty(device, "ID_CDROM", &val, 0) < 0) - goto out; + goto cleanup; else if (val == 1) str = "cd"; } if (!str) { if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) < 0) - goto out; + goto cleanup; if (val == 1) str = "sd"; } if (str) { if (VIR_STRDUP(data->storage.drive_type, str) < 0) - goto out; + goto cleanup; } else { /* If udev doesn't have it, perhaps we can guess it. */ if (udevKludgeStorageType(def) != 0) - goto out; + goto cleanup; } } @@ -893,13 +893,13 @@ static int udevProcessStorage(struct udev_device *device, } else { VIR_DEBUG("Unsupported storage type '%s'", def->caps->data.storage.drive_type); - goto out; + goto cleanup; } if (udevGenerateDeviceName(device, def, data->storage.serial) != 0) - goto out; + goto cleanup; - out: + cleanup: VIR_DEBUG("Storage ret=%d", ret); return ret; } @@ -1066,7 +1066,7 @@ static int udevSetParent(struct udev_device *device, virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get syspath for parent of '%s'"), udev_device_get_syspath(parent_device)); - goto out; + goto cleanup; } dev = virNodeDeviceFindBySysfsPath(&driver->devs, @@ -1074,22 +1074,22 @@ static int udevSetParent(struct udev_device *device, if (dev != NULL) { if (VIR_STRDUP(def->parent, dev->def->name) < 0) { virNodeDeviceObjUnlock(dev); - goto out; + goto cleanup; } virNodeDeviceObjUnlock(dev); if (VIR_STRDUP(def->parent_sysfs_path, parent_sysfs_path) < 0) - goto out; + goto cleanup; } } while (def->parent == NULL && parent_device != NULL); if (!def->parent && VIR_STRDUP(def->parent, "computer") < 0) - goto out; + goto cleanup; ret = 0; - out: + cleanup: return ret; } @@ -1101,37 +1101,37 @@ static int udevAddOneDevice(struct udev_device *device) int ret = -1; if (VIR_ALLOC(def) != 0) - goto out; + goto cleanup; if (VIR_STRDUP(def->sysfs_path, udev_device_get_syspath(device)) < 0) - goto out; + goto cleanup; if (udevGetStringProperty(device, "DRIVER", &def->driver) < 0) - goto out; + goto cleanup; if (VIR_ALLOC(def->caps) != 0) - goto out; + goto cleanup; if (udevGetDeviceType(device, &def->caps->data.type) != 0) - goto out; + goto cleanup; if (udevGetDeviceDetails(device, def) != 0) - goto out; + goto cleanup; if (udevSetParent(device, def) != 0) - goto out; + goto cleanup; /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ dev = virNodeDeviceAssignDef(&driver->devs, def); if (dev == NULL) - goto out; + goto cleanup; virNodeDeviceObjUnlock(dev); ret = 0; - out: + cleanup: if (ret != 0) { VIR_DEBUG("Discarding device %d %p %s", ret, def, def ? NULLSTR(def->sysfs_path) : ""); @@ -1178,7 +1178,7 @@ static int udevEnumerateDevices(struct udev *udev) ret = udev_enumerate_scan_devices(udev_enumerate); if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("udev scan devices returned %d"), ret); - goto out; + goto cleanup; } udev_list_entry_foreach(list_entry, @@ -1187,7 +1187,7 @@ static int udevEnumerateDevices(struct udev *udev) udevProcessDeviceListEntry(udev, list_entry); } - out: + cleanup: udev_enumerate_unref(udev_enumerate); return ret; } @@ -1259,14 +1259,14 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, if (fd != udev_fd) { virReportError(VIR_ERR_INTERNAL_ERROR, _("File descriptor returned by udev %d does not " "match node device file descriptor %d"), fd, udev_fd); - goto out; + goto cleanup; } device = udev_monitor_receive_device(udev_monitor); if (device == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_receive_device returned NULL")); - goto out; + goto cleanup; } action = udev_device_get_action(device); @@ -1274,15 +1274,15 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, if (STREQ(action, "add") || STREQ(action, "change")) { udevAddOneDevice(device); - goto out; + goto cleanup; } if (STREQ(action, "remove")) { udevRemoveOneDevice(device); - goto out; + goto cleanup; } - out: + cleanup: udev_device_unref(device); nodeDeviceUnlock(); return; @@ -1305,37 +1305,37 @@ udevGetDMIData(virNodeDevCapDataPtr data) if (device == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get udev device for syspath '%s' or '%s'"), DMI_DEVPATH, DMI_DEVPATH_FALLBACK); - goto out; + return; } } if (udevGetStringSysfsAttr(device, "product_name", &data->system.product_name) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "sys_vendor", &data->system.hardware.vendor_name) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "product_version", &data->system.hardware.version) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "product_serial", &data->system.hardware.serial) < 0) - goto out; + goto cleanup; if (virGetHostUUID(data->system.hardware.uuid)) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "bios_vendor", &data->system.firmware.vendor_name) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "bios_version", &data->system.firmware.version) < 0) - goto out; + goto cleanup; if (udevGetStringSysfsAttr(device, "bios_date", &data->system.firmware.release_date) < 0) - goto out; + goto cleanup; - out: + cleanup: if (device != NULL) udev_device_unref(device); return; @@ -1353,10 +1353,10 @@ static int udevSetupSystemDev(void) return -1; if (VIR_STRDUP(def->name, "computer") < 0) - goto out; + goto cleanup; if (VIR_ALLOC(def->caps) != 0) - goto out; + goto cleanup; #if defined(__x86_64__) || defined(__i386__) || defined(__amd64__) udevGetDMIData(&def->caps->data); @@ -1364,13 +1364,13 @@ static int udevSetupSystemDev(void) dev = virNodeDeviceAssignDef(&driver->devs, def); if (dev == NULL) - goto out; + goto cleanup; virNodeDeviceObjUnlock(dev); ret = 0; - out: + cleanup: if (ret == -1) virNodeDeviceDefFree(def); @@ -1430,7 +1430,7 @@ static int nodeStateInitialize(bool privileged, nodeDeviceLock(); if (udevPCITranslateInit(privileged) < 0) - goto out_unlock; + goto cleanup; /* * http://www.kernel.org/pub/linux/utils/kernel/hotplug/libudev/libudev-udev.ht... @@ -1448,7 +1448,7 @@ static int nodeStateInitialize(bool privileged, if (priv->udev_monitor == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("udev_monitor_new_from_netlink returned NULL")); - goto out_unlock; + goto cleanup; } udev_monitor_enable_receiving(priv->udev_monitor); @@ -1465,20 +1465,20 @@ static int nodeStateInitialize(bool privileged, VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, NULL, NULL); if (priv->watch == -1) - goto out_unlock; + goto cleanup; /* Create a fictional 'computer' device to root the device tree. */ if (udevSetupSystemDev() != 0) - goto out_unlock; + goto cleanup; /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) - goto out_unlock; + goto cleanup; ret = 0; - out_unlock: + cleanup: nodeDeviceUnlock(); if (ret == -1) -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:08 +0200, Ján Tomko wrote:
Instead of the custom out and out_unlock. --- src/node_device/node_device_udev.c | 154 ++++++++++++++++++------------------- 1 file changed, 77 insertions(+), 77 deletions(-)
ACK
Filter out some subsystems we are not interested in. --- After the netdev cleanups, this speeds up the driver initialization from 18 ms to 13 ms, which is percentually a lot, but neligible in absolute times. This patch uses a negative filter because I could not find an exhaustive list of possible subsystems. A positive filter could be applied to udev_monitor as well. src/node_device/node_device_udev.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b46fec6..2e86230 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1167,13 +1167,34 @@ static int udevProcessDeviceListEntry(struct udev *udev, } +const char *subsystem_blacklist[] = { + "acpi", "tty", "vc", "i2c", +}; + +static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(subsystem_blacklist); i++) { + const char *s = subsystem_blacklist[i]; + if (udev_enumerate_add_nomatch_subsystem(udev_enumerate, s) < 0) { + virReportSystemError(errno, "%s", _("failed to add susbsystem filter")); + return -1; + } + } + return 0; +} + + static int udevEnumerateDevices(struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; - int ret = 0; + int ret = -1; udev_enumerate = udev_enumerate_new(udev); + if (udevEnumerateAddMatches(udev_enumerate) < 0) + goto cleanup; ret = udev_enumerate_scan_devices(udev_enumerate); if (ret != 0) { -- 2.7.3
On Mon, Jun 06, 2016 at 11:02:09 +0200, Ján Tomko wrote:
Filter out some subsystems we are not interested in. --- After the netdev cleanups, this speeds up the driver initialization from 18 ms to 13 ms, which is percentually a lot, but neligible in absolute times.
This patch uses a negative filter because I could not find an exhaustive list of possible subsystems. A positive filter could be applied to udev_monitor as well.
src/node_device/node_device_udev.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b46fec6..2e86230 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1167,13 +1167,34 @@ static int udevProcessDeviceListEntry(struct udev *udev, }
I think this deserves a comment explaining the reason and what to add here.
+const char *subsystem_blacklist[] = { + "acpi", "tty", "vc", "i2c", +}; + +static int udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(subsystem_blacklist); i++) { + const char *s = subsystem_blacklist[i]; + if (udev_enumerate_add_nomatch_subsystem(udev_enumerate, s) < 0) { + virReportSystemError(errno, "%s", _("failed to add susbsystem filter")); + return -1;
Should this be fatal? On the other hand other stuff will probably fail too.
+ } + } + return 0; +} + + static int udevEnumerateDevices(struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; - int ret = 0; + int ret = -1;
udev_enumerate = udev_enumerate_new(udev); + if (udevEnumerateAddMatches(udev_enumerate) < 0) + goto cleanup;
ret = udev_enumerate_scan_devices(udev_enumerate); if (ret != 0) {
ACK with the comment added.
participants (2)
-
Ján Tomko -
Peter Krempa