The patch most probably fixes the bug but I've got some comments inline
below.
On 04/28/2010 12:00 AM, David Cantrell wrote:
> Originally worked up in November 2009, I've updated the patch a bit to
> work with our current code. Tested in text mode and gui mode. If you
> have any unformatted DASD devices, anaconda prompts you and asks if you
> want to format them.
> ---
> iw/filter_gui.py | 2 +-
> storage/__init__.py | 2 +-
> storage/dasd.py | 56 ++++++++++++++++++++++----------------------------
> 3 files changed, 27 insertions(+), 33 deletions(-)
>
> diff --git a/iw/filter_gui.py b/iw/filter_gui.py
> index 6fed1a9..9fee628 100644
> --- a/iw/filter_gui.py
> +++ b/iw/filter_gui.py
> @@ -550,7 +550,7 @@ class FilterWindow(InstallWindow):
> storage.iscsi.iscsi().startup(anaconda.intf)
> storage.fcoe.fcoe().startup(anaconda.intf)
> storage.zfcp.ZFCP().startup()
> - storage.dasd.DASD().startup(anaconda.intf)
> + storage.dasd.DASD().startup(anaconda.intf, anaconda.id.storage.zeroMbr)
> disks = filter(udev_device_is_disk, udev_get_block_devices())
> (singlepaths, mpaths, partitions) = identifyMultipaths(disks)
>
> diff --git a/storage/__init__.py b/storage/__init__.py
> index cbf155b..0da8795 100644
> --- a/storage/__init__.py
> +++ b/storage/__init__.py
> @@ -348,7 +348,7 @@ class Storage(object):
> self.iscsi.startup(self.anaconda.intf)
> self.fcoe.startup(self.anaconda.intf)
> self.zfcp.startup()
> - self.dasd.startup(intf=self.anaconda.intf, zeroMbr=self.zeroMbr)
> + self.dasd.startup(self.anaconda.intf, self.zeroMbr)
> if self.anaconda.id.getUpgrade():
> clearPartType = CLEARPART_TYPE_NONE
> else:
> diff --git a/storage/dasd.py b/storage/dasd.py
> index 469957e..ba4f565 100644
> --- a/storage/dasd.py
> +++ b/storage/dasd.py
> @@ -1,7 +1,7 @@
> #
> # dasd.py - DASD class
> #
> -# Copyright (C) 2009 Red Hat, Inc. All rights reserved.
> +# Copyright (C) 2009, 2010 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
> @@ -44,15 +44,17 @@ class DASD:
> def __init__(self):
> self._dasdlist = []
> self._devices = [] # list of DASDDevice objects
> - self._totalCylinders = 0
> + self.totalCylinders = 0
> self._completedCylinders = 0.0
> self._maxFormatJobs = 0
> + self.dasdfmt = "/sbin/dasdfmt"
> + self.commonArgv = ["-y", "-d", "cdl", "-b", "4096"]
> self.started = False
>
> def __call__(self):
> return self
>
> - def startup(self, *args, **kwargs):
> + def startup(self, intf, zeroMbr):
> """ Look for any unformatted DASDs in the system and offer the user
> the option for format them with dasdfmt or exit the installer.
> """
> @@ -64,9 +66,6 @@ class DASD:
> if not iutil.isS390():
> return
>
> - intf = kwargs.get("intf")
> - zeroMbr = kwargs.get("zeroMbr")
> -
> log.info("Checking for unformatted DASD devices:")
>
> for device in os.listdir("/sys/block"):
> @@ -81,8 +80,9 @@ class DASD:
> status = f.read().strip()
> f.close()
Although it's not required to fix the bug at hand, I'd like to remind us
of the following:
https://www.redhat.com/archives/anaconda-devel-list/2009-October/msg00469.html
> On 10/28/2009 09:00 PM, David Cantrell wrote:
>> On Wed, 28 Oct 2009, Chris Lumens wrote:
>>>> Under RHEL-5, this process was serial and the user had to click Yes for
>>>> each unformatted DASD found, which could take a really long time if
>>>> you had thousands of DASDs.
>>>>
>>>> The idea now is that if the DASD is seen by anaconda, we want to use it
>>>> for installation. The stage 1 device initialization routines as well as
>>>> the CMS conf file provided at boot time allow the user to restrict the
>>>> range of devices we see during installation. If any of the devices we
>>>> see are unformatted, run dasdfmt before building the devicetree.
>>>
>>> I just realized that this is going to need to change when I get my
>>> storage filtering patches in. At the least, DASD.startup is going to
>>> have to respect storage.exclusiveDisks, or we could be formatting things
>>> that the user explicitly said to not use.
>>
>> That's fine, let me know what needs to be added and I can fix it up in a
>> future patch.
>
> - if status == "unformatted":
> - log.info(" %s is an unformatted DASD" % (device,))
> + if status in ["unformatted"]:
> + log.info(" %s status is %s, needs dasdfmt" % (device,
> + status,))
In order for the log message to be helpful, it should also contain the
PATH_ID so we can see the DASD's devno and not just the arbitrary kernel
device name.
> self._dasdlist.append(device)
>
> if not len(self._dasdlist):
> @@ -116,7 +116,21 @@ class DASD:
> log.info(" rescue mode: not running dasdfmt")
> return
>
> - argv = ["-y", "-P", "-d", "cdl", "-b", "4096"]
> + # gather total cylinder count
> + argv = ["-t", "-v"] + self.commonArgv
> + for dasd in self._dasdlist:
> + buf = iutil.execWithCapture(self.dasdfmt, argv + [dasd],
> + stderr="/dev/tty5")
In order to detect such bugs more easily in the future, it would be
good, if you'd check the error level of external commands and react
appropriately.
> + for line in buf.splitlines():
> + if line.startswith("Drive Geometry: "):
> + # line will look like this:
> + # Drive Geometry: 3339 Cylinders * 15 Heads = 50085 Tracks
> + cyls = long(filter(lambda s: s, line.split(' '))[2])
> + self.totalCylinders += cyls
> + break
> +
> + # format DASDs
> + argv = ["-P"] + self.commonArgv
>
> if intf:
> title = P_("Formatting DASD Device", "Formatting DASD Devices", c)
> @@ -126,7 +140,7 @@ class DASD:
>
> for dasd in self._dasdlist:
> log.info("Running dasdfmt on %s" % (dasd,))
> - iutil.execWithCallback("/sbin/dasdfmt", argv + [dasd],
> + iutil.execWithCallback(self.dasdfmt, argv + [dasd],
> stdout="/dev/tty5", stderr="/dev/tty5",
> callback=self._updateProgressWindow,
> callback_data=pw, echo=False)
In order to detect such bugs more easily in the future, it would be
good, if you'd check the error level of external commands and react
appropriately.
> @@ -135,7 +149,7 @@ class DASD:
> else:
> for dasd in self._dasdlist:
> log.info("Running dasdfmt on %s" % (dasd,))
> - iutil.execWithRedirect("/sbin/dasdfmt", argv + [dasd],
> + iutil.execWithRedirect(self.dasdfmt, argv + [dasd],
> stdout="/dev/tty5", stderr="/dev/tty5")
>
> def addDASD(self, dasd):
> @@ -168,26 +182,6 @@ class DASD:
> self._completedCylinders += 1.0
> callback_data.set(self._completedCylinders / self.totalCylinders)
>
> - @property
> - def totalCylinders(self):
> - """ Total number of cylinders of all unformatted DASD devices. """
> - if self._totalCylinders:
> - return self._totalCylinders
> -
> - argv = ["-t", "-v", "-y", "-d", "cdl", "-b", "4096"]
> - for dasd in self._dasdlist:
> - buf = iutil.execWithCapture("/sbin/dasdfmt", argv + [dasd],
> - stderr="/dev/tty5")
> - for line in buf.splitlines():
> - if line.startswith("Drive Geometry: "):
> - # line will look like this:
> - # Drive Geometry: 3339 Cylinders * 15 Heads = 50085 Tracks
> - cyls = long(filter(lambda s: s, line.split(' '))[2])
> - self._totalCylinders += cyls
> - break
> -
> - return self._totalCylinders
> -
> # Create DASD singleton
> DASD = DASD()
>
Steffen
Linux on System z Development
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
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
