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