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

[Bruno Wolff III]

On Sun, Jul 25, 2010 at 02:26:32 +0200,
  Mads Kiilerich <mads at kiilerich.com> wrote:
> 
> It puzzled me why you don't juse use the strerror attribute - that 
> usually works for me in other contexts.

Because there won't be a string there.

> 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.')

Right. But urlgrabber does something similar so I think we want to work
around these other packages raising errors with non-standard arguments
in a reasonable manner.

> 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.

I agree, but it is somewhat out of our control.

> Both these projects are closely related with Fedora. Couldn't this issue 
> be fixed there instead?

Possibly, but those may not be the only things that do bogus things with
IOError and OSError.

> 2:
> 
> OSError with one argument contradicts 
> http://docs.python.org/library/exceptions.html#exceptions.OSError and 
> would obviously be a bug.

Same with IOError.

> AFAIK there is no evidence that anybody does that.

At this point I agree with that. But I figured if we are protecting against
bogus uses of IOError it's reasonable to protect against bogus uses of
OSError in a similar manner.

> Introducing workarounds for this would add unnecessary bloat and risk to 
> the code. I don't think that is a good idea.

What do others think about the tradeoff here?

> 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?)

You understood the purpose correctly. So I need to coerce the list elements to
str and then do a join?

> (Note that args[-1:] will give a list with the last element from args - 
> if any)

That was the intent.

> 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:] ?

The intent was to handle the case with no args.

> 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

If you read that carefully, there is a backwards compatibility there.

Quoting from the doc:
When an EnvironmentError  exception is instantiated with a 3-tuple, the first
two items are available as above, while the third item is available on the
filename  attribute. However, for backwards compatibility, the args attribute
contains only a 2-tuple of the first two constructor arguments.

> My conclusion:
> 
> It would be better to just apply Marcs original patch.

The problem with that is that is going to post weird looking error messages
for standard IOError exceptions as it will combine both the errno and
strerror into a single error message. (At least that's my understanding
of what happens when you refer just to the error structure.)

> We should try to get the real issue solved in urlgrabber and pykickstart.

We can file bugs against them, but I don't see a big issue with being
defensive about the code handling.
If we go down that road then I think we need to use strerror rather than
Marc's patch. In the short run that is going to produce looney error
messages as well, but not generate another exception.

> Meanwhile I propose to use .strerror if available, with fallback to 
> using args[0] until the real issue gets fixed. For example with (untested!):

This seems reasonable. It's simpler and limited. However, I think we
should consider using strerror in other places even if we don't use the
or e.args[0] backup. This prevents additional exceptions and we just see
error messages that say None.

What do other people think? If we want to pursue this, I can look over the
exception checks and see if just this one has to deal with bogus stuff
from urlgrabber and pykickstart. Change the two arg exception checks to
one and use strerror for the string with or args[0] in the problem
place (or other if I see any) and file bugs against urlgrabber and
pykickstart?

> 
> --- /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