Re: [libvirt PATCH 2/4] nodedev: dont rely on ignoring errors on missing properties
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(a)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(a)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")) {