[PATCH 1/2] node_device: Rework udevKludgeStorageType()

[Peter Krempa]

On Wed, Jan 26, 2022 at 14:13:20 +0100, Michal Privoznik wrote:
> The udevKludgeStorageType() function looks at devlink name
> (/dev/XXX) and guesses the type of the (storage) device using a
> series of STRPREFIX() calls. Well those can be turn into an array
> and a for() loop, especially if we are about to add a new case
> (in the next commit).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/node_device/node_device_udev.c | 40 +++++++++++++++---------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index cd1722f934..14acb3fee0 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -890,32 +890,32 @@ udevProcessDASD(struct udev_device *device,
>  static int
>  udevKludgeStorageType(virNodeDeviceDef *def)
>  {
> +    size_t i;
> +    const char *fixups[][2] = {
> +        /* virtio disk */
> +        { "/dev/vd", "disk" },
> +
> +        /* For Direct Access Storage Devices (DASDs) there are
> +         * currently no identifiers in udev besides ID_PATH. Since
> +         * ID_TYPE=disk does not exist on DASDs they fall through
> +         * the udevProcessStorage detection logic. */
> +        { "/dev/dasd", "dasd" },
> +    };

I'd probably slightly prefer if this was an array of structs where the
fields are named ...

> +
>      VIR_DEBUG("Could not find definitive storage type for device "
>                "with sysfs path '%s', trying to guess it",
>                def->sysfs_path);
>  
> -    /* virtio disk */
> -    if (STRPREFIX(def->caps->data.storage.block, "/dev/vd")) {
> -        def->caps->data.storage.drive_type = g_strdup("disk");
> -        VIR_DEBUG("Found storage type '%s' for device "
> -                  "with sysfs path '%s'",
> -                  def->caps->data.storage.drive_type,
> -                  def->sysfs_path);
> -        return 0;
> +    for (i = 0; i < G_N_ELEMENTS(fixups); i++) {
> +        if (STRPREFIX(def->caps->data.storage.block, fixups[i][0])) {
> +            def->caps->data.storage.drive_type = g_strdup(fixups[i][1]);

So that's a bit more obvious which field here is used in a particular
situation.

Granted this is just a single function.

Reviewed-by: Peter Krempa <pkrempa at redhat.com>