Clean up sos_get_command_output() and friends

Returning a 3-tuple has always been ugly. It gets worse as the
parameter list for this family of functions grows. Worse, the 3rd
member of the tuple is unused and is always set to 0.

Rip the whole mess out and return a single, simple dictionary
object with 'status' and 'output' keys.

Update utilities_tests to reflect the new interface.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

7 files changed, 60 insertions, 47 deletions

diff --git a/sos/plugins/__init__.py b/sos/plugins/__init__.py
index 33d13572..8a938a9b 100644
--- a/sos/plugins/__init__.py
+++ b/sos/plugins/__init__.py
@@ -426,13 +426,13 @@ class Plugin(object):
         self.copy_paths.update(copy_paths)
 
     def get_command_output(self, prog, timeout=300):
-        (status, output, runtime) = sos_get_command_output(prog, timeout)
-        if status == 124:
+        result = sos_get_command_output(prog, timeout)
+        if result['status'] == 124:
             self.soslog.warning("command '%s' timed out after %ds"
                     % (prog, timeout))
-        if status == 127:
+        if result['status'] == 127:
             self.soslog.info("could not run '%s': command not found" % prog)
-        return (status, output, runtime)
+        return result
 
     def call_ext_prog(self, prog, timeout=300):
         """Execute a command independantly of the output gathering part of
@@ -446,8 +446,7 @@ class Plugin(object):
         sosreport and check the return code. Return True for a return code of 0
         and False otherwise.
         """
-        (status, output, runtime) = self.call_ext_prog(prog)
-        return (status == 0)
+        return (self.call_ext_prog(prog)['status'] == 0)
 
 
     def add_cmd_output(self, exe, suggest_filename=None, root_symlink=None, timeout=300):
@@ -503,8 +502,8 @@ class Plugin(object):
         report.
         """
         # pylint: disable-msg = W0612
-        status, shout, runtime = self.get_command_output(exe, timeout=timeout)
-        if (status == 127):
+        result = self.get_command_output(exe, timeout=timeout)
+        if (result['status'] == 127):
             return None
 
         if suggest_filename:
@@ -513,13 +512,17 @@ class Plugin(object):
             outfn = self.make_command_filename(exe)
 
         outfn_strip = outfn[len(self.commons['cmddir'])+1:]
-        self.archive.add_string(shout, outfn)
+        self.archive.add_string(result['output'], outfn)
         if root_symlink:
             self.archive.add_link(outfn, root_symlink)
 
         # save info for later
         self.executed_commands.append({'exe': exe, 'file':outfn_strip}) # save in our list
-        self.commons['xmlreport'].add_command(cmdline=exe,exitcode=status,f_stdout=outfn_strip,runtime=runtime)
+        self.commons['xmlreport'].add_command(
+            cmdline=exe,
+            exitcode=result['status'],
+            f_stdout=outfn_strip
+        )
 
         return os.path.join(self.archive.get_archive_path(), outfn)
 
diff --git a/sos/plugins/cluster.py b/sos/plugins/cluster.py
index f4ed405b..5b13294b 100644
--- a/sos/plugins/cluster.py
+++ b/sos/plugins/cluster.py
@@ -102,9 +102,12 @@ class Cluster(Plugin, RedHatPlugin):
                     % (crm_dest, crm_from))
 
     def do_lockdump(self):
-        status, output, time = self.call_ext_prog("dlm_tool ls")
+        dlm_tool = "dlm_tool ls"
+        result = self.call_ext_prog(dlm_tool)
+        if result['status'] != 0:
+            return
         for lockspace in re.compile(r'^name\s+([^\s]+)$',
-                re.MULTILINE).findall(output):
+                re.MULTILINE).findall(result['output']):
             self.add_cmd_output("dlm_tool lockdebug -svw '%s'" % lockspace,
                         suggest_filename = "dlm_locks_%s" % lockspace)
 
diff --git a/sos/plugins/networking.py b/sos/plugins/networking.py
index 27a55b7c..45dd8bbf 100644
--- a/sos/plugins/networking.py
+++ b/sos/plugins/networking.py
@@ -27,12 +27,16 @@ class Networking(Plugin):
     def setup(self):
         super(Networking, self).setup()
 
-    def get_bridge_name(self,brctl_out):
+    def get_bridge_name(self,brctl_file):
         """Return a list for which items are bridge name according to the
         output of brctl show stored in brctl_file.
         """
         out=[]
-        for line in brctl_out[1].splitlines():
+        try:
+            brctl_out = open(brctl_file).read()
+        except:
+            return out
+        for line in brctl_out.splitlines():
             if line.startswith("bridge name") \
                or line.isspace() \
                or line[:1].isspace():
@@ -46,7 +50,7 @@ class Networking(Plugin):
         names taken from the output of "ip -o link".
         """
         out={}
-        for line in ip_link_out[1].splitlines():
+        for line in ip_link_out.splitlines():
             match=re.match('.*link/ether', line)
             if match:
                 iface=match.string.split(':')[1].lstrip()
@@ -61,8 +65,7 @@ class Networking(Plugin):
         relevant rules in that table """
 
 
-        (status, output, time) = self.call_ext_prog("lsmod | grep -q "+tablename)
-        if status == 0:
+        if self.check_ext_prog("grep -q %s /proc/modules" % tablename):
             cmd = "iptables -t "+tablename+" -nvL"
             self.add_cmd_output(cmd)
 
@@ -101,9 +104,9 @@ class Networking(Plugin):
         self.add_cmd_output("ip mroute show")
         self.add_cmd_output("ip maddr show")
         self.add_cmd_output("ip neigh show")
-        ip_link_out=self.call_ext_prog("ip -o link")
-        if ip_link_out:
-            for eth in self.get_eth_interfaces(ip_link_out):
+        ip_link_result=self.call_ext_prog("ip -o link")
+        if ip_link_result['status'] == 0:
+            for eth in self.get_eth_interfaces(ip_link_result['output']):
                 self.add_cmd_output("ethtool "+eth)
                 self.add_cmd_output("ethtool -i "+eth)
                 self.add_cmd_output("ethtool -k "+eth)
@@ -112,10 +115,9 @@ class Networking(Plugin):
                 self.add_cmd_output("ethtool -c "+eth)
                 self.add_cmd_output("ethtool -g "+eth)
 
-        brctl_file=self.add_cmd_output("brctl show")
-        brctl_out=self.call_ext_prog("brctl show")
-        if brctl_out:
-            for br_name in self.get_bridge_name(brctl_out):
+        brctl_file=self.get_cmd_output_now("brctl show")
+        if brctl_file:
+            for br_name in self.get_bridge_name(brctl_file):
                 self.add_cmd_output("brctl showstp "+br_name)
 
         if self.get_option("traceroute"):
diff --git a/sos/plugins/neutron.py b/sos/plugins/neutron.py
index 2200606b..ced85a07 100644
--- a/sos/plugins/neutron.py
+++ b/sos/plugins/neutron.py
@@ -53,10 +53,11 @@ class Neutron(Plugin):
     def get_ovs_dumps(self):
         # Check to see if we are using the Open vSwitch plugin. If not we 
         # should be able to skip the rest of the dump.
-        ovs_conf_check_out = self.call_ext_prog('grep "^core_plugin.*openvswitch" ' +
+        ovs_conf_check = self.call_ext_prog('grep "^core_plugin.*openvswitch" ' +
             ("/etc/%s/*.conf" + self.component_name))
-
-        if not ovs_conf_check_out or len(ovs_conf_check_out[1].splitlines()) == 0:
+        if not (ovs_conf_check['status'] == 0):
+            return
+        if len(ovs_conf_check['output'].splitlines()) == 0:
             return
 
         # The '-s' option enables dumping of packet counters on the
@@ -73,10 +74,13 @@ class Neutron(Plugin):
         # in the short term: create a local instance and "borrow" some of the
         # functionality, or simply copy some of the functionality.
         prefixes = ["qdhcp", "qrouter"]
-        nslist = self.call_ext_prog("ip netns")
+        ip_netns_result = self.call_ext_prog("ip netns")
+        if not (ip_netns_result['status'] == 0):
+            return
+        nslist = ip_netns_result['output']
         lease_directories = []
         if nslist:
-            for nsname in nslist[1].splitlines():
+            for nsname in nslist.splitlines():
                 prefix, netid = nsname.split('-', 1)
                 if len(netid) > 0 and prefix in prefixes:
                     self.ns_gather_data(nsname)
@@ -90,7 +94,7 @@ class Neutron(Plugin):
         output of ifconifg-a stored in ifconfig_file.
         """
         out={}
-        for line in ip_addr_out[1].splitlines():
+        for line in ip_addr_out.splitlines():
             match=re.match('.*link/ether', line)
             if match:
                 int=match.string.split(':')[1].lstrip()
@@ -103,9 +107,9 @@ class Neutron(Plugin):
         self.add_cmd_output(cmd_prefix + "ifconfig -a")
         self.add_cmd_output(cmd_prefix + "route -n")
         # borrowed from networking plugin
-        ip_addr_out=self.call_ext_prog(cmd_prefix + "ip -o addr")
-        if ip_addr_out:
-            for eth in self.get_interface_name(ip_addr_out):
+        ip_addr_result=self.call_ext_prog(cmd_prefix + "ip -o addr")
+        if ip_addr_result['status'] == 0:
+            for eth in self.get_interface_name(ip_addr_result['output']):
                 self.add_cmd_output(cmd_prefix + "ethtool "+eth)
                 self.add_cmd_output(cmd_prefix + "ethtool -i "+eth)
                 self.add_cmd_output(cmd_prefix + "ethtool -k "+eth)
diff --git a/sos/plugins/postgresql.py b/sos/plugins/postgresql.py
index 69084af0..290b6a5d 100644
--- a/sos/plugins/postgresql.py
+++ b/sos/plugins/postgresql.py
@@ -60,17 +60,17 @@ class PostgreSQL(Plugin):
                 dest_file,
                 self.get_option("dbname")
             )
-        (status, output, rtime) = self.call_ext_prog(cmd)
+        result = self.call_ext_prog(cmd)
         if old_env_pgpassword is not None:
             os.environ["PGPASSWORD"] = str(old_env_pgpassword)
-        if (status == 0):
+        if (result['status'] == 0):
             self.add_copy_spec(dest_file)
         else:
             self.soslog.error(
-                "Unable to execute pg_dump. Error(%s)" % (output)
+                "Unable to execute pg_dump. Error(%s)" % (result['output'])
             )
             self.add_alert(
-                "ERROR: Unable to execute pg_dump.  Error(%s)" % (output)
+                "ERROR: Unable to execute pg_dump. Error(%s)" % (result['output'])
             )
 
     def setup(self):
diff --git a/sos/utilities.py b/sos/utilities.py
index 8f69ad98..fae424ad 100644
--- a/sos/utilities.py
+++ b/sos/utilities.py
@@ -147,7 +147,7 @@ def sos_get_command_output(command, timeout=300):
     if p.returncode == 127:
         stdout = ""
     
-    return (p.returncode, stdout.decode('utf-8'), 0)
+    return {'status': p.returncode, 'output': stdout.decode('utf-8')}
 
 def import_module(module_fqname, superclasses=None):
     """Imports the module module_fqname and returns a list of defined classes
@@ -165,9 +165,10 @@ def import_module(module_fqname, superclasses=None):
     return modules
 
 def shell_out(cmd):
-    """Uses subprocess.Popen to make a system call and returns stdout.
-    Does not handle exceptions."""
-    return sos_get_command_output(cmd)[1]
+    """Shell out to an external command and return the output or the empty
+    string in case of error.
+    """
+    return sos_get_command_output(cmd)['output']
 
 class ImporterHelper(object):
     """Provides a list of modules that can be imported in a package.
diff --git a/tests/utilities_tests.py b/tests/utilities_tests.py
index 22b2bbea..60087c18 100644
--- a/tests/utilities_tests.py
+++ b/tests/utilities_tests.py
@@ -58,15 +58,15 @@ class ExecutableTest(unittest.TestCase):
 
     def test_output(self):
         path = os.path.join(TEST_DIR, 'test_exe.py')
-        ret, out, junk = sos_get_command_output(path)
-        self.assertEquals(ret, 0)
-        self.assertEquals(out, "executed\n")
+        result = sos_get_command_output(path)
+        self.assertEquals(result['status'], 0)
+        self.assertEquals(result['output'], "executed\n")
 
     def test_output_non_exe(self):
         path = os.path.join(TEST_DIR, 'utility_tests.py')
-        ret, out, junk = sos_get_command_output(path)
-        self.assertEquals(ret, 127)
-        self.assertEquals(out, "")
+        result = sos_get_command_output(path)
+        self.assertEquals(result['status'], 127)
+        self.assertEquals(result['output'], "")
 
     def test_shell_out(self):
         path = os.path.join(TEST_DIR, 'test_exe.py')