Patch review request for exception handling

[Bruno Wolff III]

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.

[Jasper Hartline]

On Sat, Jul 24, 2010 at 1:30 PM, Bruno Wolff III <bruno(a)wolff.to&gt; wrote:
>  I have attached the diff. The commit note is below. I like to get an
> ack or
>  two on this patch. Thanks! 
The patches operate fine in testing here.
I would give this a thumbs up.

The only concern I have is an inaccurate error message displayed which may
be a fault of pykickstart.. where it says it fails to read livecd.ks
when this file actually does exist
and then says the error in that kickstart file is on line 0, when in
fact it is on line 11.

Doesn't make much sense. Curious if this is a side product of
livecd-creator code or
a blatant pykickstart bug.

(Thumbs up on the exception handling patches from me)

[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

[Bruno Wolff III]

On Sat, Jul 24, 2010 at 14:47:29 -0700,
  Jasper Hartline <jasper.hartline(a)gmail.com&gt; wrote:
>  
>  The only concern I have is an inaccurate error message displayed which may
>  be a fault of pykickstart.. where it says it fails to read livecd.ks
>  when this file actually does exist
>  and then says the error in that kickstart file is on line 0, when in
>  fact it is on line 11.
>  
>  Doesn't make much sense. Curious if this is a side product of
>  livecd-creator code or
>  a blatant pykickstart bug. 
The line number issue is in pykickstart. I think that is out of scope
for livecd-tools. The line number is put into the returned string; it
isn't supposed to be returned as an argument that livecd-creator can
do something with.

[Bruno Wolff III]

On Sun, Jul 25, 2010 at 02:26:32 +0200,
  Mads Kiilerich <mads(a)kiilerich.com&gt; 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

[Bruno Wolff III]

On Sat, Jul 24, 2010 at 22:53:30 -0500,
  Bruno Wolff III <bruno(a)wolff.to&gt; wrote:
>  
>  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? 
Going down this road, I have something worked up that would need to be
tested before committing. The patch is attached.

The git commit log:

    Fix issue with improper exception args used by urlgrabber and pykickstart.
    
    urlgrabber and pykickstart don't always supply both and error number and
    an error message when raising an IOError exception. This was causing
    an exception while processing the exception for some errors.
    
    To prevent exceptions from insufficient arguments, the exception calls
    were changed to use a single variable instead of a tuple and to use
    strerror instead of the second argument.
    
    As a temporary work around for urlgrabber and pykickstart issues, if
    strerror is null, args[0] is used as a fallback so that we get a
    reasonable error message.
    
    This resolves bug 551932.

[Bruno Wolff III]

On Sun, Jul 25, 2010 at 02:08:26 -0500,
  Bruno Wolff III <bruno(a)wolff.to&gt; wrote:
>  
>  Going down this road, I have something worked up that would need to be
>  tested before committing. The patch is attached. 
There is a error in the patch I used 'strerror' instead of 'errno' in one
place. The corrected patch is attached.

[Jasper Hartline]

I have tested normal livecd creation, I have also tested
livecd-creation with invalid kickstart data
and these patches work fine in the short run. I would like to see these patches
committed as this provides temporary relief of the underlying issues
with pykickstart
and urlgrabber in the immediate time frame.

This will buy us time to work on fixing the underlying issues mentioned above.
Thanks Bruno, +1 from me.

>  Going down this road, I have something worked up that would need to
> be
>  tested before committing. The patch is attached.
>
>  The git commit log:
>
>     Fix issue with improper exception args used by urlgrabber and pykickstart.
>
>     urlgrabber and pykickstart don't always supply both and error number and
>     an error message when raising an IOError exception. This was causing
>     an exception while processing the exception for some errors.
>
>     To prevent exceptions from insufficient arguments, the exception calls
>     were changed to use a single variable instead of a tuple and to use
>     strerror instead of the second argument.
>
>     As a temporary work around for urlgrabber and pykickstart issues, if
>     strerror is null, args[0] is used as a fallback so that we get a
>     reasonable error message.
>
>     This resolves bug 551932.

[Mads Kiilerich]

  Bruno Wolff III wrote, On 07/25/2010 09:28 AM:
>  On Sun, Jul 25, 2010 at 02:08:26 -0500,
>     Bruno Wolff III<bruno(a)wolff.to>  wrote:
> > Going down this road, I have something worked up that would need to be
> > tested before committing. The patch is attached.
>  There is a error in the patch I used 'strerror' instead of 'errno' in
> one
>  place. The corrected patch is attached. 
Agreed, thanks, this looks like a very good patch and I have no 
objections. Handling OSError this way seems elegant and correct.

I haven't tested the patch.

/Mads

[Bruno Wolff III]

On Sun, Jul 25, 2010 at 03:09:35 -0700,
  Jasper Hartline <jasper.hartline(a)gmail.com&gt; wrote:
>  
>  This will buy us time to work on fixing the underlying issues mentioned above.
>  Thanks Bruno, +1 from me. 
I'll get this commited and make sure bug reports are filed against
urlgrabber and pykickstart.

Thanks to everyone who provided feedback on the patches.