1. Patchew
  2. Libvirt
  3. [PATCH 0/5] Support CSS and DASD devices in node_device driver
  4. View patch

[PATCH 4/5] node_device: detect DASD devices

Bjoern Walk posted 5 patches 5 years, 5 months ago
There is a newer version of this series
[PATCH 4/5] node_device: detect DASD devices
Posted by Bjoern Walk 5 years, 5 months ago 
From: Boris Fiuczynski <fiuczy@linux.ibm.com>

Make Direct Access Storage Devices (DASDs) available in the node_device driver.

Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@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)
+{
+    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);
+        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);
-- 
2.24.1
Re: [PATCH 4/5] node_device: detect DASD devices
Posted by Erik Skultety 5 years, 5 months ago 
On Mon, Aug 24, 2020 at 01:59:14PM +0200, Bjoern Walk wrote:
> From: Boris Fiuczynski <fiuczy@linux.ibm.com>
>
> Make Direct Access Storage Devices (DASDs) available in the node_device driver.
>
> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
> Signed-off-by: Boris Fiuczynski <fiuczy@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

> +{
> +    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@redhat.com>
Re: [PATCH 4/5] node_device: detect DASD devices
Posted by Boris Fiuczynski 5 years, 4 months ago 
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@linux.ibm.com>
>>
>> Make Direct Access Storage Devices (DASDs) available in the node_device driver.
>>
>> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
>> Signed-off-by: Boris Fiuczynski <fiuczy@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@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
Re: [PATCH 4/5] node_device: detect DASD devices
Posted by Erik Skultety 5 years, 4 months ago 
...
> > >       }
> > > +    /* 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. :-)

Okay, can you put a concise version of ^this in a commentary then?

Erik
Re: [PATCH 4/5] node_device: detect DASD devices
Posted by Boris Fiuczynski 5 years, 4 months ago 
On 9/14/20 7:17 AM, Erik Skultety wrote:
> ...
>>>>        }
>>>> +    /* 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. :-)
> 
> Okay, can you put a concise version of ^this in a commentary then?
> 
> Erik
> 
Done, I am going to send a v2 later.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.