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

[Vratislav Podzimek]

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.
>
> (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/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()

The with block closes the file for you. And I believe you need to close
the file after reading lines not after creating the generator. So you
should drop the f.close() line and indent the for loop more to become a
part of the with block.
> 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

Same question as with the previous version of the patch -- do you want
to clear the passed list or just return a new empty one?
> +def make_dasd_list(dasds, devicetree):
> """ 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*")
> log.info("Generating DASD list...")
> for dev in devicetree.getDevicesByType("dasd"):
>    if dev not in dasds:
>        dasds.append(dev)
> return dasds
>
> +def make_unformatted_dasd_list(dasds):
> """ Return a list of DASDS which are not formatted. """
> unformatted = []
> for dasd in dasds:
>    if dasd_needs_format(dasd):
>        unformatted.append(dasd)
> return unformatted
>
> +def dasd_needs_format(dasd):
> """ Check if a DASD needs to have dasdfmt run against it or not.
>    Return True if we do need dasdfmt, False if not.
> """
> statusfile = "/sys/block/%s/device/status" % (dasd,)
> if not os.path.isfile(statusfile):
>    return False
> with open(statusfile, "r") as f:
>    status = f.read().strip()
>    f.close()

No need to close the file explicitly in the with block. The same applies
in one more place few lines below.