Merge lp:~bladernr/checkbox/1066118-modinfo-parser into lp:checkbox

[Jeff Lane ]

Added exception handling to address possible bad output from modinfo, by Jeff Lane

Fixes an issue where lines from modinfo that don't match the "key: value" format triggered a ValueError.
Most likely this is caused by blank lines appearing in the stream.

[Sylvain Pineau (sylvain-pineau)]

Even if having access to the machine where the crash occurred would have helped, I was suspecting blank lines too. This fix works perfectly.

review: Approve

[Marc Tardif (cr3)]

You know what would've helped? unit tests :)

[Jeff Lane  (bladernr)]

Possibly, but modinfo returns very specific output. Unfortunately, without access to the failing machine this was impossible to recreate. Sylvain and I both guessed blank lines, but I have never seen modinfo return a blank line before, ever. My original suspicion was the empty "Depends" field in modinfo output, but that splits out into ('depends','') which is fine.

But your point is well taken, we do need more unit tests for these things :)

=== modified file 'checkbox/parsers/modinfo.py'
--- checkbox/parsers/modinfo.py	2012-09-11 18:31:37 +0000
+++ checkbox/parsers/modinfo.py	2012-10-22 14:44:00 +0000
@@ -58,19 +58,26 @@
         for line in stream.splitlines():
             # At this point, stream should be the stdout from the modinfo
             # command, in a list.
-            key, data = line.split(':', 1)
-            data = data.strip()
-            # First, we need to handle alias, parm, firmware, and depends
-            # because there can be multiple lines of output for these.
-            if key in ('alias', 'depend', 'firmware', 'parm',):
-                self._modinfo[key].append(data)
-            # Now handle unknown keys
-            elif key not in self._modinfo.keys():
-                self._modinfo[key] = ("WARNING: Unknown Key %s providing "
+            try:
+                key, data = line.split(':', 1)
+            except ValueError:
+                # Most likely this will be caused by a blank line in the
+                # stream, so we just ignore it and move on.
+                continue                
+            else:
+                key = key.strip()
+                data = data.strip()
+                # First, we need to handle alias, parm, firmware, and depends
+                # because there can be multiple lines of output for these.
+                if key in ('alias', 'depend', 'firmware', 'parm',):
+                    self._modinfo[key].append(data)
+                # Now handle unknown keys
+                elif key not in self._modinfo.keys():
+                    self._modinfo[key] = ("WARNING: Unknown Key %s providing "
                                      "data: %s") % (key, data)
-            # And finally known keys
-            else:
-                self._modinfo[key] = data
+                # And finally known keys
+                else:
+                    self._modinfo[key] = data
 
     def get_all(self):
         return self._modinfo
 
=== modified file 'debian/changelog'
--- debian/changelog	2012-10-19 20:20:53 +0000
+++ debian/changelog	2012-10-22 14:44:00 +0000
@@ -74,8 +74,10 @@
     rather than fwts directly.  Ensured all are calling with the proper
     options. Fixed log name problems that caused log attachment jobs to not
     work.
+  * checkbox/parsers/modinfo.py - added exception handling to address possible
+    bad output from modinfo causing a ValueError to occur. (LP: #1066118)
 
- -- Jeff Lane <[e-mail]>  Thu, 18 Oct 2012 16:52:08 -0400
+ -- Jeff Lane <[e-mail]>  Mon, 22 Oct 2012 10:35:36 -0400
 
 checkbox (0.14.6) quantal; urgency=low