Re: [Fedora-livecd-list] Patch review request for exception handling
Bruno Wolff III wrote, On 07/24/2010 10:30 PM:
I have attached the diff. The commit note is below. I like to get an
ack or
two on this patch. Thanks!
Fix issues due to raises supplying one argument when two are expected.
OSError and IOError normally have 2 or 3 arguments. However some routines
raise these with only one argument. This patch changes the handling to
use just the last argument provided (which would be the error message) or
the empty string if there isn't one.
I didn't print the list of arguments, since for these two exceptions the
first argument is an error number, that wouldn't look good in the error
message.
I didn't message with any of non-builtin exceptions, as I am assuming that
in those cases a mismatch is an error that should get fixed. Most of those
cases use the whole argument list as the error message in any case.
This resolves bug 551932.
NACK ;-)
1:
It puzzled me why you don't juse use the strerror attribute - that
usually works for me in other contexts.
It seems like the problem is caused by:
/usr/lib/python2.6/site-packages/pykickstart/parser.py:
raise IOError, formatErrorMsg(lineno, msg=_("Unable to open %%ksappend
file: %s") % e.strerror)
raise IOError, formatErrorMsg(0, msg=_("Unable to open input kickstart
file: %s") % e.strerror)
raise IOError, formatErrorMsg(0, msg=_("Unable to open input kickstart
file: %s") % e.strerror)
(where formatErrorMsg returns one string)
/usr/lib/python2.6/site-packages/urlgrabber/byterange.py
raise IOError('seek from end of file not supported.')
Based on
http://docs.python.org/library/exceptions.html#exceptions.EnvironmentError
and experience with how Python usually uses IOError this use with one
parameter is very confusing and unfortunate. I would call it a bug.
Both these projects are closely related with Fedora. Couldn't this issue
be fixed there instead?
2:
OSError with one argument contradicts
http://docs.python.org/library/exceptions.html#exceptions.OSError and
would obviously be a bug.
AFAIK there is no evidence that anybody does that.
Introducing workarounds for this would add unnecessary bloat and risk to
the code. I don't think that is a good idea.
3:
The ''.join(e.args[-1:]) snippet puzzles me.
First: This will crash on any non-string elements (and I assume the
intention here is to be tolerant to all kinds of garbage in args?)
(Note that args[-1:] will give a list with the last element from args -
if any)
Second: Do we really want to handle the case with no args? otherwise the
snippet could be written better as just e.args[-1] . Or perhaps you
meant args[1:] ?
Third: A to
http://docs.python.org/library/exceptions.html#exceptions.EnvironmentError
the optional third and last element in args could be a filename - we
probably don't want to report just that without any explanation
My conclusion:
It would be better to just apply Marcs original patch.
We should try to get the real issue solved in urlgrabber and pykickstart.
Meanwhile I propose to use .strerror if available, with fallback to
using args[0] until the real issue gets fixed. For example with (untested!):
--- /usr/lib/python2.6/site-packages/imgcreate/kickstart.py
2009-11-08 22:11:30.000000000 +0100
+++ kickstart.py 2010-07-25 02:14:29.000000000 +0200
@@ -53,9 +53,9 @@
try:
ksfile = urlgrabber.urlgrab(path)
ks.readKickstart(ksfile)
- except IOError, (err, msg):
+ except IOError, e:
raise errors.KickstartError("Failed to read kickstart file "
- "'%s' : %s" % (path, msg))
+ "'%s' : %s" % (path, e.strerror or
e.args[0]))
except kserrors.KickstartError, e:
raise errors.KickstartError("Failed to parse kickstart file "
"'%s' : %s" % (path, e))
/Mads