2:37 a.m.
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
2:37 a.m.
New subject: [PATCH 1/4] node_device_udev: Make udevGenerateDeviceName() return void
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(a)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
2:37 a.m.
New subject: [PATCH 2/4] node_device_udev: Make udevGetStringProperty() return void
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(a)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
2:48 a.m.
New subject: [PATCH 2/4] node_device_udev: Make udevGetStringProperty() return void
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(a)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);
2:58 a.m.
New subject: [PATCH 2/4] node_device_udev: Make udevGetStringProperty() return void
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(a)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.
2:37 a.m.
New subject: [PATCH 3/4] node_device_udev: Don't overwrite @ret in udevProcessStorage()
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(a)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
2:37 a.m.
New subject: [PATCH 4/4] node_device_udev: Also process ID_TYPE=cd/dvd in udevProcessStorage()
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(a)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
3:05 a.m.
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(a)redhat.com>
if you clarify the change in 2/4
1304
days inactive
1304
days old
7 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa