[PATCH 4/5] node_device: detect DASD devices

[Boris Fiuczynski]

On 9/8/20 5:50 PM, Erik Skultety wrote:
> On Mon, Aug 24, 2020 at 01:59:14PM +0200, Bjoern Walk wrote:
>> From: Boris Fiuczynski <fiuczy at linux.ibm.com>
>>
>> Make Direct Access Storage Devices (DASDs) available in the node_device driver.
>>
>> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
>> Signed-off-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>> ---
>>   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

Changed it

> 
>> +{
>> +    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?
> 

For DASDs there are currently no identifies in udev besides ID_PATH.
ID_TYPE=disk does not exist. That's why the DASDs fall through the 
udevProcessStorage detection logic. Without this hunk the dasd devices 
are being detected as storage devices but than end up as "Unsupported 
storage type".
The short answer is yes. :-)

>> +        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 at redhat.com>

Thanks

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294