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

[Bruno Wolff III]

On Sat, Jul 24, 2010 at 22:53:30 -0500,
  Bruno Wolff III <bruno at wolff.to> 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.
-------------- next part --------------
diff --git a/imgcreate/creator.py b/imgcreate/creator.py
index 4deb49e..0ba5af5 100644
--- a/imgcreate/creator.py
+++ b/imgcreate/creator.py
@@ -385,9 +385,9 @@ class ImageCreator(object):
         try:
             self.__builddir = tempfile.mkdtemp(dir =  os.path.abspath(self.tmpdir),
                                                prefix = "imgcreate-")
-        except OSError, (err, msg):
+        except OSError, e:
             raise CreatorError("Failed create build directory in %s: %s" %
-                               (self.tmpdir, msg))
+                               (self.tmpdir, e.strerror))
 
     def __sanity_check(self):
         """Ensure that the config we've been given is sane."""
@@ -705,9 +705,9 @@ class ImageCreator(object):
             try:
                 subprocess.check_call([s.interp, script],
                                       preexec_fn = preexec, env = env)
-            except OSError, (err, msg):
+            except OSError, e:
                 raise CreatorError("Failed to execute %%post script "
-                                   "with '%s' : %s" % (s.interp, msg))
+                                   "with '%s' : %s" % (s.interp, e.strerror))
             except subprocess.CalledProcessError, err:
                 if s.errorOnFail:
                     raise CreatorError("%%post script failed with code %d "
diff --git a/imgcreate/debug.py b/imgcreate/debug.py
index c45047d..84b8d30 100644
--- a/imgcreate/debug.py
+++ b/imgcreate/debug.py
@@ -30,9 +30,9 @@ def handle_logging(option, opt, val, parser, logger, level):
 def handle_logfile(option, opt, val, parser, logger, stream):
     try:
         logfile = logging.FileHandler(val,"a")
-    except IOError, (err, msg):
+    except IOError, e:
         raise optparse.OptionValueError("Cannot open file '%s' : %s" %
-                                        (val, msg))
+                                        (val, e.strerror))
 
 
     logger.removeHandler(stream)
diff --git a/imgcreate/fs.py b/imgcreate/fs.py
index 16445a5..91252d4 100644
--- a/imgcreate/fs.py
+++ b/imgcreate/fs.py
@@ -36,8 +36,8 @@ def makedirs(dirname):
     """
     try:
         os.makedirs(dirname)
-    except OSError, (err, msg):
-        if err != errno.EEXIST:
+    except OSError, e:
+        if e.strerror != errno.EEXIST:
             raise
 
 def mksquashfs(in_img, out_img, compress_type):
diff --git a/imgcreate/kickstart.py b/imgcreate/kickstart.py
index 328043c..6600fd7 100644
--- a/imgcreate/kickstart.py
+++ b/imgcreate/kickstart.py
@@ -53,9 +53,11 @@ def read_kickstart(path):
     try:
         ksfile = urlgrabber.urlgrab(path)
         ks.readKickstart(ksfile)
-    except IOError, (err, msg):
+# Fallback to e.args[0] is a workaround for bugs in urlgragger and pykickstart.
+    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))
@@ -153,8 +155,8 @@ class TimezoneConfig(KickstartConfig):
         try:
             shutil.copyfile(self.path("/usr/share/zoneinfo/%s" %(tz,)),
                             self.path("/etc/localtime"))
-        except OSError, (errno, msg):
-            log.error("Error copying timezone: %s" %(msg,))
+        except OSError, e:
+            log.error("Error copying timezone: %s" %(e.strerror,))
 
 
 class AuthConfig(KickstartConfig):