-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Wed, 28 Apr 2010, Steffen Maier wrote:
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.
I'm sorry, I was only thinking of instances where we'd get an exception from
the iutil.exec* functions, which would happen if we were missing the command
or there was an OSError starting the subprocess. I was not thinking of the
return values of dasdfmt.
That said, I do not want to change the calling semantics of execWithCapture()
because we use that all over the place. I've updated the patch to catch
exceptions and check the return value from execWithRedirect() and
execWithCallback(), much like we do in the storage/formats/fs.py code. On
error, I raise a DasdFormatError with the details collected. This is in line
with what we do with the existing storage code.
@@ -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.
Third revision:
[PATCH 1/2] 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 | 109 +++++++++++++++++++++++++++++---------------------
storage/errors.py | 3 +
4 files changed, 70 insertions(+), 48 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..a76943b 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.
"""
@@ -60,13 +62,12 @@ class DASD:
return
self.started = True
+ out = "/dev/tty5"
+ err = "/dev/tty5"
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 +82,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,27 +120,60 @@ 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=err)
+ 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
+ update = self._updateProgressWindow
+
+ title = P_("Formatting DASD Device", "Formatting DASD Devices", c)
+ msg = P_("Preparing %d DASD device for use with Linux..." % c,
+ "Preparing %d DASD devices for use with Linux..." % c, c)
if intf:
- - title = P_("Formatting DASD Device", "Formatting DASD Devices", c)
- - msg = P_("Preparing %d DASD device for use with Linux..." % c,
- - "Preparing %d DASD devices for use with Linux..." % c, c)
- - pw = intf.progressWindow(title, msg, 1.0)
+ if self.totalCylinders:
+ pw = intf.progressWindow(title, msg, 1.0)
+ else:
+ pw = intf.progressWindow(title, msg, 100, pulse=True)
- - for dasd in self._dasdlist:
- - log.info("Running dasdfmt on %s" % (dasd,))
- - iutil.execWithCallback("/sbin/dasdfmt", argv + [dasd],
- - stdout="/dev/tty5", stderr="/dev/tty5",
- - callback=self._updateProgressWindow,
- - callback_data=pw, echo=False)
- -
- - pw.pop()
- - else:
- - for dasd in self._dasdlist:
- - log.info("Running dasdfmt on %s" % (dasd,))
- - iutil.execWithRedirect("/sbin/dasdfmt", argv + [dasd],
- - stdout="/dev/tty5", stderr="/dev/tty5")
+ for dasd in self._dasdlist:
+ bypath = deviceNameToDiskByPath("/dev/" + dasd)
+ log.info("Running dasdfmt on %s" % (bypath,))
+ arglist = argv + [dasd]
+
+ try:
+ if intf and self.totalCylinders:
+ rc = iutil.execWithCallback(self.dasdfmt, arglist,
+ stdout=out, stderr=err,
+ callback=update,
+ callback_data=pw,
+ echo=False)
+ elif intf:
+ rc = iutil.execWithPulseProgress(self.dasdfmt, arglist,
+ stdout=out, stderr=err,
+ progress=pw)
+ else:
+ rc = iutil.execWithRedirect(self.dasdfmt, arglist,
+ stdout=out, stderr=err)
+ except Exception as e:
+ raise DasdFormatError(e, bypath)
+
+ if rc:
+ raise DasdFormatError("dasdfmt failed: %s" % rc, bypath)
+
+ if intf:
+ pw.pop()
def addDASD(self, dasd):
""" Adds a DASDDevice to the internal list of DASDs. """
@@ -168,26 +205,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()
diff --git a/storage/errors.py b/storage/errors.py
index a0f5f60..c4d4313 100644
- --- a/storage/errors.py
+++ b/storage/errors.py
@@ -148,3 +148,6 @@ class UdevError(StorageError):
class UnrecognizedFSTabEntryError(StorageError):
pass
+# dasd
+class DasdFormatError(StorageError):
+ pass
- --
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAkvYbk0ACgkQ5hsjjIy1VkkbzACfZjMkUL/AViQlh4ARkRhjDG0J
ojIAoNo9JW6RH26JpV3eK7mRwotB/qHj
=ijKr
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
