[libvirt PATCH 0/4] nodedev: cleanup some historical baggage
Daniel P. Berrangé (4): nodedev: improve debugging logs from udev device/event processing nodedev: dont rely on ignoring errors on missing properties nodedev: drop DKD_MEDIA_AVAILABLE property check nodedev: report errors about missing integer properties src/node_device/node_device_udev.c | 70 +++++++++++++----------------- 1 file changed, 30 insertions(+), 40 deletions(-) -- 2.28.0
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 65f312d8f4..b1b1886c54 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -141,8 +141,9 @@ udevGetDeviceProperty(struct udev_device *udev_device, ret = udev_device_get_property_value(udev_device, property_key); - VIR_DEBUG("Found property key '%s' value '%s' for device with sysname '%s'", - property_key, NULLSTR(ret), udev_device_get_sysname(udev_device)); + VIR_DEBUG("Found property key '%s' value '%s' for device with sysname '%s' errno='%s'", + property_key, NULLSTR(ret), udev_device_get_sysname(udev_device), + ret ? "" : g_strerror(errno)); return ret; } @@ -1609,7 +1610,7 @@ udevHandleOneDevice(struct udev_device *device) { const char *action = udev_device_get_action(device); - VIR_DEBUG("udev action: '%s'", action); + VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); if (STREQ(action, "add") || STREQ(action, "change")) return udevAddOneDevice(device); -- 2.28.0
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 :|
The access of DKD_MEDIA_AVAILABLE for floppy disks, is mistakenly protected by a check for ID_CDROM_MEDIA, introduced in: commit 10427db77983edfaafec74ec13cc5015bab6aa95 Author: Ján Tomko <jtomko@redhat.com> Date: Fri Jun 3 16:10:21 2016 +0200 Only return two values in udevGetUintProperty Thus the check of DKD_MEDIA_AVAILABLE never run. In practice this didn't matter since this property is set by the DeviceKit-Disks daemon which was only around for 3 Fedora releases before being killed off around F13. Thus we can just remove this legacy property. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_udev.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e48e62dab8..a25babcf0d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -837,11 +837,7 @@ udevProcessFloppy(struct udev_device *device, { int has_media = 0; - if (udevHasDeviceProperty(device, "ID_CDROM_MEDIA")) { - /* USB floppy */ - if (udevGetIntProperty(device, "DKD_MEDIA_AVAILABLE", &has_media, 0) < 0) - return -1; - } else if (udevHasDeviceProperty(device, "ID_FS_LABEL")) { + if (udevHasDeviceProperty(device, "ID_FS_LABEL")) { /* Legacy floppy */ has_media = 1; } -- 2.28.0
The helper methods for getting integer properties ignore a missing property setting its value to zero. This lack of error reporting resulted in missing the regression handling hotplug of USB devices with the vendor and model IDs getting set to zero silently. The few callers which relied on this silent defaulting have been fixed, so now we can report fatal errors immediately. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_udev.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a25babcf0d..86b0ece5c0 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -169,10 +169,17 @@ udevGetIntProperty(struct udev_device *udev_device, const char *str = NULL; str = udevGetDeviceProperty(udev_device, property_key); + if (!str) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing udev property '%s' on '%s'"), + property_key, udev_device_get_sysname(udev_device)); + return -1; + } - if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (virStrToLong_i(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to convert '%s' to int"), str); + _("Failed to parse int '%s' from udev property '%s' on '%s'"), + str, property_key, udev_device_get_sysname(udev_device)); return -1; } return 0; @@ -188,10 +195,17 @@ udevGetUintProperty(struct udev_device *udev_device, const char *str = NULL; str = udevGetDeviceProperty(udev_device, property_key); + if (!str) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing udev property '%s' on '%s'"), + property_key, udev_device_get_sysname(udev_device)); + return -1; + } - if (str && virStrToLong_ui(str, NULL, base, value) < 0) { + if (virStrToLong_ui(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to convert '%s' to int"), str); + _("Failed to parse uint '%s' from udev property '%s' on '%s'"), + str, property_key, udev_device_get_sysname(udev_device)); return -1; } return 0; -- 2.28.0
On 11/17/20 7:56 AM, Daniel P. Berrangé wrote:
Daniel P. Berrangé (4): nodedev: improve debugging logs from udev device/event processing nodedev: dont rely on ignoring errors on missing properties nodedev: drop DKD_MEDIA_AVAILABLE property check nodedev: report errors about missing integer properties
src/node_device/node_device_udev.c | 70 +++++++++++++----------------- 1 file changed, 30 insertions(+), 40 deletions(-)
Series Reviewed-by: Laine Stump <laine@redhat.com> (I made one comment on 2/4, but it's nothing that needs any action - just pointing out the level of trust you have in everybody always doing what is "sensible" :-)
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Laine Stump