-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Sun, 16 Aug 2009, Steffen Maier wrote:
On 07/11/2009 01:44 AM, David Cantrell wrote:
On Fri, 10 Jul 2009, Steffen Maier wrote:
Activation happens in correct order and synchronizes with udev_settle.
Error cases are handled gracefully and report user readable error
messages.
Supports both old (RHEL 5) and new sysfs zfcp device driver interface.
---
storage/zfcp.py | 69
+++++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 62 insertions(+), 7 deletions(-)
+ if not os.path.exists(online):
+ loggedWriteLineToFile("/proc/cio_ignore",
+ "free %s" %(self.devnum,))
+ udev_settle()
+ sleep(1)
Is the sleep(1) necessary?
Not sure, tried without and it succeeded, so let's go without and add it
back in, should it fail to access sysfs attributes in the future.
Sounds fine.
+ except Exception, e:
+ raise ValueError, _(
+ "could not free zfcp device %s from device ignore
list (%s)"
+ %(self.devnum, e))
Generic catch-all exception blocks like this can make debugging later on
difficult. I would say we should be catching specific exceptions and
taking
necessary action rather than having a catch-all and logging an error.
Agreed. I'll send reworked patches.
The new patches look better.
The blocks above are catching any exception from:
Could someone please help me as a Python newbie identify which
exceptions may be thrown by the following functions?
After looking at the Python doc and some anaconda source, I came up with
this:
loggedWriteLineToFile()
IOError
udev_settle()
IOError, os.error, RuntimeError
file opening/closing
IOError
Looks more or less complete to me.
loggedWriteLineToFile() could be modified to handle exceptions, if we even
care. It opens a file, writes data, closes the file.
loggedWriteLineToFile does not know enough to throw a user readable
error message to be logged or displayed, so we don't handle exceptions
here but rather at the caller.
OK.
We do care very much about those exceptions. What you refer to as
accessing a "file" are sysfs attributes. They may be there or not, where
the latter is no reason to bail out from the installer. Writing to sysfs
attributes provided by device drivers is comparable to an ioctl and as
such they can return different errors, e.g. EINVAL, EBUSY, ENODEV, or
the like. Again we want to know about such errors, since knowing about
the device driver error will also help developers to determine user
problems.
BTW, ignoring errors is really a suboptimal habit at least for s390
specific code in anaconda. I wish people would become aware, that
correctly driving the device driver user space interface is important.
Bailing out of the installer or even silently dropping errors is
anything but helpful. I heard customers complaining that they could not
activate their FCP LUNs and didn't get any error. The "Add zFCP" dialog
just returned but their expected SCSI disk was still not there. I think
we don't want users to peek into /tmp/anaconda.log living in a volatile
ramdisk and hope to find something there, as long as we're able to
provide meaningful error messages. Users (of an enterprise Linux
distribution), especially on s390, expect resilience and right so.
That's fine and that's where your experience with s390 is helping us. On no
other platform do we need to load modules and then fiddle with bits in sysfs
to tell the driver what we want it to do. We're not generally used to that
sort of driver behavior. But if that's what s390 requires, that's fine. Your
new patches are done more in line with what I was saying. It's the catch-all
for Exception that is a red flag to me. From my point of view, if something
other than IOError occurs, we do want to see a traceback because it might
require a different recovery path than IOError does.
Likewise for the open file open and close.
Mostly sysfs attributes, of which I want to know if they were not there
but continue with the installer. An error here usually just means that
the user could not get his LUN activated.
OK.
udev_settle() we probably care about catching exceptions from it.
Assuming IOError, os.error, and RuntimeError might be thrown by
udev_settle, I guess I'm not interested in catching RuntimeError, since
this seems to be thrown if things in execWithRedirect are really wrong.
Not sure about os.error, though. Any comments?
RuntimeError usually indicates a larger problem. One that doesn't have a
recovery path in anaconda.
OSError probably also indicates something larger, but it does mean we are
still running and could possibly recover. Catch it if you want to.
- --
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAkqLL0gACgkQ5hsjjIy1VknzMQCeIbnoIfmihyo6nz63iIQ4F2wk
WWcAn1KYfEmMtQbfKY7KfuPYNO3Na639
=53We
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
