remove shell=True

Signed-off-by: Mai Bui <maibui@microsoft.com>

config/main.py

@@ -870,9 +870,10 @@ def _stop_services():
 
 
 def _get_sonic_services():
-    cmd = "systemctl list-dependencies --plain sonic.target | sed '1d'"
-    out, _ = clicommon.run_command(cmd, return_cmd=True, shell=True)
-    return (unit.strip() for unit in out.splitlines())
+    cmd = ['systemctl', 'list-dependencies', '--plain', 'sonic.target']
+    out, _ = clicommon.run_command(cmd, return_cmd=True)
+    out = out.strip().split('\n')[1:]
+    return (unit.strip() for unit in out)
 
 def _reset_failed_services():
     for service in _get_sonic_services():
@@ -1654,8 +1655,19 @@ def load_mgmt_config(filename):
         command = ['ip'] + (["-6"] if mgmt_conf.version == 6 else []) + ['rule', 'add', 'from', str(mgmt_conf.ip), 'table', 'default']
         clicommon.run_command(command, display_cmd=True, ignore_error=True)
     if len(config_data['MGMT_INTERFACE'].keys()) > 0:
-        command = "[ -f /var/run/dhclient.eth0.pid ] && kill `cat /var/run/dhclient.eth0.pid` && rm -f /var/run/dhclient.eth0.pid"
-        clicommon.run_command(command, display_cmd=True, ignore_error=True, shell=True)
+        filepath = '/var/run/dhclient.eth0.pid'
+        if os.path.isfile(filepath):
+            out0, rc0 = clicommon.run_command(['cat', filepath], display_cmd=True, return_cmd=True)
+            if rc0 == 0:
+                out1, rc1 = clicommon.run_command(['kill', out0], return_cmd=True)
+                if rc1 == 0:
+                    clicommon.run_command(['rm', '-f', filepath], display_cmd=True, return_cmd=True)
+                else:
+                    sys.exit('Exit: {}. Command: kill {} failed.'.format(rc1, out0))
+            else:
+                sys.exit('Exit: {}. Command: cat {} failed.'.format(rc0, filepath))
+        else:
+            sys.exit('File {} does not exist'.format(filepath))
     click.echo("Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.")
 
 @config.command("load_minigraph")

tests/config_test.py

@@ -69,7 +69,8 @@
 Running command: ifconfig eth0 10.0.0.100 netmask 255.255.255.0
 Running command: ip route add default via 10.0.0.1 dev eth0 table default
 Running command: ip rule add from 10.0.0.100 table default
-Running command: [ -f /var/run/dhclient.eth0.pid ] && kill `cat /var/run/dhclient.eth0.pid` && rm -f /var/run/dhclient.eth0.pid
+Running command: kill `cat /var/run/dhclient.eth0.pid`
+Running command: rm -f /var/run/dhclient.eth0.pid
 Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.
 """
 
@@ -80,7 +81,8 @@
 Running command: ifconfig eth0 add fc00:1::32/64
 Running command: ip -6 route add default via fc00:1::1 dev eth0 table default
 Running command: ip -6 rule add from fc00:1::32 table default
-Running command: [ -f /var/run/dhclient.eth0.pid ] && kill `cat /var/run/dhclient.eth0.pid` && rm -f /var/run/dhclient.eth0.pid
+Running command: kill `cat /var/run/dhclient.eth0.pid`
+Running command: rm -f /var/run/dhclient.eth0.pid
 Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.
 """
 
@@ -94,7 +96,8 @@
 Running command: ifconfig eth0 add fc00:1::32/64
 Running command: ip -6 route add default via fc00:1::1 dev eth0 table default
 Running command: ip -6 rule add from fc00:1::32 table default
-Running command: [ -f /var/run/dhclient.eth0.pid ] && kill `cat /var/run/dhclient.eth0.pid` && rm -f /var/run/dhclient.eth0.pid
+Running command: kill `cat /var/run/dhclient.eth0.pid`
+Running command: rm -f /var/run/dhclient.eth0.pid
 Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.
 """
 
@@ -131,10 +134,6 @@
 Reloading Monit configuration ...
 """
 
-reload_config_with_untriggered_timer_output="""\
-Relevant services are not up. Retry later or use -f to avoid system checks
-"""
-
 def mock_run_command_side_effect(*args, **kwargs):
     command = args[0]
     if isinstance(command, str):
@@ -143,13 +142,15 @@ def mock_run_command_side_effect(*args, **kwargs):
         command = ' '.join(command)
 
     if kwargs.get('display_cmd'):
+        if 'cat /var/run/dhclient.eth0.pid' in command:
+            command = 'kill `cat /var/run/dhclient.eth0.pid`'
         click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))
 
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
             return 'snmp.timer', 0
-        elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss', 0
+        elif command == "systemctl list-dependencies --plain sonic.target":
+            return 'sonic.target\nswss', 0
         elif command == "systemctl is-enabled snmp.timer":
             return 'enabled', 0
         else:
@@ -168,8 +169,8 @@ def mock_run_command_side_effect_disabled_timer(*args, **kwargs):
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
             return 'snmp.timer', 0
-        elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss', 0
+        elif command == "systemctl list-dependencies --plain sonic.target":
+            return 'sonic.target\nswss', 0
         elif command == "systemctl is-enabled snmp.timer":
             return 'masked', 0
         elif command == "systemctl show swss.service --property ActiveState --value":
@@ -1563,7 +1564,7 @@ def test_config_load_mgmt_config_ipv4_only(self, get_cmd_module, setup_single_br
                 }
             }
         }
-        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_only_output, 5)
+        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_only_output, 7)
 
     def test_config_load_mgmt_config_ipv6_only(self, get_cmd_module, setup_single_broadcom_asic):
         device_desc_result = {
@@ -1578,7 +1579,7 @@ def test_config_load_mgmt_config_ipv6_only(self, get_cmd_module, setup_single_br
                 }
             }
         }
-        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv6_only_output, 5)
+        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv6_only_output, 7)
 
     def test_config_load_mgmt_config_ipv4_ipv6(self, get_cmd_module, setup_single_broadcom_asic):
         device_desc_result = {
@@ -1596,7 +1597,7 @@ def test_config_load_mgmt_config_ipv4_ipv6(self, get_cmd_module, setup_single_br
                 }
             }
         }
-        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_ipv6_output, 8)
+        self.check_output(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_ipv6_output, 10)
 
     def check_output(self, get_cmd_module, parse_device_desc_xml_result, expected_output, expected_command_call_count):
         def parse_device_desc_xml_side_effect(filename):
@@ -1605,20 +1606,21 @@ def parse_device_desc_xml_side_effect(filename):
         def change_hostname_side_effect(hostname):
             print("change hostname to {}".format(hostname))
         with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command:
-            with mock.patch('config.main.parse_device_desc_xml', mock.MagicMock(side_effect=parse_device_desc_xml_side_effect)):
-                with mock.patch('config.main._change_hostname', mock.MagicMock(side_effect=change_hostname_side_effect)):
-                    (config, show) = get_cmd_module
-                    runner = CliRunner()
-                    with runner.isolated_filesystem():
-                        with open('device_desc.xml', 'w') as f:
-                            f.write('dummy')
-                            result = runner.invoke(config.config.commands["load_mgmt_config"], ["-y", "device_desc.xml"])
-                            print(result.exit_code)
-                            print(result.output)
-                            traceback.print_tb(result.exc_info[2])
-                            assert result.exit_code == 0
-                            assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == expected_output
-                            assert mock_run_command.call_count == expected_command_call_count
+            with mock.patch('os.path.isfile', mock.MagicMock(return_value=True)):
+                with mock.patch('config.main.parse_device_desc_xml', mock.MagicMock(side_effect=parse_device_desc_xml_side_effect)):
+                    with mock.patch('config.main._change_hostname', mock.MagicMock(side_effect=change_hostname_side_effect)):
+                        (config, show) = get_cmd_module
+                        runner = CliRunner()
+                        with runner.isolated_filesystem():
+                            with open('device_desc.xml', 'w') as f:
+                                f.write('dummy')
+                                result = runner.invoke(config.config.commands["load_mgmt_config"], ["-y", "device_desc.xml"])
+                                print(result.exit_code)
+                                print(result.output)
+                                traceback.print_tb(result.exc_info[2])
+                                assert result.exit_code == 0
+                                assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == expected_output
+                                assert mock_run_command.call_count == expected_command_call_count
 
     @classmethod
     def teardown_class(cls):

utilities_common/cli.py

@@ -391,16 +391,12 @@ def print_output_in_alias_mode(output, index):
 
     click.echo(output.rstrip('\n'))
 
-def run_command_in_alias_mode(command, shell=False):
+def run_command_in_alias_mode(command):
     """Run command and replace all instances of SONiC interface names
        in output with vendor-sepecific interface aliases.
     """
-    if not shell:
-        command_str = ' '.join(command)
-    else:
-        command_str = command
-
-    process = subprocess.Popen(command, shell=shell, text=True, stdout=subprocess.PIPE)
+    command_str = ' '.join(command)
+    process = subprocess.Popen(command, text=True, stdout=subprocess.PIPE)
 
     while True:
         output = process.stdout.readline()
@@ -515,7 +511,7 @@ def run_command_in_alias_mode(command, shell=False):
         sys.exit(rc)
 
 
-def run_command(command, display_cmd=False, ignore_error=False, return_cmd=False, shell=False, interactive_mode=False):
+def run_command(command, display_cmd=False, ignore_error=False, return_cmd=False, interactive_mode=False):
     """
     Run bash command. Default behavior is to print output to stdout. If the command returns a non-zero
     return code, the function will exit with that return code.
@@ -528,11 +524,7 @@ def run_command(command, display_cmd=False, ignore_error=False, return_cmd=False
                           multiple lines of output over time
         shell: Boolean; If true, the command will be run in a shell
     """
-    if not shell:
-        command_str = ' '.join(command)
-    else:
-        command_str = command
-
+    command_str = ' '.join(command)
     if display_cmd == True:
         click.echo(click.style("Running command: ", fg='cyan') + click.style(command_str, fg='green'))
 
@@ -541,10 +533,10 @@ def run_command(command, display_cmd=False, ignore_error=False, return_cmd=False
     # IP route table cannot be handled in function run_command_in_alias_mode since it is in JSON format 
     # with a list for next hops 
     if get_interface_naming_mode() == "alias" and not command_str.startswith("intfutil") and not re.search("show ip|ipv6 route", command_str):
-        run_command_in_alias_mode(command, shell=shell)
+        run_command_in_alias_mode(command)
         sys.exit(0)
 
-    proc = subprocess.Popen(command, shell=shell, text=True, stdout=subprocess.PIPE)
+    proc = subprocess.Popen(command, text=True, stdout=subprocess.PIPE)
 
     if return_cmd:
         output = proc.communicate()[0]