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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
