Merge pull request #329 from thomas-mangin/T2226

util: T2226: improvement and fixes

9 files changed, 145 insertions, 56 deletions

diff --git a/python/vyos/authutils.py b/python/vyos/authutils.py
index 90a46ffb4..66b5f4a74 100644
--- a/python/vyos/authutils.py
+++ b/python/vyos/authutils.py
@@ -22,7 +22,7 @@ def make_password_hash(password):
     """ Makes a password hash for /etc/shadow using mkpasswd """
 
     mkpassword = 'mkpasswd --method=sha-512 --stdin'
-    return cmd(mkpassword, input=password.encode(), timeout=5)
+    return cmd(mkpassword, input=password, timeout=5)
 
 def split_ssh_public_key(key_string, defaultname=""):
     """ Splits an SSH public key into its components """
diff --git a/python/vyos/remote.py b/python/vyos/remote.py
index f918461d1..1b4d3876e 100644
--- a/python/vyos/remote.py
+++ b/python/vyos/remote.py
@@ -18,7 +18,8 @@ import os
 import re
 import fileinput
 
-from vyos.util import cmd, DEVNULL
+from vyos.util import cmd
+from vyos.util import DEVNULL
 
 
 def check_and_add_host_key(host_name):
@@ -31,10 +32,10 @@ def check_and_add_host_key(host_name):
         mode = 0o600
         os.mknod(known_hosts, 0o600)
 
-    keyscan_cmd = 'ssh-keyscan -t rsa {} 2>/dev/null'.format(host_name)
+    keyscan_cmd = 'ssh-keyscan -t rsa {}'.format(host_name)
 
     try:
-        host_key = cmd(keyscan_cmd, shell=True, stderr=DEVNULL)
+        host_key = cmd(keyscan_cmd, stderr=DEVNULL)
     except OSError:
         sys.exit("Can not get RSA host key")
 
@@ -61,9 +62,9 @@ def check_and_add_host_key(host_name):
         print("Host key has changed!")
         print("If you trust the host key fingerprint below, continue.")
 
-    fingerprint_cmd = 'ssh-keygen -lf /dev/stdin <<< "{}"'.format(host_key)
+    fingerprint_cmd = 'ssh-keygen -lf /dev/stdin'
     try:
-        fingerprint = cmd(fingerprint_cmd, shell=True, stderr=DEVNULL)
+        fingerprint = cmd(fingerprint_cmd, stderr=DEVNULL, input=host_key)
     except OSError:
         sys.exit("Can not get RSA host key fingerprint.")
 
@@ -125,7 +126,7 @@ def get_remote_config(remote_file):
         # Try header first, and look for 'OK' or 'Moved' codes:
         curl_cmd = 'curl {0} -q -I {1}'.format(redirect_opt, remote_file)
         try:
-            curl_output = cmd(curl_cmd, shell=True)
+            curl_output = cmd(curl_cmd)
         except OSError:
             sys.exit(1)
 
@@ -142,6 +143,6 @@ def get_remote_config(remote_file):
         curl_cmd = 'curl {0} -# {1}'.format(redirect_opt, remote_file)
 
     try:
-        return cmd(curl_cmd, shell=True, stderr=None)
+        return cmd(curl_cmd, stderr=None)
     except OSError:
         return None
diff --git a/python/vyos/util.py b/python/vyos/util.py
index 291ce64ea..9ca229136 100644
--- a/python/vyos/util.py
+++ b/python/vyos/util.py
@@ -16,67 +16,137 @@
 import os
 import re
 import sys
-from subprocess import Popen, PIPE, STDOUT, DEVNULL
+from subprocess import Popen
+from subprocess import PIPE
+from subprocess import STDOUT
+from subprocess import DEVNULL
+
 
 def debug(flag):
-    # this is to force all new flags to be registered here so that
-    # they can be documented:
-    # - developer: the code will drop into PBD on un-handled exception
-    # - ifconfig: prints command and sysfs access on stdout for interface
+    """
+    Check is a debug flag was set by the user. 
+    a flag can be set by touching the file /tmp/vyos.flag.debug
+    with flag being the flag name, the current flags are:
+     - developer: the code will drop into PBD on un-handled exception
+     - ifconfig: prints command and sysfs access on stdout for interface
+    The function returns an empty string if the flag was not set,
+    """
+
+    # this is to force all new flags to be registered here to be documented:
     if flag not in ['developer', 'ifconfig']:
         return ''
     return flag if os.path.isfile(f'/tmp/vyos.{flag}.debug') else ''
 
 
-def debug_msg(message, section=''):
-    if debug(section):
-        print(f'DEBUG/{section:<6} {message}')
+def debug_msg(message, flag=''):
+    """
+    print a debug message line on stdout if debugging is enabled for the flag
+    """
+
+    if debug(flag):
+        print(f'DEBUG/{flag:<6} {message}')
+
 
+# There is many (too many) ways to run command with python
+# os.system, subprocess.Popen, subproces.{run,call,check_output}
+# which all have slighty different behaviour
 
-def popen(command, section='', shell=None, input=None, timeout=None, env=None,
-          universal_newlines=None, stdout=PIPE, stderr=STDOUT, decode=None):
-    """ popen does not raise, returns the output and error code of command """
+
+def popen(command, flag='', shell=None, input=None, timeout=None, env=None,
+          stdout=PIPE, stderr=None, decode=None):
+    """
+    popen is a wrapper helper aound subprocess.Popen
+    with it default setting it will return a tuple (out, err)
+    out: the output of the program run
+    err: the error code returned by the program
+
+    it can be affected by the following flags:
+    shell:   do not try to auto-detect if a shell is required
+             for example if a pipe (|) or redirection (>, >>) is used
+    input:   data to sent to the child process via STDIN
+             the data should be bytes but string will be converted
+    timeout: time after which the command will be considered to have failed
+    env:     mapping that defines the environment variables for the new process
+    stdout:  define how the output of the program should be handled
+              - PIPE (default), sends stdout to the output
+              - DEVNULL, discard the output
+    stderr:  define how the output of the program should be handled
+              - None (default), send/merge the data to/with stderr
+              - PIPE, popen will append it to output
+              - STDOUT, send the data to be merged with stdout
+              - DEVNULL, discard the output
+    decode:  specify the expected text encoding (utf-8, ascii, ...)
+
+    usage:
+    to get both stdout, and stderr: popen('command', stdout=PIPE, stderr=STDOUT)
+    to discard stdout and get stderr: popen('command', stdout=DEVNUL, stderr=PIPE)
+    """
+    debug_msg(f"cmd '{command}'", flag)
     use_shell = shell
+    stdin = None
     if shell is None:
-        use_shell = True if ' ' in command else False
+        use_shell = False
+        if ' ' in command:
+            use_shell = True
+        if env:
+            use_shell = True
+    if input:
+        stdin = PIPE
+        input = input.encode() if type(input) is str else input
     p = Popen(
         command,
-        stdout=stdout, stderr=stderr,
+        stdin=stdin, stdout=stdout, stderr=stderr,
         env=env, shell=use_shell,
-        universal_newlines=universal_newlines,
     )
-    tmp = p.communicate(input, timeout)[0].strip()
-    debug_msg(f"cmd '{command}'", section)
-    decoded = tmp.decode(decode) if decode else tmp.decode()
+    tmp = p.communicate(input, timeout)
+    out1 = b''
+    out2 = b''
+    if stdout == PIPE:
+        out1 = tmp[0]
+    if stderr == PIPE:
+        out2 += tmp[1]
+    decoded1 = out1.decode(decode) if decode else out1.decode()
+    decoded2 = out2.decode(decode) if decode else out2.decode()
+    decoded1 = decoded1.replace('\r\n', '\n').strip()
+    decoded2 = decoded2.replace('\r\n', '\n').strip()
+    nl = '\n' if decoded1 and decoded2 else ''
+    decoded = decoded1 + nl + decoded2
     if decoded:
-        debug_msg(f"returned:\n{decoded}", section)
+        debug_msg(f"returned:\n{decoded}", flag)
     return decoded, p.returncode
 
 
-def run(command, section='', shell=None, input=None, timeout=None, env=None,
-        universal_newlines=None, stdout=PIPE, stderr=STDOUT, decode=None):
-    """ does not raise exception on error, returns error code """
+def run(command, flag='', shell=None, input=None, timeout=None, env=None,
+        stdout=DEVNULL, stderr=None, decode=None):
+    """
+    A wrapper around vyos.util.popen, which discard the stdout and
+    will return the error code of a command
+    """
     _, code = popen(
-        command, section,
+        command, flag,
         stdout=stdout, stderr=stderr,
         input=input, timeout=timeout,
         env=env, shell=shell,
-        universal_newlines=universal_newlines,
         decode=decode,
     )
     return code
 
 
-def cmd(command, section='', shell=None, input=None, timeout=None, env=None,
-        universal_newlines=None, stdout=PIPE, stderr=STDOUT, decode=None,
+def cmd(command, flag='', shell=None, input=None, timeout=None, env=None,
+        stdout=PIPE, stderr=None, decode=None,
         raising=None, message=''):
-    """ does raise exception, returns output of command """
+    """
+    A wrapper around vyos.util.popen, which returns the stdout and
+    will raise the error code of a command
+
+    raising: specify which call should be used when raising (default is OSError)
+             the class should only require a string as parameter
+    """
     decoded, code = popen(
-        command, section,
+        command, flag,
         stdout=stdout, stderr=stderr,
         input=input, timeout=timeout,
         env=env, shell=shell,
-        universal_newlines=universal_newlines,
         decode=decode,
     )
     if code != 0:
@@ -92,15 +162,17 @@ def cmd(command, section='', shell=None, input=None, timeout=None, env=None,
     return decoded
 
 
-def call(command, section='', shell=None, input=None, timeout=None, env=None,
-        universal_newlines=None, stdout=PIPE, stderr=STDOUT, decode=None):
-    """ does not raise exception on error, returns error code, print output """
+def call(command, flag='', shell=None, input=None, timeout=None, env=None,
+         stdout=PIPE, stderr=None, decode=None):
+    """
+    A wrapper around vyos.util.popen, which print the stdout and
+    will return the error code of a command
+    """
     out, code = popen(
-        command, section,
+        command, flag,
         stdout=stdout, stderr=stderr,
         input=input, timeout=timeout,
         env=env, shell=shell,
-        universal_newlines=universal_newlines,
         decode=decode,
     )
     print(out)
diff --git a/src/conf_mode/flow_accounting_conf.py b/src/conf_mode/flow_accounting_conf.py
index eef32687e..54928cdfe 100755
--- a/src/conf_mode/flow_accounting_conf.py
+++ b/src/conf_mode/flow_accounting_conf.py
@@ -83,7 +83,7 @@ def _iptables_get_nflog():
     for iptables_variant in ['iptables', 'ip6tables']:
         # run iptables, save output and split it by lines
         iptables_command = "sudo {0} -t {1} -S {2}".format(iptables_variant, iptables_nflog_table, iptables_nflog_chain)
-        cmd(iptables_command, universal_newlines=True, message='Failed to get flows list')
+        cmd(iptables_command, message='Failed to get flows list')
         iptables_out = stdout.splitlines()
 
         # parse each line and add information to list
diff --git a/src/conf_mode/system-login.py b/src/conf_mode/system-login.py
index 43732cfae..7e854c9c9 100755
--- a/src/conf_mode/system-login.py
+++ b/src/conf_mode/system-login.py
@@ -28,6 +28,8 @@ from vyos.defaults import directories as vyos_data_dir
 from vyos import ConfigError
 from vyos.util import cmd
 from vyos.util import call
+from vyos.util import DEVNULL
+
 
 radius_config_file = "/etc/pam_radius_auth.conf"
 
@@ -211,6 +213,14 @@ def generate(login):
             os.system("vyos_libexec_dir=/usr/libexec/vyos /opt/vyatta/sbin/my_set system login user '{}' authentication plaintext-password '' >/dev/null".format(user['name']))
             os.system("vyos_libexec_dir=/usr/libexec/vyos /opt/vyatta/sbin/my_set system login user '{}' authentication encrypted-password '{}' >/dev/null".format(user['name'], user['password_encrypted']))
 
+            # env = os.environ.copy()
+            # env['vyos_libexec_dir'] = '/usr/libexec/vyos'
+
+            # call("/opt/vyatta/sbin/my_set system login user '{}' authentication plaintext-password ''".format(user['name']),
+            #     env=env)
+            # call("/opt/vyatta/sbin/my_set system login user '{}' authentication encrypted-password '{}'".format(user['name'], user['password_encrypted']),
+            #     env=env)
+
     if len(login['radius_server']) > 0:
         # Prepare Jinja2 template loader from files
         tmpl_path = os.path.join(vyos_data_dir['data'], 'templates', 'system-login')
@@ -256,7 +266,7 @@ def apply(login):
         command += " {}".format(user['name'])
 
         try:
-            call(command)
+            cmd(command)
 
             uid = getpwnam(user['name']).pw_uid
             gid = getpwnam(user['name']).pw_gid
@@ -299,7 +309,7 @@ def apply(login):
                 call('pkill -HUP -u {}'.format(user))
 
             # Remove user account but leave home directory to be safe
-            call('userdel -r {} 2>/dev/null'.format(user))
+            call(f'userdel -r {user}', stderr=DEVNULL)
 
         except Exception as e:
             raise ConfigError('Deleting user "{}" raised an exception: {}'.format(user, e))
@@ -309,8 +319,10 @@ def apply(login):
     #
     if len(login['radius_server']) > 0:
         try:
+            env = os.environ.copy()
+            env['DEBIAN_FRONTEND'] = 'noninteractive'
             # Enable RADIUS in PAM
-            os.system("DEBIAN_FRONTEND=noninteractive pam-auth-update --package --enable radius")
+            cmd("pam-auth-update --package --enable radius", env=env)
 
             # Make NSS system aware of RADIUS, too
             command = "sed -i -e \'/\smapname/b\' \
@@ -321,15 +333,18 @@ def apply(login):
                           -e \'/^group:[^#]*$/s/: */&mapname /\' \
                           /etc/nsswitch.conf"
 
-            call(command)
+            cmd(command)
 
         except Exception as e:
             raise ConfigError('RADIUS configuration failed: {}'.format(e))
 
     else:
         try:
+            env = os.environ.copy()
+            env['DEBIAN_FRONTEND'] = 'noninteractive'
+
             # Disable RADIUS in PAM
-            os.system("DEBIAN_FRONTEND=noninteractive pam-auth-update --package --remove radius")
+            cmd("pam-auth-update --package --remove radius", env=env)
 
             command = "sed -i -e \'/^passwd:.*mapuid[ \t]/s/mapuid[ \t]//\' \
                    -e \'/^passwd:.*[ \t]mapname/s/[ \t]mapname//\' \
@@ -337,10 +352,10 @@ def apply(login):
                    -e \'s/[ \t]*$//\' \
                    /etc/nsswitch.conf"
 
-            call(command)
+            cmd(command)
 
         except Exception as e:
-            raise ConfigError('Removing RADIUS configuration failed'.format(e))
+            raise ConfigError('Removing RADIUS configuration failed.\n{}'.format(e))
 
     return None
 
diff --git a/src/op_mode/flow_accounting_op.py b/src/op_mode/flow_accounting_op.py
index 7f3ad7476..71fdfc288 100755
--- a/src/op_mode/flow_accounting_op.py
+++ b/src/op_mode/flow_accounting_op.py
@@ -76,7 +76,7 @@ def _uacctd_running():
 # get list of interfaces
 def _get_ifaces_dict():
     # run command to get ifaces list
-    out = cmd('/bin/ip link show', universal_newlines=True)
+    out = cmd('/bin/ip link show')
 
     # read output
     ifaces_out = out.splitlines()
@@ -95,7 +95,6 @@ def _get_ifaces_dict():
 def _get_flows_list():
     # run command to get flows list
     out = cmd(f'/usr/bin/pmacct -s -O json -T flows -p {uacctd_pipefile}',
-                universal_newlines=True,
                 message='Failed to get flows list')
 
     # read output
diff --git a/src/op_mode/powerctrl.py b/src/op_mode/powerctrl.py
index 0f3619411..4ab91384b 100755
--- a/src/op_mode/powerctrl.py
+++ b/src/op_mode/powerctrl.py
@@ -24,6 +24,7 @@ from vyos.util import ask_yes_no
 from vyos.util import cmd
 from vyos.util import call
 from vyos.util import run
+from vyos.util import STDOUT
 
 systemd_sched_file = "/run/systemd/shutdown/scheduled"
 
@@ -97,14 +98,14 @@ def execute_shutdown(time, reboot = True, ask=True):
     chk_vyatta_based_reboots()
     ###
 
-    out = cmd(f'/sbin/shutdown {action} now')
+    out = cmd(f'/sbin/shutdown {action} now', stderr=STDOUT)
     print(out.split(",",1)[0])
     return
   elif len(time) == 1:
     # Assume the argument is just time
     ts = parse_time(time[0])
     if ts:
-      cmd(f'/sbin/shutdown {action} {time[0]}')
+      cmd(f'/sbin/shutdown {action} {time[0]}', stderr=STDOUT)
     else:
       sys.exit("Invalid time \"{0}\". The valid format is HH:MM".format(time[0]))
   elif len(time) == 2:
@@ -115,7 +116,7 @@ def execute_shutdown(time, reboot = True, ask=True):
       t = datetime.combine(ds, ts)
       td = t - datetime.now()
       t2 = 1 + int(td.total_seconds())//60 # Get total minutes
-      cmd('/sbin/shutdown {action} {t2}')
+      cmd('/sbin/shutdown {action} {t2}', stderr=STDOUT)
     else:
       if not ts:
         sys.exit("Invalid time \"{0}\". The valid format is HH:MM".format(time[0]))
diff --git a/src/op_mode/version.py b/src/op_mode/version.py
index fe6ecbae5..8599c958f 100755
--- a/src/op_mode/version.py
+++ b/src/op_mode/version.py
@@ -33,6 +33,7 @@ import vyos.limericks
 from vyos.util import cmd
 from vyos.util import call
 from vyos.util import run
+from vyos.util import DEVNULL
 
 
 parser = argparse.ArgumentParser()
@@ -82,7 +83,7 @@ if __name__ == '__main__':
     # Get hypervisor name, if any
     system_type = "bare metal"
     try:
-        hypervisor = cmd('hvinfo 2>/dev/null')
+        hypervisor = cmd('hvinfo',stderr=DEVNULL)
         system_type = "{0} guest".format(hypervisor)
     except OSError:
         # hvinfo returns 1 if it cannot detect any hypervisor
diff --git a/src/system/keepalived-fifo.py b/src/system/keepalived-fifo.py
index 2778deaab..7e2076820 100755
--- a/src/system/keepalived-fifo.py
+++ b/src/system/keepalived-fifo.py
@@ -87,7 +87,7 @@ class KeepalivedFifo:
     def _run_command(self, command):
         logger.debug("Running the command: {}".format(command))
         try:
-            cmd(command, universal_newlines=True)
+            cmd(command)
         except OSError as err:
             logger.error(f'Unable to execute command "{command}": {err}')