[libvirt PATCH 2/4] nodedev: dont rely on ignoring errors on missing properties

Laine Stump laine at redhat.com
Tue Nov 17 16:04:07 UTC 2020


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 at 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 at 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")) {





More information about the libvir-list mailing list