[blivet][master/rhel7-branch] Re-write the DASD storage code. (#1001070)

David Lehman dlehman at redhat.com
Tue Feb 25 15:02:14 UTC 2014


On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
> This gets rid of the DASD class and instead introduces a small
> number of functions to interface with and manage DASDs. This is
> completely divorced from the anaconda code and much more
> lightweight.

You know I am quite happy to see this code moved into devicelibs, where
it belongs, but I have a couple of comments below.

> 
> (The corresponding Fedora bug for this is 859997.)
> 
> Resolves:rhbz#1001070
> 
> ---
>  blivet/__init__.py        |  13 ++-
>  blivet/dasd.py            | 222 ----------------------------------------------
>  blivet/devicelibs/dasd.py | 134 ++++++++++++++++++++++++++++
>  blivet/devicetree.py      |   6 +-
>  4 files changed, 143 insertions(+), 232 deletions(-)
>  delete mode 100644 blivet/dasd.py
>  create mode 100644 blivet/devicelibs/dasd.py
> 
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index 6908b2a..037a40f 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -77,11 +77,11 @@ import devicefactory
>  from devicelibs.dm import name_from_dm_node
>  from devicelibs.crypto import generateBackupPassphrase
>  from devicelibs.edd import get_edd_dict
> +from devicelibs.dasd import *

This would be better as three lines that import make_dasd_list,
clear_dasd_list, and write_dasd_conf individually. I know there are
several 'import *' lines in there already, but let's not add more of
them.

<snip>

> diff --git a/blivet/devicelibs/dasd.py b/blivet/devicelibs/dasd.py
> new file mode 100644
> index 0000000..7b71552
> --- /dev/null
> +++ b/blivet/devicelibs/dasd.py
> @@ -0,0 +1,134 @@
> +#
> +# dasd.py - DASD functions
> +#
> +# Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# Red Hat Author(s): Samantha N. Bueno
> +#
> +
> +import os
> +from blivet.errors import DasdFormatError
> +from blivet.devices import deviceNameToDiskByPath
> +from blivet import util
> +from blivet import arch
> +from blivet.udev import udev_trigger
> +
> +import logging
> +log = logging.getLogger("blivet")
> +
> +import gettext
> +_ = lambda x: gettext.ldgettext("blivet", x)
> +P_ = lambda x, y, z: gettext.ldngettext("blivet", x, y, z)
> +
> +def get_dasd_ports():
> +    """ Return comma delimited string of valid DASD ports. """
> +    ports = []
> +
> +    with open("/proc/dasd/devices", "r") as f:
> +        lines = (line.strip() for line in f.readlines())
> +        f.close()
> +
> +    for line in lines:
> +        if "unknown" in line:
> +            continue
> +
> +        if "(FBA )" in line or "(ECKD)" in line:
> +            ports.append(line.split('(')[0])
> +
> +    return ','.join(ports)
> +
> +def format_dasd(dasd):
> +    """ Run dasdfmt on a DASD. Aside from one type of device noted below, this
> +        function _does not_ check if a DASD needs to be formatted, but rather,
> +        assumes the list passed needs formatting.
> +
> +        We don't need to show or update any progress bars, since disk actions
> +        will be taking place all in the progress hub, which is just one big
> +        progress bar.
> +    """
> +    try:
> +        rc = util.run_program(["/sbin/dasdfmt", "-y", "-d", "cdl", "-b", "4096", "/dev/" + dasd])
> +    except Exception as err:
> +        raise DasdFormatError(err)
> +
> +    if rc:
> +        raise DasdFormatError("dasdfmt failed: %s" % rc)
> +
> +def clear_dasd_list(dasds):
> +    """ Zero out the list of DASDs. """
> +    dasds = []
> +    return dasds
> +
> +def make_dasd_list(dasds, devicetree):

Can you make this take a list of devices instead? You could pass it
'storage.devices' or even 'storage.disks' if you didn't want to filter
by type 'dasd' in the caller of this function. Most of the devicelibs
code doesn't even know about StorageDevice -- let alone DeviceTree. I've
tried to keep the lower-level subpackages (devicelibs, formats) from
having any knowledge of the higher-level ones (devicetree, devices,
deviceaction).

> +    """ Create a list of DASDs recognized by the system. """
> +    if not arch.isS390():
> +        return
> +
> +    # Trigger udev data about the dasd devices on the system
> +    udev_trigger(action="change", name="dasd*")

This should have happened during system boot. Is there a reason it has
to be done explicitly here?

> +    log.info("Generating DASD list...")
> +    for dev in devicetree.getDevicesByType("dasd"):

If you changed this function to take a list of devices you could change
the above line to something like this:

for dev in (d for d in disks if d.type == "dasd"):

Looks okay otherwise.

David



More information about the anaconda-patches mailing list