The udevProcessStorage method relies on udevGetIntProperty ignoring
errors about non-existant properties and instead setting the value to
zero. In theory when seeing ID_CDROM=1, you might expect that devices
which are not CDs will get ID_CDROM=0, but that's not what happens in
practice. Instead the property simply won't get set at all.
IOW, the code does not need to care about the value of the property,
merely whether it exists or not.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/node_device/node_device_udev.c | 35 ++++++------------------------
1 file changed, 7 insertions(+), 28 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index b1b1886c54..e48e62dab8 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -962,37 +962,16 @@ udevProcessStorage(struct udev_device *device,
if (!storage->drive_type ||
STREQ(def->caps->data.storage.drive_type, "generic")) {
- 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", &val, 0) < 0)
+ if (udevHasDeviceProperty(device, "ID_DRIVE_FLOPPY"))
+ storage->drive_type = g_strdup("floppy");
+ else if (udevHasDeviceProperty(device, "ID_CDROM"))
+ storage->drive_type = g_strdup("cd");
+ else if (udevHasDeviceProperty(device, "ID_DRIVE_FLASH_SD"))
+ storage->drive_type = g_strdup("sd");
+ else if (udevKludgeStorageType(def) != 0)
goto cleanup;
- else if (val == 1)
- str = "floppy";
-
- if (!str) {
- if (udevGetIntProperty(device, "ID_CDROM", &val, 0) < 0)
- goto cleanup;
- else if (val == 1)
- str = "cd";
- }
-
- if (!str) {
- if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) < 0)
- goto cleanup;
- if (val == 1)
- str = "sd";
- }
-
- if (str) {
- storage->drive_type = g_strdup(str);
- } else {
- /* If udev doesn't have it, perhaps we can guess it. */
- if (udevKludgeStorageType(def) != 0)
- goto cleanup;
- }
}
if (STREQ(def->caps->data.storage.drive_type, "cd")) {
--
2.28.0
On 11/17/20 7:56 AM, Daniel P. Berrangé wrote:
> The udevProcessStorage method relies on udevGetIntProperty ignoring
> errors about non-existant properties and instead setting the value to
> zero. In theory when seeing ID_CDROM=1, you might expect that devices
> which are not CDs will get ID_CDROM=0, but that's not what happens in
> practice. Instead the property simply won't get set at all.
Taking you at your word that, e.g. ID_CDROM will *always* be unset, and
never set to 0 when the device isn't a CD,
Reviewed-by: Laine Stump <laine@redhat.com>
(It totally makes sense that this would never happen, and I don't doubt
that your statement is correct. I'm just covering myself for the
eventuality that someone somewhere did something uh.... "unconventional"
:-))
>
> IOW, the code does not need to care about the value of the property,
> merely whether it exists or not.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> src/node_device/node_device_udev.c | 35 ++++++------------------------
> 1 file changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index b1b1886c54..e48e62dab8 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -962,37 +962,16 @@ udevProcessStorage(struct udev_device *device,
>
> if (!storage->drive_type ||
> STREQ(def->caps->data.storage.drive_type, "generic")) {
> - 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", &val, 0) < 0)
> + if (udevHasDeviceProperty(device, "ID_DRIVE_FLOPPY"))
> + storage->drive_type = g_strdup("floppy");
> + else if (udevHasDeviceProperty(device, "ID_CDROM"))
> + storage->drive_type = g_strdup("cd");
> + else if (udevHasDeviceProperty(device, "ID_DRIVE_FLASH_SD"))
> + storage->drive_type = g_strdup("sd");
> + else if (udevKludgeStorageType(def) != 0)
> goto cleanup;
> - else if (val == 1)
> - str = "floppy";
> -
> - if (!str) {
> - if (udevGetIntProperty(device, "ID_CDROM", &val, 0) < 0)
> - goto cleanup;
> - else if (val == 1)
> - str = "cd";
> - }
> -
> - if (!str) {
> - if (udevGetIntProperty(device, "ID_DRIVE_FLASH_SD", &val, 0) < 0)
> - goto cleanup;
> - if (val == 1)
> - str = "sd";
> - }
> -
> - if (str) {
> - storage->drive_type = g_strdup(str);
> - } else {
> - /* If udev doesn't have it, perhaps we can guess it. */
> - if (udevKludgeStorageType(def) != 0)
> - goto cleanup;
> - }
> }
>
> if (STREQ(def->caps->data.storage.drive_type, "cd")) {
On Tue, Nov 17, 2020 at 11:04:07AM -0500, Laine Stump wrote: > On 11/17/20 7:56 AM, Daniel P. Berrangé wrote: > > The udevProcessStorage method relies on udevGetIntProperty ignoring > > errors about non-existant properties and instead setting the value to > > zero. In theory when seeing ID_CDROM=1, you might expect that devices > > which are not CDs will get ID_CDROM=0, but that's not what happens in > > practice. Instead the property simply won't get set at all. > > > Taking you at your word that, e.g. ID_CDROM will *always* be unset, and > never set to 0 when the device isn't a CD, ID_CDROM=1 is set when the '/lib/udev/cdrom_id' program determins that the device is a CDROM. There's no trace of anything evers etting ID_CDROM=0 in the standard source. I guess in theory someone could write a udev rule to force ID_CROM=0 instead of deleting the property, but that's kind of silly. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.