Comments inline
On 10/13/2009 03:51 AM, David Cantrell wrote:
> Expand the DASDDevice class to hold the device bus ID and flags that are
> passed at boot time. Add udev functions to return the bus ID and flag
> values for DASD devices. When building the device tree, read the DASD
> information and pass that to the DASDDevice object.
> ---
> storage/devices.py | 9 ++++-----
> storage/devicetree.py | 10 ++++++++++
> storage/udev.py | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/storage/devices.py b/storage/devices.py
> index 5f9d068..7d31b26 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -3156,11 +3156,10 @@ class DASDDevice(DiskDevice):
> """ A mainframe DASD. """
> _type = "dasd"
>
> - def __init__(self, device, size=None, major=None, minor=None,
> - parents=None, sysfsPath=''):
> - DiskDevice.__init__(self, device, size=size,
> - major=major, minor=minor,
> - parents=parents, sysfsPath=sysfsPath)
> + def __init__(self, device, **kwargs):
> + self.busid = kwargs.get('busid')
> + self.opts = kwargs.get('opts')
> + DiskDevice.__init__(self, device, kwargs)
>
>
> class NFSDevice(StorageDevice, NetworkStorageDevice):
> diff --git a/storage/devicetree.py b/storage/devicetree.py
> index d355804..cff7591 100644
> --- a/storage/devicetree.py
> +++ b/storage/devicetree.py
> @@ -1192,6 +1192,16 @@ class DeviceTree(object):
> kwargs["exists"] = True
> elif udev_device_is_dasd(info):
> diskType = DASDDevice
> + kwargs["busid"] = udev_device_get_dasd_bus_id(info)
> + kwargs["opts"] = {"ro": udev_device_get_dasd_flag(info,
> + "readonly"),
> + "diag": udev_device_get_dasd_flag(info,
> + "use_diag"),
> + "erplog": udev_device_get_dasd_flag(info,
> + "erplog"),
> + "failfast": udev_device_get_dasd_flag(info,
> + "failfast")
I would prefer no to do any translation to the "old" DASD kernel
parameter naming scheme for options. Instead we could just have a list
of sysfs attribute names and our option names would exactly be the same.
The code just loops over the list to collect the option values from
sysfs. This way we could easily extend the list in the future without
having to add a single line of code.
Maybe we want to have a dict of sysfs attribute names along with the
default value for each attribute. If the read value from sysfs equals
the default value, we would not even emit this attribute and its value
to kwargs["opts"].
> + }
> log.debug("%s is a dasd device" % name)
> else:
> diskType = DiskDevice
> diff --git a/storage/udev.py b/storage/udev.py
> index d051157..aeb9062 100644
> --- a/storage/udev.py
> +++ b/storage/udev.py
> @@ -165,6 +165,36 @@ def udev_device_is_dasd(info):
> else:
> return False
>
> +def udev_device_get_dasd_bus_id(info):
> + """ Return the CCW bus ID of the dasd device. """
> + ccwdevices = "/sys/bus/ccw/devices"
> +
> + if not os.path.isdir(ccwdevices):
> + return None
> +
> + for busid in os.listdir(ccwdevices):
Oh, this linear search might become a problem when having thousands of
devices. It even searches all kinds of devices on the ccw bus, so not
only DASDs but also zFCP HBAs, network devices, consoles, printers,
readers, etc.
[root@XYZ:/sys/class/block/dasda]# ll /sys/class/block/dasda
lrwxrwxrwx 1 root root 0 2009-10-11 18:00 /sys/class/block/dasda ->
../../devices/css0/0.0.0005/0.0.eb26/block/dasda
[root@H4245013:/sys/class/block/dasda]# pwd -P
/sys/devices/css0/0.0.0005/0.0.eb26/block/dasda
info.get("link_ref") should contain:
../../devices/css0/0.0.0005/0.0.eb26/block/dasda
info.get("sysfs_path") should contain:
/sys/devices/css0/0.0.0005/0.0.eb26/block/dasda
This should do all this method needs:
return info.get("sysfs_path").split("/")[-3]
# ll /sys/devices/css0/0.0.0005/0.0.eb26/block/dasda/device
lrwxrwxrwx 1 root root 0 2009-10-11 18:04
/sys/devices/css0/0.0.0005/0.0.eb26/block/dasda/device -> ../../../0.0.eb26
Alternatively, we could also do this, although this requires an extra
file system access:
return os.readlink(os.path.join("/sys", info.get("sysfs_path"),
"device")).split("/")[-1]
> + if info.get("DEVTYPE") != "disk":
> + continue
Irrelevant if we get rid of the loop, but: info is a method argument and
does not seem to be modified by the for loop, so checking this here in
side the loop will always evaluate the same?
> +
> + buspath = "%s/%s/block/%s" % (ccwdevices, busid, info.get("DEVNAME"),)
> + dasd = os.path.realpath(buspath)
> +
> + if os.path.isdir(dasd):
> + return busid
> +
> + return None
> +
> +def udev_device_get_dasd_flag(info, flag=None):
> + """ Return the specified flag for the dasd device. """
> + if flag is None:
> + return None
> +
> + path = "/sys" + info.get("sysfs_path") + "/device/" + flag
> + if not os.path.isfile(path):
> + return None
> +
> + return bool(open(path, 'r').read().strip())
We shouldn't imply that the sysfs attributes are of boolean type. While
this happens to be the case for our current set, it might change in the
future. I'd prefer to "blindly" read the sysfs attribute value string
and use this without knowing what it is (except for the above mentioned
default value, but that would just be an equal operation on two strings
whose syntax and semantics anaconda doesn't have to know).
> +
> def udev_device_is_cdrom(info):
> """ Return True if the device is an optical drive. """
> # FIXME: how can we differentiate USB drives from CD-ROM drives?
Steffen
Linux on System z Development
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Erich Baier
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
