[Fedora-livecd-list] Patch review request for exception handling

[Mads Kiilerich]

  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