[PATCH 0/4] node_device_udev: Also process ID_TYPE=cd/dvd in udevProcessStorage()
See the last one which fixes a real bug. The rest is just cleanups. Michal Prívozník (4): node_device_udev: Make udevGenerateDeviceName() return void node_device_udev: Make udevGetStringProperty() return void node_device_udev: Don't overwrite @ret in udevProcessStorage() node_device_udev: Also process ID_TYPE=cd/dvd in udevProcessStorage() src/node_device/node_device_udev.c | 114 +++++++++++------------------ 1 file changed, 43 insertions(+), 71 deletions(-) -- 2.31.1
This function can't fail really as it's returning 0 no matter what. This is probably a residue from old days when we cared about propagating OOM errors. Now we just abort. Make its return type void then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 52 ++++++++++-------------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 658170a767..d5f3beb389 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -319,7 +319,7 @@ udevGetUint64SysfsAttr(struct udev_device *udev_device, } -static int +static void udevGenerateDeviceName(struct udev_device *device, virNodeDeviceDef *def, const char *s) @@ -327,8 +327,6 @@ udevGenerateDeviceName(struct udev_device *device, nodeDeviceGenerateName(def, udev_device_get_subsystem(device), udev_device_get_sysname(device), s); - - return 0; } static virMutex pciaccessMutex = VIR_MUTEX_INITIALIZER; @@ -410,8 +408,7 @@ udevProcessPCI(struct udev_device *device, goto cleanup; } - if (udevGenerateDeviceName(device, def, NULL) != 0) - goto cleanup; + udevGenerateDeviceName(device, def, NULL); /* The default value is -1, because it can't be 0 * as zero is valid node number. */ @@ -493,8 +490,7 @@ udevProcessDRMDevice(struct udev_device *device, virNodeDevCapDRM *drm = &def->caps->data.drm; int minor; - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); if (udevGetIntProperty(device, "MINOR", &minor, 10) < 0) return -1; @@ -544,8 +540,7 @@ udevProcessUSBDevice(struct udev_device *device, &usb_dev->product_name) < 0) return -1; - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -573,8 +568,7 @@ udevProcessUSBInterface(struct udev_device *device, &usb_if->protocol, 16) < 0) return -1; - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -605,8 +599,7 @@ udevProcessNetworkInterface(struct udev_device *device, if (udevGetUintSysfsAttr(device, "addr_len", &net->address_len, 0) < 0) return -1; - if (udevGenerateDeviceName(device, def, net->address) != 0) - return -1; + udevGenerateDeviceName(device, def, net->address); if (virNetDevGetLinkInfo(net->ifname, &net->lnk) < 0) return -1; @@ -638,8 +631,7 @@ udevProcessSCSIHost(struct udev_device *device G_GNUC_UNUSED, virNodeDeviceGetSCSIHostCaps(&def->caps->data.scsi_host); - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -658,8 +650,7 @@ udevProcessSCSITarget(struct udev_device *device, virNodeDeviceGetSCSITargetCaps(def->sysfs_path, &def->caps->data.scsi_target); - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -755,8 +746,7 @@ udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED, goto cleanup; } - if (udevGenerateDeviceName(device, def, NULL) != 0) - goto cleanup; + udevGenerateDeviceName(device, def, NULL); ret = 0; @@ -1008,8 +998,7 @@ udevProcessStorage(struct udev_device *device, goto cleanup; } - if (udevGenerateDeviceName(device, def, storage->serial) != 0) - goto cleanup; + udevGenerateDeviceName(device, def, storage->serial); cleanup: VIR_DEBUG("Storage ret=%d", ret); @@ -1025,8 +1014,7 @@ udevProcessSCSIGeneric(struct udev_device *dev, !def->caps->data.sg.path) return -1; - if (udevGenerateDeviceName(dev, def, NULL) != 0) - return -1; + udevGenerateDeviceName(dev, def, NULL); return 0; } @@ -1068,8 +1056,7 @@ udevProcessMediatedDevice(struct udev_device *dev, if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0) goto cleanup; - if (udevGenerateDeviceName(dev, def, NULL) != 0) - goto cleanup; + udevGenerateDeviceName(dev, def, NULL); data->iommuGroupNumber = iommugrp; @@ -1114,8 +1101,7 @@ udevProcessCCW(struct udev_device *device, if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) return -1; - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -1134,8 +1120,7 @@ udevProcessCSS(struct udev_device *device, if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) return -1; - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); if (virNodeDeviceGetCSSDynamicCaps(def->sysfs_path, &def->caps->data.ccw_dev) < 0) return -1; @@ -1181,8 +1166,7 @@ static int udevProcessVDPA(struct udev_device *device, virNodeDeviceDef *def) { - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); if (udevGetVDPACharDev(def->sysfs_path, &def->caps->data) < 0) return -1; @@ -1209,8 +1193,7 @@ udevProcessAPCard(struct udev_device *device, return -1; } - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } @@ -1235,8 +1218,7 @@ udevProcessAPQueue(struct udev_device *device, return -1; } - if (udevGenerateDeviceName(device, def, NULL) != 0) - return -1; + udevGenerateDeviceName(device, def, NULL); return 0; } -- 2.31.1
This function can't fail really as it's returning 0 no matter what. This is probably a residue from old days when we cared about propagating OOM errors. Now we just abort. Make its return type void then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 46 ++++++++++-------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d5f3beb389..f48789d98f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -165,14 +165,12 @@ udevGetDeviceProperty(struct udev_device *udev_device, } -static int +static void udevGetStringProperty(struct udev_device *udev_device, const char *property_key, char **value) { *value = g_strdup(udevGetDeviceProperty(udev_device, property_key)); - - return 0; } @@ -517,10 +515,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_VENDOR_ID", &usb_dev->vendor, 16) < 0) return -1; - if (udevGetStringProperty(device, - "ID_VENDOR_FROM_DATABASE", - &usb_dev->vendor_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_VENDOR_FROM_DATABASE", + &usb_dev->vendor_name); if (!usb_dev->vendor_name && udevGetStringSysfsAttr(device, "manufacturer", @@ -530,10 +527,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_MODEL_ID", &usb_dev->product, 16) < 0) return -1; - if (udevGetStringProperty(device, - "ID_MODEL_FROM_DATABASE", - &usb_dev->product_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_MODEL_FROM_DATABASE", + &usb_dev->product_name); if (!usb_dev->product_name && udevGetStringSysfsAttr(device, "product", @@ -587,10 +583,7 @@ udevProcessNetworkInterface(struct udev_device *device, net->subtype = VIR_NODE_DEV_CAP_NET_80203; } - if (udevGetStringProperty(device, - "INTERFACE", - &net->ifname) < 0) - return -1; + udevGetStringProperty(device, "INTERFACE", &net->ifname); if (udevGetStringSysfsAttr(device, "address", &net->address) < 0) @@ -798,9 +791,7 @@ udevProcessRemoveableMedia(struct udev_device *device, def->caps->data.storage.flags |= VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE; - if (udevGetStringProperty(device, "ID_FS_LABEL", - &storage->media_label) < 0) - return -1; + udevGetStringProperty(device, "ID_FS_LABEL", &storage->media_label); if (udevGetUint64SysfsAttr(device, "size", &storage->num_blocks) < 0) @@ -946,10 +937,8 @@ udevProcessStorage(struct udev_device *device, storage->block = g_strdup(devnode); - if (udevGetStringProperty(device, "ID_BUS", &storage->bus) < 0) - goto cleanup; - if (udevGetStringProperty(device, "ID_SERIAL", &storage->serial) < 0) - goto cleanup; + udevGetStringProperty(device, "ID_BUS", &storage->bus); + udevGetStringProperty(device, "ID_SERIAL", &storage->serial); if (udevGetStringSysfsAttr(device, "device/vendor", &storage->vendor) < 0) goto cleanup; @@ -965,8 +954,7 @@ 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", &storage->drive_type) < 0) - goto cleanup; + udevGetStringProperty(device, "ID_TYPE", &storage->drive_type); if (!storage->drive_type || STREQ(def->caps->data.storage.drive_type, "generic")) { @@ -1010,9 +998,7 @@ static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDef *def) { - if (udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path) < 0 || - !def->caps->data.sg.path) - return -1; + udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path); udevGenerateDeviceName(dev, def, NULL); @@ -1317,8 +1303,7 @@ udevGetDeviceType(struct udev_device *device, /* The following devices do not set the DEVTYPE property, therefore * we need to rely on the SUBSYSTEM property */ - if (udevGetStringProperty(device, "SUBSYSTEM", &subsystem) < 0) - return -1; + udevGetStringProperty(device, "SUBSYSTEM", &subsystem); if (STREQ_NULLABLE(subsystem, "scsi_generic")) *type = VIR_NODE_DEV_CAP_SCSI_GENERIC; @@ -1499,8 +1484,7 @@ udevAddOneDevice(struct udev_device *device) def->sysfs_path = g_strdup(udev_device_get_syspath(device)); - if (udevGetStringProperty(device, "DRIVER", &def->driver) < 0) - goto cleanup; + udevGetStringProperty(device, "DRIVER", &def->driver); def->caps = g_new0(virNodeDevCapsDef, 1); -- 2.31.1
On Wed, Jun 02, 2021 at 09:37:41 +0200, Michal Privoznik wrote:
This function can't fail really as it's returning 0 no matter what. This is probably a residue from old days when we cared about propagating OOM errors. Now we just abort. Make its return type void then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 46 ++++++++++-------------------- 1 file changed, 15 insertions(+), 31 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d5f3beb389..f48789d98f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
[...]
@@ -517,10 +515,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_VENDOR_ID", &usb_dev->vendor, 16) < 0) return -1;
- if (udevGetStringProperty(device, - "ID_VENDOR_FROM_DATABASE", - &usb_dev->vendor_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_VENDOR_FROM_DATABASE", + &usb_dev->vendor_name);
if (!usb_dev->vendor_name &&
Here you've kept the NULL check that should be now impossible to trigger ...
udevGetStringSysfsAttr(device, "manufacturer", @@ -530,10 +527,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_MODEL_ID", &usb_dev->product, 16) < 0) return -1;
- if (udevGetStringProperty(device, - "ID_MODEL_FROM_DATABASE", - &usb_dev->product_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_MODEL_FROM_DATABASE", + &usb_dev->product_name);
if (!usb_dev->product_name &&
(same here)
udevGetStringSysfsAttr(device, "product",
[...]
@@ -965,8 +954,7 @@ 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", &storage->drive_type) < 0) - goto cleanup; + udevGetStringProperty(device, "ID_TYPE", &storage->drive_type);
if (!storage->drive_type ||
(and here, but with different logic)
STREQ(def->caps->data.storage.drive_type, "generic")) { @@ -1010,9 +998,7 @@ static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDef *def) { - if (udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path) < 0 || - !def->caps->data.sg.path)
... but here you've removed it.
- return -1; + udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path);
udevGenerateDeviceName(dev, def, NULL);
On Wed, Jun 02, 2021 at 09:48:46 +0200, Peter Krempa wrote:
On Wed, Jun 02, 2021 at 09:37:41 +0200, Michal Privoznik wrote:
This function can't fail really as it's returning 0 no matter what. This is probably a residue from old days when we cared about propagating OOM errors. Now we just abort. Make its return type void then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 46 ++++++++++-------------------- 1 file changed, 15 insertions(+), 31 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d5f3beb389..f48789d98f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c
[...]
@@ -517,10 +515,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_VENDOR_ID", &usb_dev->vendor, 16) < 0) return -1;
- if (udevGetStringProperty(device, - "ID_VENDOR_FROM_DATABASE", - &usb_dev->vendor_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_VENDOR_FROM_DATABASE", + &usb_dev->vendor_name);
if (!usb_dev->vendor_name &&
Here you've kept the NULL check that should be now impossible to trigger ...
udevGetStringSysfsAttr(device, "manufacturer", @@ -530,10 +527,9 @@ udevProcessUSBDevice(struct udev_device *device, if (udevGetUintProperty(device, "ID_MODEL_ID", &usb_dev->product, 16) < 0) return -1;
- if (udevGetStringProperty(device, - "ID_MODEL_FROM_DATABASE", - &usb_dev->product_name) < 0) - return -1; + udevGetStringProperty(device, + "ID_MODEL_FROM_DATABASE", + &usb_dev->product_name);
if (!usb_dev->product_name &&
(same here)
udevGetStringSysfsAttr(device, "product",
[...]
@@ -965,8 +954,7 @@ 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", &storage->drive_type) < 0) - goto cleanup; + udevGetStringProperty(device, "ID_TYPE", &storage->drive_type);
if (!storage->drive_type ||
(and here, but with different logic)
STREQ(def->caps->data.storage.drive_type, "generic")) { @@ -1010,9 +998,7 @@ static int udevProcessSCSIGeneric(struct udev_device *dev, virNodeDeviceDef *def) { - if (udevGetStringProperty(dev, "DEVNAME", &def->caps->data.sg.path) < 0 || - !def->caps->data.sg.path)
... but here you've removed it.
Okay, according to the manpage of 'udev_device_get_property_value' [1] which is called from 'udevGetDeviceProperty' which is in turn called from 'udevGetStringProperty' 'udev_device_get_property_value' can return NULL on failure. g_strdup will faithfully duplicate the NULL, thus this is changing the logic of the return value handling without justification. Please keep the check in place or justify why it's okay. [1] On success, udev_device_get_property_value() and udev_device_get_sysattr_value() return a pointer to a constant string of the requested value. On error, NULL is returned. Attributes that may contain NUL bytes should not be retrieved with udev_device_get_sysattr_value(); instead, read them directly from the files within the device's syspath.
Let's use a different variable for storing retvals of helper functions. This way the usual function pattern can be restored. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f48789d98f..3c31e6cde4 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -927,6 +927,7 @@ udevProcessStorage(struct udev_device *device, { virNodeDevCapStorage *storage = &def->caps->data.storage; int ret = -1; + int rv; const char* devnode; devnode = udev_device_get_devnode(device); @@ -971,22 +972,26 @@ udevProcessStorage(struct udev_device *device, } if (STREQ(def->caps->data.storage.drive_type, "cd")) { - ret = udevProcessCDROM(device, def); + rv = udevProcessCDROM(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "disk")) { - ret = udevProcessDisk(device, def); + rv = udevProcessDisk(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "floppy")) { - ret = udevProcessFloppy(device, def); + rv = udevProcessFloppy(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "sd")) { - ret = udevProcessSD(device, def); + rv = udevProcessSD(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "dasd")) { - ret = udevProcessDASD(device, def); + rv = udevProcessDASD(device, def); } else { VIR_DEBUG("Unsupported storage type '%s'", def->caps->data.storage.drive_type); goto cleanup; } + if (rv < 0) + goto cleanup; + udevGenerateDeviceName(device, def, storage->serial); + ret = 0; cleanup: VIR_DEBUG("Storage ret=%d", ret); -- 2.31.1
When processing node devices, the udevProcessStorage() will be called if the device is some form of storage. In here, ID_TYPE attribute is queried and depending on its value one of more specialized helper functions is called. For instance, for ID_TYPE=="cd" the udevProcessCDROM() is called, for ID_TYPE=="disk" the udevProcessDisk() is called, and so on. But there's a problem with ID_TYPE and its values. Coming from udev, we are not guaranteed that ID_TYPE will contain "cd" for CDROM devices. In fact, there's a rule installed by sg3_utils that will overwrite ID_TYPE to "cd/dvd" leaving us with an unhandled type. Fortunately, this was fixed in their upstream, but there are still versions out there, on OS platforms that we aim to support that contain the problematic rule. Therefore, we should accept both strings. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1848875 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/node_device/node_device_udev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 3c31e6cde4..b1c97d30b1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -971,7 +971,8 @@ udevProcessStorage(struct udev_device *device, goto cleanup; } - if (STREQ(def->caps->data.storage.drive_type, "cd")) { + if (STREQ(def->caps->data.storage.drive_type, "cd") || + STREQ(def->caps->data.storage.drive_type, "cd/dvd")) { rv = udevProcessCDROM(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "disk")) { rv = udevProcessDisk(device, def); -- 2.31.1
On Wed, Jun 02, 2021 at 09:37:39 +0200, Michal Privoznik wrote:
See the last one which fixes a real bug. The rest is just cleanups.
Michal Prívozník (4): node_device_udev: Make udevGenerateDeviceName() return void node_device_udev: Make udevGetStringProperty() return void node_device_udev: Don't overwrite @ret in udevProcessStorage() node_device_udev: Also process ID_TYPE=cd/dvd in udevProcessStorage()
src/node_device/node_device_udev.c | 114 +++++++++++------------------ 1 file changed, 43 insertions(+), 71 deletions(-)
Series: Reviewed-by: Peter Krempa <pkrempa@redhat.com> if you clarify the change in 2/4
participants (2)
-
Michal Privoznik -
Peter Krempa