6:56 a.m.
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
6:56 a.m.
New subject: [libvirt PATCH 1/4] nodedev: improve debugging logs from udev device/event processing
Signed-off-by: Daniel P. Berrangé <berrange(a)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
6:56 a.m.
New subject: [libvirt PATCH 2/4] nodedev: dont rely on ignoring errors on missing properties
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(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")) {
--
2.28.0
10:04 a.m.
New subject: [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")) {
10:15 a.m.
New subject: [libvirt PATCH 2/4] nodedev: dont rely on ignoring errors on missing properties
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 :|
6:56 a.m.
New subject: [libvirt PATCH 3/4] nodedev: drop DKD_MEDIA_AVAILABLE property check
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(a)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(a)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
6:56 a.m.
New subject: [libvirt PATCH 4/4] nodedev: report errors about missing integer properties
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(a)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
10:05 a.m.
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(a)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" :-)
1502
days inactive
1502
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Laine Stump