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.