Re: [PATCH 4/5] node_device: detect DASD devices

On Mon, Aug 24, 2020 at 01:59:14PM +0200, Bjoern Walk wrote: > From: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > > Make Direct Access Storage Devices (DASDs) available in the node_device driver. > > Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c > index 1c4df709..5f9d67cc 100644 > --- a/src/node_device/node_device_udev.c > +++ b/src/node_device/node_device_udev.c > @@ -871,6 +871,19 @@ udevProcessSD(struct udev_device *device, > } > > > +static int > +udevProcessDasd(struct udev_device *device, > + virNodeDeviceDefPtr def) s/Dasd/DASD

> +{ > + virNodeDevCapStoragePtr storage = &def->caps->data.storage; > + > + if (udevGetStringSysfsAttr(device, "device/uid", &storage->serial) < 0) > + return -1; > + > + return udevProcessDisk(device, def); > +} > + > + > /* This function exists to deal with the case in which a driver does > * not provide a device type in the usual place, but udev told us it's > * a storage device, and we can make a good guess at what kind of > @@ -891,6 +904,15 @@ udevKludgeStorageType(virNodeDeviceDefPtr def) > def->sysfs_path); > return 0; > } > + /* dasd disk */ > + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { > + def->caps->data.storage.drive_type = g_strdup("dasd"); > + VIR_DEBUG("Found storage type '%s' for device " > + "with sysfs path '%s'", > + def->caps->data.storage.drive_type, > + def->sysfs_path); I understand why we would need it for /dev/vdX, but can udev not know the drive_type from kernel? IOW Do we really need ^this hunk?

> + return 0; > + } > VIR_DEBUG("Could not determine storage type " > "for device with sysfs path '%s'", def->sysfs_path); > return -1; > @@ -978,6 +1000,8 @@ udevProcessStorage(struct udev_device *device, > ret = udevProcessFloppy(device, def); > } else if (STREQ(def->caps->data.storage.drive_type, "sd")) { > ret = udevProcessSD(device, def); > + } else if (STREQ(def->caps->data.storage.drive_type, "dasd")) { > + ret = udevProcessDasd(device, def); > } else { > VIR_DEBUG("Unsupported storage type '%s'", > def->caps->data.storage.drive_type); The rest looks good: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;