Bug 618002 - pykickstart raises IOError exceptions without providing errno
Summary: pykickstart raises IOError exceptions without providing errno
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: pykickstart
Version: 14
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Chris Lumens
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 551932
TreeView+ depends on / blocked
 
Reported: 2010-07-25 14:36 UTC by Bruno Wolff III
Modified: 2011-12-22 19:24 UTC (History)
3 users (show)

Fixed In Version: pykickstart-1.78-1
Clone Of:
Environment:
Last Closed: 2011-12-22 19:24:44 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Bruno Wolff III 2010-07-25 14:36:11 UTC
Description of problem:
In pykickstart there are three places where an idiom like the following are used:
raise IOError, formatErrorMsg(lineno, msg=_("Unable to open %%ksappend file: %s") % e.strerror)
When this is trapped by code that is expecting there to be both an errno and strerror on the args list this can result in another exception. Also when only argument is supplied e.errno and e.strerror are null. To handle this the exception handler needs not try to reference two arguments, but rather try to use strerror and if that isn't set fallback to arg[0] and hope that is the message.

Documentation of IOError exceptions is at:
http://docs.python.org/library/exceptions.html#exceptions.EnvironmentError

Version-Release number of selected component (if applicable):
pykickstart-1.69-1.fc13.noarch

How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:

Comment 1 Chris Lumens 2010-07-28 20:25:40 UTC
What errno value are you expecting?  Are you expecting something from the errno module, or would it be sufficient to propagate the grabber.URLGrabError value up?  For reference, those values are:

      URLGrabber error codes (0 -- 255)
        0    - everything looks good (you should never see this)
        1    - malformed url
        2    - local file doesn't exist
        3    - request for non-file local file (dir, etc)
        4    - IOError on fetch
        5    - OSError on fetch
        6    - no content length header when we expected one
        7    - HTTPException
        8    - Exceeded read limit (for urlread)
        9    - Requested byte range not satisfiable.
        10   - Byte range requested, but range support unavailable
        11   - Illegal reget mode
        12   - Socket timeout
        13   - malformed proxy url
        14   - HTTPError (includes .code and .exception attributes)
        15   - user abort
        16   - error writing to local file

Comment 2 Bruno Wolff III 2010-07-28 21:04:05 UTC
Mostly the issue was only getting one argument back instead of two. We have a work around in place so that that doesn't cause an exception.

For livecd-tools we currently have a work around that falls back to using e.args[0] if e.strerror isn't set. errno isn't currently used for the pykickstart and urlgrabber calls. I don't know if e.args[0] needs to be a integer to prevent strerror from not being set.

Other code might check the code to handle different ones in different ways.

I would expect an integer to be returned. I don't have a good feeling on whether returning something from urlgrabber's list or the standard errno module would be better.

Comment 3 Mads Kiilerich 2010-07-28 21:34:38 UTC
Comment fra a random python programmer using different python libraries:

I would expect pykickstart to raise pykickstart expections, and it is up to pykickstart to define how they should be interpreted. The beauty (or "beauty") of Python is that for example IOError and OSError often not are handled by libraries and will propagate until they hopefully is caught somewhere. pykickstart could also just let the urlgrabber exceptions slip through and document that urlgrabber exceptions might be raised and should be handled. It would be even better to replace it with or encapsulate it in pykickstarts own exception type.

I do however not expect any library to by itself raise exceptions defined in another library - neither explicitly nor by subclassing. If they do raise others exception types anyway then the exceptions should be 100% similar to the original source, both in syntax and semantics.

_My_ answer to Chris' question would thus be that the error codes look fine, as long as they are used in exceptions of a type where it is defined that it is that way - so it is fine in an urlgrabber or pykickstart exception, but not in IOError/OSError exceptions.

Now that you are mentioning urlgrabber: Note that bug 618003 has been filed for similar issues there.

Comment 4 Bug Zapper 2010-07-30 12:50:01 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 14 development cycle.
Changing version to '14'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 5 Jasper O'neal Hartline 2010-08-13 16:54:34 UTC
Do we have any more information on this?

Comment 6 Chris Lumens 2010-08-25 17:27:43 UTC
I've changed IOError to KickstartError, which was even being raised for certain read error conditions already.  When I build the new package, I'll update anaconda and system-config-kickstart, and send a mail to fedora-devel-list telling all other consumers of pykickstart to make a change too.

Thanks for the advice.

Comment 7 Mads Kiilerich 2010-08-25 22:50:12 UTC
Chris, can you give any advice on how these KickstartErrors should be handled? Do you consistently use the same number of parameters, or what is the recommended way to get and use the exception parameters?

(I assume it isn't explicitly documented and that the answer could be found in the code, but it could be nice to get an advice or statement of intention here.)

Comment 8 Chris Lumens 2010-09-10 17:47:30 UTC
A KickstartError is the base exception class in pykickstart.  It has only one value, which is a string containing a descriptive error message.  I try to use this class as little as possible, so right now it's only being used for IO errors (failure to download file, failure to open, etc.)


Note You need to log in before you can comment on or make changes to this bug.