Merge pull request #1865 from sfromm/issue1738

Add run_command to module_common and update modules to use it.

lib/ansible/module_common.py

@@ -677,6 +677,52 @@ class AnsibleModule(object):
                 self.set_context_if_different(src, context, False)
         os.rename(src, dest)
 
+    def run_command(self, args, **kwargs):
+        '''
+        Execute a command, returns rc, stdout, and stderr.
+        args is the command to run
+        If args is a list, the command will be run with shell=False.
+        Otherwise, the command will be run with shell=True when args is a string.
+        kwargs is a dict of keyword arguments:
+        - fail_on_rc_non_zero (boolean) Whether to call fail_json in case of
+                                        non zero RC.  Default is False.
+        - close_fds (boolean) See documentation for subprocess.Popen().
+                              Default is False.
+        - executable (string) See documentation for subprocess.Popen().
+                              Default is None.
+        '''
+        if isinstance(args, list):
+            kwargs['shell'] = False
+        elif isinstance(args, basestring):
+            kwargs['shell'] = True
+        else:
+            msg = "Argument 'args' to run_command must be list or string"
+            self.fail_json(rc=257, cmd=args, msg=msg)
+        if 'fail_on_rc_non_zero' not in kwargs:
+            kwargs['fail_on_rc_non_zero'] = False
+        if 'close_fds' not in kwargs:
+            kwargs['close_fds'] = False
+        if 'executable' not in kwargs:
+            kwargs['executable'] = None
+        rc = 0
+        msg = None
+        try:
+            cmd = subprocess.Popen(args,
+                                   executable=kwargs['executable'],
+                                   shell=kwargs['shell'],
+                                   close_fds=kwargs['close_fds'],
+                                   stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            out, err = cmd.communicate()
+            rc = cmd.returncode
+        except (OSError, IOError), e:
+            self.fail_json(rc=e.errno, msg=str(e), cmd=args)
+        except:
+            self.fail_json(rc=257, msg=traceback.format_exc(), cmd=args)
+        if rc != 0 and kwargs['fail_on_rc_non_zero']:
+            msg = err.rstrip()
+            self.fail_json(cmd=args, rc=rc, stdout=out, stderr=err, msg=msg)
+        return (rc, out, err)
+
 # == END DYNAMICALLY INSERTED CODE ===
 
 """

library/apt

@@ -93,22 +93,6 @@ warnings.filterwarnings('ignore', "apt API not stable yet", FutureWarning)
 APT_PATH = "/usr/bin/apt-get"
 APT = "DEBIAN_FRONTEND=noninteractive DEBIAN_PRIORITY=critical %s" % APT_PATH
 
-def run_apt(command):
-    try:
-        cmd = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
-    except (OSError, IOError), e:
-        rc = 1
-        err = str(e)
-        out = ''
-    except:
-        rc = 1
-        err = traceback.format_exc()
-        out = ''
-    else:
-        rc = cmd.returncode
-        return rc, out, err
-
 def package_split(pkgspec):
     parts = pkgspec.split('=')
     if len(parts) > 1:
@@ -154,7 +138,7 @@ def install(m, pkgspec, cache, upgrade=False, default_release=None, install_reco
         if not install_recommends:
             cmd += " --no-install-recommends"
 
-        rc, out, err = run_apt(cmd)
+        rc, out, err = m.run_command(cmd)
         if rc:
             m.fail_json(msg="'apt-get install %s' failed: %s" % (packages, err))
         else:
@@ -175,7 +159,7 @@ def remove(m, pkgspec, cache, purge=False):
     else:
         purge = '--purge' if purge else ''
         cmd = "%s -q -y %s remove %s" % (APT, purge,packages)
-        rc, out, err = run_apt(cmd)
+        rc, out, err = m.run_command(cmd)
         if rc:
             m.fail_json(msg="'apt-get remove %s' failed: %s" % (packages, err))
         m.exit_json(changed=True)

library/apt_repository

@@ -59,15 +59,10 @@ import platform
 APT = "/usr/bin/apt-get"
 ADD_APT_REPO = 'add-apt-repository'
 
-def _run(cmd):
+def check_cmd_needs_y():
     if platform.dist()[0] == 'debian' or float(platform.dist()[1]) >= 11.10:
-        cmd = cmd + ' -y'
+        return True
-    # returns (rc, stdout, stderr) from shell command
+    return False
-    process = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE, shell=True)
-    stdout, stderr = process.communicate()
-    return (process.returncode, stdout, stderr)
-
 
 def main():
     add_apt_repository = None
@@ -80,11 +75,14 @@ def main():
     module = AnsibleModule(argument_spec=arg_spec)
 
     add_apt_repository = module.get_bin_path(ADD_APT_REPO, True)
+    if check_cmd_needs_y():
+        add_apt_repository += ' -y'
 
     repo = module.params['repo']
     state = module.params['state']
 
-    rc, out, err = _run('%s "%s" --remove' % (add_apt_repository, repo))
+    cmd = '%s "%s" --remove' % (add_apt_repository, repo)
+    rc, out, err = module.run_command(cmd)
     existed = 'Error' not in out
 
     if state == 'absent':
@@ -95,7 +93,7 @@ def main():
 
     cmd = '%s "%s"' % (add_apt_repository, repo)
 
-    rc, out, err = _run(cmd)
+    rc, out, err = module.run_command(cmd)
 
     changed = rc == 0 and not existed
 
@@ -103,7 +101,7 @@ def main():
         module.fail_json(msg=err)
 
     if changed:
-        rc, out, err = _run('%s update' % APT)
+        rc, out, err = module.run_command('%s update' % APT)
 
     module.exit_json(changed=changed, repo=repo, state=state)
 

library/command

@@ -18,7 +18,6 @@
 # You should have received a copy of the GNU General Public License
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
-import subprocess
 import sys
 import datetime
 import traceback
@@ -100,13 +99,7 @@ def main():
         args = shlex.split(args)
     startd = datetime.datetime.now()
 
-    try:
+    rc, out, err = module.run_command(args, shell=shell, executable=executable)
-        cmd = subprocess.Popen(args, executable=executable, shell=shell, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
-    except (OSError, IOError), e:
-        module.fail_json(rc=e.errno, msg=str(e), cmd=args)
-    except:
-        module.fail_json(rc=257, msg=traceback.format_exc(), cmd=args)
 
     endd = datetime.datetime.now()
     delta = endd - startd
@@ -120,7 +113,7 @@ def main():
         cmd     = args,
         stdout  = out.rstrip("\r\n"),
         stderr  = err.rstrip("\r\n"),
-        rc      = cmd.returncode,
+        rc      = rc,
         start   = str(startd),
         end     = str(endd),
         delta   = str(delta),

library/cron

@@ -122,19 +122,13 @@ author: Dane Summers
 import re
 import tempfile
 
-def get_jobs_file(user,tmpfile):
+def get_jobs_file(module, user, tmpfile):
     cmd = "crontab -l %s > %s" % (user,tmpfile)
-    cmd = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    return module.run_command(cmd)
-    (out, err) = cmd.communicate()
-    rc = cmd.returncode
-    return (rc, out, err)
 
-def install_jobs(user,tmpfile):
+def install_jobs(module, user, tmpfile):
     cmd = "crontab %s %s" % (user,tmpfile)
-    cmd = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    return module.run_command(cmd)
-    (out, err) = cmd.communicate()
-    rc = cmd.returncode
-    return (rc, out, err)
 
 def get_jobs(tmpfile):
     lines = open(tmpfile).read().splitlines()
@@ -155,13 +149,9 @@ def find_job(name,tmpfile):
             return j
     return []
 
-def add_job(name,job,tmpfile):
+def add_job(module,name,job,tmpfile):
     cmd = "echo \"#Ansible: %s\n%s\" >> %s" % (name,job,tmpfile)
-    cmd = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    return module.run_command(cmd)
-    (out, err) = cmd.communicate()
-    rc = cmd.returncode
-    return (rc, out, err)
-
 
 def update_job(name,job,tmpfile):
     return _update_job(name,job,tmpfile,do_add_job)
@@ -269,17 +259,17 @@ def main():
     if job is None and do_install:
         module.fail_json(msg="You must specify 'job' to install a new cron job")
     tmpfile = tempfile.NamedTemporaryFile()
-    (rc, out, err) = get_jobs_file(user,tmpfile.name)
+    (rc, out, err) = get_jobs_file(module,user,tmpfile.name)
     if rc != 0 and rc != 1: # 1 can mean that there are no jobs.
         module.fail_json(msg=err)
     (handle,backupfile) = tempfile.mkstemp(prefix='crontab')
-    (rc, out, err) = get_jobs_file(user,backupfile)
+    (rc, out, err) = get_jobs_file(module,user,backupfile)
     if rc != 0 and rc != 1:
         module.fail_json(msg=err)
     old_job = find_job(name,backupfile)
     if do_install:
         if len(old_job) == 0:
-            (rc, out, err) = add_job(name,job,tmpfile.name)
+            (rc, out, err) = add_job(module,name,job,tmpfile.name)
             changed = True
         if len(old_job) > 0 and old_job[1] != job:
             (rc, out, err) = update_job(name,job,tmpfile.name)
@@ -293,7 +283,7 @@ def main():
     if changed:
         if backup:
             module.backup_local(backupfile)
-        (rc, out, err) = install_jobs(user,tmpfile.name)
+        (rc, out, err) = install_jobs(module,user,tmpfile.name)
         if (rc != 0):
             module.fail_json(msg=err)
 

library/easy_install

@@ -55,27 +55,19 @@ requirements: [ "virtualenv" ]
 author: Matt Wright
 '''
 
-def _ensure_virtualenv(env, virtualenv):
+def _ensure_virtualenv(module, env, virtualenv):
     if os.path.exists(os.path.join(env, 'bin', 'activate')):
         return 0, '', ''
     else:
-        return _run('%s %s' % (virtualenv, env))
+        return module.run_command('%s %s' % (virtualenv, env))
 
 
-def _is_package_installed(name, easy_install):
+def _is_package_installed(module, name, easy_install):
     cmd = '%s --dry-run %s' % (easy_install, name)
-    rc, status_stdout, status_stderr = _run(cmd)
+    rc, status_stdout, status_stderr = module.run_command(cmd)
     return not ('Reading' in status_stdout or 'Downloading' in status_stdout)
 
 
-def _run(cmd):
-    # returns (rc, stdout, stderr) from shell command
-    process = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE, shell=True)
-    stdout, stderr = process.communicate()
-    return (process.returncode, stdout, stderr)
-
-
 def main():
     arg_spec = dict(
         name=dict(required=True),
@@ -97,7 +89,7 @@ def main():
         if virtualenv is None:
             module.fail_json(msg='virtualenv is not installed')
 
-        rc_venv, out_venv, err_venv = _ensure_virtualenv(env, virtualenv)
+        rc_venv, out_venv, err_venv = _ensure_virtualenv(module, env, virtualenv)
 
         rc += rc_venv
         out += out_venv
@@ -105,11 +97,11 @@ def main():
 
     cmd = None
     changed = False
-    installed = _is_package_installed(name, easy_install)
+    installed = _is_package_installed(module, name, easy_install)
 
     if not installed:
         cmd = '%s %s' % (easy_install, name)
-        rc_pip, out_pip, err_pip = _run(cmd)
+        rc_pip, out_pip, err_pip = module.run_command(cmd)
 
         rc += rc_pip
         out += out_pip

library/facter

@@ -38,24 +38,13 @@ requirements: [ "facter", "ruby-json" ]
 author: Michael DeHaan
 '''
 
-import subprocess
-
-def get_facter_data():
-    p = subprocess.Popen(["/usr/bin/env", "facter", "--json"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    (out, err) = p.communicate()
-    rc = p.returncode
-    return rc, out, err
-
-
 def main():
     module = AnsibleModule(
         argument_spec = dict()
     )
 
-    rc, out, err = get_facter_data()
+    cmd = ["/usr/bin/env", "facter", "--json"]
-    if rc != 0:
+    rc, out, err = module.run_command(cmd, fail_on_rc_non_zero=True)
-        module.fail_json(msg=err)
-    else:
     module.exit_json(**json.loads(out))
 
 # this is magic, see lib/ansible/module_common.py

library/fireball

@@ -70,7 +70,6 @@ import base64
 import syslog
 import signal
 import time
-import subprocess
 import signal
 
 syslog.openlog('ansible-%s' % os.path.basename(__file__))
@@ -153,15 +152,14 @@ def command(data):
         return dict(failed=True, msg='internal error: executable is required')
 
     log("executing: %s" % data['cmd'])
-    p = subprocess.Popen(data['cmd'], executable=data['executable'], shell=True, stdout=subprocess.PIPE, close_fds=True)
+    rc, stdout, stderr = module.run_command(data['cmd'], executable=data['executable'], close_fds=True)
-    (stdout, stderr) = p.communicate()
     if stdout is None:
         stdout = ''
     if stderr is None:
         stderr = ''
     log("got stdout: %s" % stdout)
 
-    return dict(rc=p.returncode, stdout=stdout, stderr=stderr)
+    return dict(rc=rc, stdout=stdout, stderr=stderr)
 
 def fetch(data):
     if 'in_path' not in data:

library/git

@@ -64,12 +64,6 @@ examples:
 import re
 import tempfile
 
-def _run(args):
-    cmd = subprocess.Popen(args, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    out, err = cmd.communicate()
-    rc = cmd.returncode
-    return (rc, out, err)
-
 def get_version(dest):
     ''' samples the version of the git repo '''
     os.chdir(dest)
@@ -78,7 +72,7 @@ def get_version(dest):
     sha = sha[0].split()[1]
     return sha
 
-def clone(repo, dest, remote):
+def clone(module, repo, dest, remote):
     ''' makes a new git repo if it does not already exist '''
     dest_dirname = os.path.dirname(dest)
     try:
@@ -86,7 +80,8 @@ def clone(repo, dest, remote):
     except:
         pass
     os.chdir(dest_dirname)
-    return _run("git clone -o %s %s %s" % (remote, repo, dest))
+    return module.run_command("git clone -o %s %s %s" % (remote, repo, dest),
+                              fail_on_rc_non_zero=True)
 
 def has_local_mods(dest):
     os.chdir(dest)
@@ -104,12 +99,12 @@ def reset(module,dest,force):
     os.chdir(dest)
     if not force and has_local_mods(dest):
         module.fail_json(msg="Local modifications exist in repository (force=no).")
-    return _run("git reset --hard HEAD")
+    return module.run_command("git reset --hard HEAD", fail_on_rc_non_zero=True)
 
 def get_branches(module, dest):
     os.chdir(dest)
     branches = []
-    (rc, out, err) = _run("git branch -a")
+    (rc, out, err) = module.run_command("git branch -a")
     if rc != 0:
         module.fail_json(msg="Could not determine branch data - received %s" % out)
     for line in out.split('\n'):
@@ -185,11 +180,11 @@ def get_head_branch(module, dest, remote):
 def fetch(module, repo, dest, version, remote):
     ''' updates repo from remote sources '''
     os.chdir(dest)
-    (rc, out1, err1) = _run("git fetch %s" % remote)
+    (rc, out1, err1) = module.run_command("git fetch %s" % remote)
     if rc != 0:
         module.fail_json(msg="Failed to download remote objects and refs")
 
-    (rc, out2, err2) = _run("git fetch --tags %s" % remote)
+    (rc, out2, err2) = module.run_command("git fetch --tags %s" % remote)
     if rc != 0:
         module.fail_json(msg="Failed to download remote objects and refs")
     return (rc, out1 + out2, err1 + err2)
@@ -203,7 +198,7 @@ def switch_version(module, dest, remote, version):
             if not is_local_branch(module, dest, version):
                 cmd = "git checkout --track -b %s %s/%s" % (version, remote, version)
             else:
-                (rc, out, err) = _run("git checkout --force %s" % version)
+                (rc, out, err) = module.run_command("git checkout --force %s" % version)
                 if rc != 0:
                     module.fail_json(msg="Failed to checkout branch %s" % version)
                 cmd = "git reset --hard %s/%s" % (remote, version)
@@ -211,11 +206,11 @@ def switch_version(module, dest, remote, version):
             cmd = "git checkout --force %s" % version
     else:
         branch = get_head_branch(module, dest, remote)
-        (rc, out, err) = _run("git checkout --force %s" % branch)
+        (rc, out, err) = module.run_command("git checkout --force %s" % branch)
         if rc != 0:
             module.fail_json(msg="Failed to checkout branch %s" % branch)
         cmd = "git reset --hard %s" % remote
-    return _run(cmd)
+    return module.run_command(cmd, fail_on_rc_non_zero=True)
 
 # ===========================================
 
@@ -245,9 +240,7 @@ def main():
     before = None
     local_mods = False
     if not os.path.exists(gitconfig):
-        (rc, out, err) = clone(repo, dest, remote)
+        (rc, out, err) = clone(module, repo, dest, remote)
-        if rc != 0:
-            module.fail_json(msg=err)
     else:
         # else do a pull
         local_mods = has_local_mods(dest)
@@ -262,8 +255,6 @@ def main():
     # switch to version specified regardless of whether
     # we cloned or pulled
     (rc, out, err) = switch_version(module, dest, remote, version)
-    if rc != 0:
-        module.fail_json(msg=err)
 
     # determine if we changed anything
     after = get_version(dest)

library/group

@@ -91,10 +91,7 @@ class Group(object):
             syslog.openlog('ansible-%s' % os.path.basename(__file__))
             syslog.syslog(syslog.LOG_NOTICE, 'Command %s' % '|'.join(cmd))
 
-        p = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        return self.module.run_command(cmd)
-        (out, err) = p.communicate()
-        rc = p.returncode
-        return (rc, out, err)
 
     def group_del(self):
         cmd = [self.module.get_bin_path('groupdel', True), self.name]

library/mount

@@ -187,7 +187,7 @@ def unset_mount(**kwargs):
     return (args['name'], changed)
 
 
-def mount(**kwargs):
+def mount(module, **kwargs):
     """ mount up a path or remount if needed """
 
     name = kwargs['name']
@@ -196,25 +196,23 @@ def mount(**kwargs):
     else:
         cmd = [ '/bin/mount', name ]
 
-    call = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    rc, out, err = module.run_command(cmd)
-    out, err = call.communicate()
+    if rc == 0:
-    if call.returncode == 0:
         return 0, ''
     else:
-        return call.returncode, out+err
+        return rc, out+err
 
-def umount(**kwargs):
+def umount(module, **kwargs):
     """ unmount a path """
 
     name = kwargs['name']
     cmd = ['/bin/umount', name]
 
-    call = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    rc, out, err = module.run_command(cmd)
-    out, err = call.communicate()
+    if rc == 0:
-    if call.returncode == 0:
         return 0, ''
     else:
-        return call.returncode, out+err
+        return rc, out+err
 
 def main():
 
@@ -258,7 +256,7 @@ def main():
         name, changed = unset_mount(**args)
         if changed:
             if os.path.ismount(name):
-                res,msg  = umount(**args)
+                res,msg  = umount(module, **args)
                 if res:
                     module.fail_json(msg="Error unmounting %s: %s" % (name, msg))
 
@@ -272,7 +270,7 @@ def main():
 
     if state == 'unmounted':
         if os.path.ismount(name):
-            res,msg  = umount(**args)
+            res,msg  = umount(module, **args)
             if res:
                 module.fail_json(msg="Error unmounting %s: %s" % (name, msg))
             changed = True
@@ -291,10 +289,10 @@ def main():
             res = 0
             if os.path.ismount(name):
                 if changed:
-                    res,msg = mount(**args)
+                    res,msg = mount(module, **args)
             else:
                 changed = True
-                res,msg = mount(**args)
+                res,msg = mount(module, **args)
 
             if res:
                 module.fail_json(msg="Error mounting %s: %s" % (name, msg))

library/ohai

@@ -38,20 +38,12 @@ requirements: [ "ohai" ]
 author: Michael DeHaan
 '''
 
-def get_ohai_data():
-    p = subprocess.Popen(["/usr/bin/env", "ohai"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    (out, err) = p.communicate()
-    rc = p.returncode
-    return rc, out, err
-
 def main():
     module = AnsibleModule(
         argument_spec = dict()
     )
-    rc, out, err = get_ohai_data()
+    cmd = ["/usr/bin/env", "ohai"]
-    if rc != 0:
+    rc, out, err = module.run_command(cmd, fail_on_rc_non_zero=True)
-        module.fail_json(msg=err)
-    else:
     module.exit_json(**json.loads(out))
 
 # this is magic, see lib/ansible/module_common.py

library/pip

@@ -114,14 +114,6 @@ def _get_pip(module, env):
     return pip
 
 
-def _run(cmd):
-    # returns (rc, stdout, stderr) from shell command
-    process = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE, shell=True)
-    stdout, stderr = process.communicate()
-    return (process.returncode, stdout, stderr)
-
-
 def _fail(module, cmd, out, err):
     msg = ''
     if out:
@@ -173,7 +165,7 @@ def main():
         virtualenv = module.get_bin_path('virtualenv', True)
         if not os.path.exists(os.path.join(env, 'bin', 'activate')):
             cmd = '%s %s' % (virtualenv, env)
-            rc, out_venv, err_venv = _run(cmd)
+            rc, out_venv, err_venv = module.run_command(cmd)
             out += out_venv
             err += err_venv
             if rc != 0:
@@ -191,7 +183,7 @@ def main():
     elif requirements:
         cmd += ' -r %s' % requirements
 
-    rc, out_pip, err_pip = _run(cmd)
+    rc, out_pip, err_pip = module.run_command(cmd)
     out += out_pip
     err += err_pip
     if rc == 1 and state == 'absent' and 'not installed' in out_pip:

library/service

@@ -139,10 +139,7 @@ class Service(object):
             syslog.openlog('ansible-%s' % os.path.basename(__file__))
             syslog.syslog(syslog.LOG_NOTICE, 'Command %s' % '|'.join(cmd))
 
-        p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        return self.module.run_command(cmd)
-        (out, err) = p.communicate()
-        rc = p.returncode
-        return (rc, out, err)
 
     def check_ps(self):
         # Set ps flags

library/setup

@@ -178,10 +178,8 @@ class Facts(object):
         lsb_path = module.get_bin_path('lsb_release')
         if lsb_path is None:
             return self.facts
-        cmd = subprocess.Popen([lsb_path, "-a"], shell=False,
+        rc, out, err = module.run_command([lsb_path, "-a"])
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        if rc == 0:
-        out, err = cmd.communicate()
-        if cmd.returncode == 0:
             self.facts['lsb'] = {}
         for line in out.split('\n'):
             if len(line) < 1:
@@ -381,9 +379,7 @@ class SunOSHardware(Hardware):
         return self.facts
 
     def get_cpu_facts(self):
-        cmd = subprocess.Popen("/usr/sbin/psrinfo -v", shell=True,
+        rc, out, err = module.run_command("/usr/sbin/psrinfo -v")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         self.facts['processor'] = []
         for line in out.split('\n'):
             if 'processor operates' in line:
@@ -394,15 +390,11 @@ class SunOSHardware(Hardware):
         self.facts['processor_count'] = len(self.facts['processor'])
 
     def get_memory_facts(self):
-        cmd = subprocess.Popen("/usr/sbin/prtconf", shell=False,
+        rc, out, err = module.run_command(["/usr/sbin/prtconf"])
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         for line in out.split('\n'):
             if 'Memory size' in line:
                 self.facts['memtotal_mb'] = line.split()[2]
-        cmd = subprocess.Popen("/usr/sbin/swap -s", shell=True,
+        rc, out, err = module.run_command("/usr/sbin/swap -s")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         allocated = long(out.split()[1][:-1])
         reserved = long(out.split()[5][:-1])
         used = long(out.split()[8][:-1])
@@ -436,17 +428,14 @@ class FreeBSDHardware(Hardware):
 
     def get_cpu_facts(self):
         self.facts['processor'] = []
-        cmd = subprocess.Popen("/sbin/sysctl -n hw.ncpu", shell=True,
+        rc, out, err = module.run_command("/sbin/sysctl -n hw.ncpu")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         self.facts['processor_count'] = out.strip()
 
         try:
             dmesg_boot = open(FreeBSDHardware.DMESG_BOOT)
         except IOError:
-            dmesg_cmd = subprocess.Popen("/sbin/dmesg", shell=True,
+            rc, out, err = module.run_command("/sbin/dmesg")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            dmesg_boot = out
-            dmesg_boot = dmesg_cmd.stdout
 
         for line in dmesg_boot.readlines():
             if 'CPU:' in line:
@@ -457,9 +446,7 @@ class FreeBSDHardware(Hardware):
 
 
     def get_memory_facts(self):
-        cmd = subprocess.Popen("/sbin/sysctl vm.stats", shell=True,
+        rc, out, err = module.run_command("/sbin/sysctl vm.stats")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         for line in out.split('\n'):
             data = line.split()
             if 'vm.stats.vm.v_page_size' in line:
@@ -474,9 +461,7 @@ class FreeBSDHardware(Hardware):
         # Device          1M-blocks     Used    Avail Capacity
         # /dev/ada0p3        314368        0   314368     0%
         #
-        cmd = subprocess.Popen("/usr/sbin/swapinfo -m", shell=True,
+        rc, out, err = module.run_command("/usr/sbin/swapinfo -m")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         lines = out.split('\n')
         if len(lines[-1]) == 0:
             lines.pop()
@@ -557,12 +542,12 @@ class LinuxNetwork(Network):
         for v in 'v4', 'v6':
             if v == 'v6' and not socket.has_ipv6:
                 continue
-            output = subprocess.Popen(command[v], stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0]
+            rc, out, err = module.run_command(command[v])
-            if not output:
+            if not out:
                 # v6 routing may result in 
                 #   RTNETLINK answers: Invalid argument
                 continue
-            words = output.split('\n')[0].split()
+            words = out.split('\n')[0].split()
             # A valid output starts with the queried address on the first line
             if len(words) > 0 and words[0] == command[v][-1]:
                 for i in range(len(words) - 1):
@@ -580,8 +565,8 @@ class LinuxNetwork(Network):
             all_ipv4_addresses = [],
             all_ipv6_addresses = [],
         )
-        output = subprocess.Popen([ip_path, 'addr', 'show'], stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0]
+        rc, out, err = module.run_command([ip_path, 'addr', 'show'])
-        for line in output.split('\n'):
+        for line in out.split('\n'):
             if line:
                 words = line.split()
                 if not line.startswith(' '):
@@ -825,9 +810,7 @@ class SunOSVirtual(Virtual):
         return self.facts
 
     def get_virtual_facts(self):
-        cmd = subprocess.Popen("/usr/sbin/prtdiag", shell=True,
+        rc, out, err = module.run_command("/usr/sbin/prtdiag")
-                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         for line in out.split('\n'):
             if 'VMware' in line:
                 self.facts['virtualization_type'] = 'vmware'
@@ -843,9 +826,7 @@ class SunOSVirtual(Virtual):
                 self.facts['virtualization_role'] = 'guest'
         # Check if it's a zone
         if os.path.exists("/usr/bin/zonename"):
-            cmd = subprocess.Popen("/usr/bin/zonename", shell=True,
+            rc, out, err = module.run_command("/usr/bin/zonename")
-                stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            out, err = cmd.communicate()
             if out.rstrip() != "global":
                 self.facts['container'] = 'zone'
         # Check if it's a branded zone (i.e. Solaris 8/9 zone)
@@ -854,9 +835,7 @@ class SunOSVirtual(Virtual):
         # If it's a zone check if we can detect if our global zone is itself virtualized.
         # Relies on the "guest tools" (e.g. vmware tools) to be installed
         if 'container' in self.facts and self.facts['container'] == 'zone':
-            cmd = subprocess.Popen("/usr/sbin/modinfo", shell=True,
+            rc, out, err = module.run_command("/usr/sbin/modinfo")
-                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-            out, err = cmd.communicate()
             for line in out.split('\n'):
                 if 'VMware' in line:
                     self.facts['virtualization_type'] = 'vmware'
@@ -895,9 +874,7 @@ def run_setup(module):
     # ruby-json is ALSO installed, include facter data in the JSON
 
     if os.path.exists("/usr/bin/facter"):
-        cmd = subprocess.Popen("/usr/bin/facter --json", shell=True,
+        rc, out, err = module.run_command("/usr/bin/facter --json")
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         facter = True
         try:
             facter_ds = json.loads(out)
@@ -912,9 +889,7 @@ def run_setup(module):
     # templating w/o making a nicer key for it (TODO)
 
     if os.path.exists("/usr/bin/ohai"):
-        cmd = subprocess.Popen("/usr/bin/ohai", shell=True,
+        rc, out, err = module.run_command("/usr/bin/ohai")
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
         ohai = True
         try:
             ohai_ds = json.loads(out)

library/subversion

@@ -85,11 +85,7 @@ class Subversion(object):
         if self.password:
             bits.append("--password '%s'" % self.password)
         bits.append(args)
-        cmd = subprocess.Popen(' '.join(bits), shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        rc, out, err = self.module.run_command(' '.join(bits), fail_on_rc_non_zero=True)
-        out, err = cmd.communicate()
-        rc = cmd.returncode
-        if rc != 0:
-            self.module.fail_json(msg=err)
         return out.splitlines()
 
     def checkout(self):

library/supervisorctl

@@ -45,24 +45,6 @@ requirements: [ ]
 author: Matt Wright
 '''
 
-def _is_present(name, supervisorctl):
-    rc, out, err = _run('%s status' % supervisorctl)
-    return name in out
-
-
-def _is_running(name, supervisorctl):
-    rc, out, err = _run('%s status %s' % (supervisorctl, name))
-    return 'RUNNING' in out
-
-
-def _run(cmd):
-    # returns (rc, stdout, stderr) from shell command
-    process = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE, shell=True)
-    stdout, stderr = process.communicate()
-    return (process.returncode, stdout, stderr)
-
-
 def main():
     arg_spec = dict(
         name=dict(required=True),
@@ -76,12 +58,13 @@ def main():
 
     SUPERVISORCTL = module.get_bin_path('supervisorctl', True)
 
-    present = _is_present(name, SUPERVISORCTL)
+    rc, out, err = module.run_command('%s status' % supervisorctl)
+    present = name in out
 
     if state == 'present':
         if not present:
-            _run('%s reread' % SUPERVISORCTL)
+            module.run_command('%s reread' % SUPERVISORCTL, fail_on_rc_non_zero=True)
-            rc, out, err = _run('%s add %s' % (SUPERVISORCTL, name))
+            rc, out, err = module.run_command('%s add %s' % (SUPERVISORCTL, name))
 
             if '%s: added process group' % name in out:
                 module.exit_json(changed=True, name=name, state=state)
@@ -90,13 +73,14 @@ def main():
 
         module.exit_json(changed=False, name=name, state=state)
 
-    running = _is_running(name, SUPERVISORCTL)
+    rc, out, err = module.run_command('%s status %s' % (supervisorctl, name))
+    running = 'RUNNING' in out
 
     if running and state == 'started':
         module.exit_json(changed=False, name=name, state=state)
 
     if running and state == 'stopped':
-        rc, out, err = _run('%s stop %s' % (SUPERVISORCTL, name))
+        rc, out, err = module.run_command('%s stop %s' % (SUPERVISORCTL, name))
 
         if '%s: stopped' % name in out:
             module.exit_json(changed=True, name=name, state=state)
@@ -104,8 +88,8 @@ def main():
         module.fail_json(msg=out)
 
     elif running and state == 'restarted':
-        rc, out, err = _run('%s update %s' % (SUPERVISORCTL, name))
+        rc, out, err = module.run_command('%s update %s' % (SUPERVISORCTL, name))
-        rc, out, err = _run('%s restart %s' % (SUPERVISORCTL, name))
+        rc, out, err = module.run_command('%s restart %s' % (SUPERVISORCTL, name))
 
         if '%s: stopped' % name in out and '%s: started' % name in out:
             module.exit_json(changed=True, name=name, state=state)
@@ -113,7 +97,7 @@ def main():
         module.fail_json(msg=out)
 
     elif not running and state == 'started':
-        rc, out, err = _run('%s start %s' % (SUPERVISORCTL, name))
+        rc, out, err = module.run_command('%s start %s' % (SUPERVISORCTL, name))
 
         if '%s: started' % name in out:
             module.exit_json(changed=True, name=name, state=state)

library/svr4pkg

@@ -69,8 +69,8 @@ def package_installed(module, name):
     cmd = [module.get_bin_path('pkginfo', True)]
     cmd.append('-q')
     cmd.append(name)
-    res = subprocess.call(cmd)
+    rc, out, err = module.run_command(' '.join(cmd), shell=False)
-    if res == 0:
+    if rc == 0:
         return True
     else:
         return False
@@ -102,10 +102,7 @@ basedir=default
 def run_command(module, cmd):
     progname = cmd[0]
     cmd[0] = module.get_bin_path(progname, True)
-    p = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    return module.run_command(cmd)
-    (out, err) = p.communicate()
-    rc = p.returncode
-    return (rc, out, err)
 
 def package_install(module, name, src, proxy):
     adminfile = create_admin_file()

library/user

@@ -223,10 +223,7 @@ class User(object):
             syslog.openlog('ansible-%s' % os.path.basename(__file__))
             syslog.syslog(syslog.LOG_NOTICE, 'Command %s' % '|'.join(cmd))
 
-        p = subprocess.Popen(cmd, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        return self.module.run_command(cmd)
-        (out, err) = p.communicate()
-        rc = p.returncode
-        return (rc, out, err)
 
     def remove_user_userdel(self):
         cmd = [self.module.get_bin_path('userdel', True)]

library/yum

@@ -135,9 +135,9 @@ def is_installed(module, repoq, pkgspec, conf_file, qf=def_qf, en_repos=[], dis_
     else:
 
         cmd = repoq + ["--disablerepo=*", "--pkgnarrow=installed", "--qf", qf, pkgspec]
-        rc,out,err = run(cmd)
+        rc,out,err = module.run_command(cmd)
         cmd = repoq + ["--disablerepo=*", "--pkgnarrow=installed", "--qf", qf, "--whatprovides", pkgspec]
-        rc2,out2,err2 = run(cmd)
+        rc2,out2,err2 = module.run_command(cmd)
         if rc == 0 and rc2 == 0:
             out += out2
             return [ p for p in out.split('\n') if p.strip() ]
@@ -170,7 +170,7 @@ def is_available(module, repoq, pkgspec, conf_file, qf=def_qf, en_repos=[], dis_
     else:
 
         cmd = repoq + ["--qf", qf, pkgspec]
-        rc,out,err = run(cmd)
+        rc,out,err = module.run_command(cmd)
         if rc == 0:
             return [ p for p in out.split('\n') if p.strip() ]
         else:
@@ -211,7 +211,7 @@ def is_update(module, repoq, pkgspec, conf_file, qf=def_qf, en_repos=[], dis_rep
     else:
 
         cmd = repoq + ["--pkgnarrow=updates", "--qf", qf, pkgspec]
-        rc,out,err = run(cmd)
+        rc,out,err = module.run_command(cmd)
         
         if rc == 0:
             return set([ p for p in out.split('\n') if p.strip() ])
@@ -248,9 +248,9 @@ def what_provides(module, repoq, req_spec, conf_file,  qf=def_qf, en_repos=[], d
     else:
 
         cmd = repoq + ["--qf", qf, "--whatprovides", req_spec]
-        rc,out,err = run(cmd)
+        rc,out,err = module.run_command(cmd)
         cmd = repoq + ["--qf", qf, req_spec]
-        rc2,out2,err2 = run(cmd)
+        rc2,out2,err2 = module.run_command(cmd)
         if rc == 0 and rc2 == 0:
             out += out2
             pkgs = set([ p for p in out.split('\n') if p.strip() ])
@@ -267,7 +267,7 @@ def local_nvra(path):
     
     cmd = ['/bin/rpm', '-qp' ,'--qf', 
             '%{name}-%{version}-%{release}.%{arch}\n', path ]
-    rc, out, err = run(cmd)
+    rc, out, err = module.run_command(cmd)
     if rc != 0:
         return None
     nvra = out.split('\n')[0]
@@ -300,7 +300,7 @@ def pkg_to_dict(pkgstr):
 def repolist(repoq, qf="%{repoid}"):
 
     cmd = repoq + ["--qf", qf, "-a"]
-    rc,out,err = run(cmd)
+    rc,out,err = module.run_command(cmd)
     ret = []
     if rc == 0:
         ret = set([ p for p in out.split('\n') if p.strip() ])
@@ -324,30 +324,6 @@ def list_stuff(module, conf_file, stuff):
     else:
         return [ pkg_to_dict(p) for p in is_installed(module, repoq, stuff, conf_file, qf=qf) + is_available(module, repoq, stuff, conf_file, qf=qf) if p.strip() ]
 
-def run(command):
-
-    try:
-        cmd = subprocess.Popen(command,
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        out, err = cmd.communicate()
-    except (OSError, IOError), e:
-        rc = 1
-        err = str(e)
-        out = ''
-    except:
-        rc = 1
-        err = traceback.format_exc()
-        out = ''
-        if out is None:
-            out = ''
-        if err is None:
-            err = ''
-    else:
-        rc = cmd.returncode
-
-    return rc, out, err
-
-
 def install(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos):
 
     res = {}
@@ -414,7 +390,7 @@ def install(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos):
             pkg = spec
 
         cmd = yum_basecmd + ['install', pkg]
-        rc, out, err = run(cmd)
+        rc, out, err = module.run_command(cmd)
 
         res['rc'] += rc
         res['results'].append(out)
@@ -449,7 +425,7 @@ def remove(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos):
 
         # run an actual yum transaction
         cmd = yum_basecmd + ["remove", pkg]
-        rc, out, err = run(cmd)
+        rc, out, err = module.run_command(cmd)
 
         res['rc'] += rc
         res['results'].append(out)
@@ -518,7 +494,7 @@ def latest(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos):
             pkg = spec
 
         cmd = yum_basecmd + [basecmd, pkg]
-        rc, out, err = run(cmd)
+        rc, out, err = module.run_command(cmd)
 
         res['rc'] += rc
         res['results'].append(out)