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.
>> + 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 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
> 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.
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.
> 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.
> 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?
Steffen
Linux on System z Development
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Erich Baier
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
