On 04/28/2010 05:31 AM, David Cantrell wrote:
> On Wed, 28 Apr 2010, Steffen Maier wrote:
>
>> 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/storage/__init__.py b/storage/__init__.py
>>> @@ -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.
>
> The iutil exec functions are intended to do this for us. Exceptions are
> caught in those functions and messages logged and/or exceptions raised
> depending on what happened.
Erm, no.
All iutil.execWith* only throw an exception, if they could not even
fork/exec the command.
iutil.execWithRedirect returns the error level no matter what its value
was. In this case here, dasdfmt most probably returned with !=0 and we
did not see this, since you ignore the return value.
iutil.execWithCapture returns the program output but proc.returncode not
at all, which is bad since we cannot detect if the executed command
exited with !=0.
iutil.execWithCallback returns os.WEXITSTATUS(status) and thus behaves
similar to execWithRedirect regarding the return of the programs error
level.
Those return values containing the program's error level need to be
checked and acted appropriately (fatal vs. non-fatal). Relying on a
follow-up exception because of an accidental division by zero is not the
right thing to do.
>>> @@ -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.
>
> See above.
Dito.
> Revised patch:
Looks good with regard to fixing the bug, but still I think the two
comments above should eventually be integrated---could be a different
patch, though.
> [PATCH] Offer to format unformatted DASD devices (#560702)
>
> On s390, if you had a single DASD that needed dasdfmt run, you would get
> a ZeroDivisionError traceback because the self._totalCylinders value was
> zero. The problem was caused by the DASD.totalCylinders property and
> how it initialized. It would initialize itself on the first call, but
> since we'd already fired off a dasdfmt process for the device, the one
> running to capture the cylinder count couldn't get access to the device.
>
> Users with more than one DASD needing a dasdfmt would not see the
> traceback, but would have an incorrect total cylinder count, so 100%
> would be reached when n-1 devices had been formatted.
>
> Honor the exclusiveDisks list in the storage object and log the
> /dev/disk/by-path alias for the device when logging the info message
> about what device was found that was unformatted.
>
> [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 | 4 ++-
> storage/__init__.py | 2 +-
> storage/dasd.py | 58
> +++++++++++++++++++++++---------------------------
> 3 files changed, 31 insertions(+), 33 deletions(-)
>
> diff --git a/iw/filter_gui.py b/iw/filter_gui.py
> index 6fed1a9..6291018 100644
> - --- a/iw/filter_gui.py
> +++ b/iw/filter_gui.py
> @@ -550,7 +550,9 @@ 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.exclusiveDisks,
> + 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..e4139c0 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.exclusiveDisks,
> self.zeroMbr)
> if self.anaconda.id.getUpgrade():
> clearPartType = CLEARPART_TYPE_NONE
> else:
> diff --git a/storage/dasd.py b/storage/dasd.py
> index 469957e..8eea176 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, exclusiveDisks, 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,11 @@ class DASD:
> status = f.read().strip()
> f.close()
>
> - - if status == "unformatted":
> - - log.info(" %s is an unformatted DASD" % (device,))
> + if status in ["unformatted"] and device not in exclusiveDisks:
> + bypath = deviceNameToDiskByPath("/dev/" + device)
> + log.info(" %s (%s) status is %s, needs dasdfmt" %
> (device,
> +
> bypath,
> +
> status,))
> self._dasdlist.append(device)
>
> if not len(self._dasdlist):
> @@ -116,7 +118,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")
> + 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 +142,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)
> @@ -135,7 +151,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 +184,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
