Skip to content

Commit 28afb55

Browse files
maipbuimaipbui
authored
Revert "Remove subprocess with shell=True (#24)" (#33)
This reverts commit 409ecc3.
1 parent 409ecc3 commit 28afb55

22 files changed

+467
-554
lines changed

‎scripts/caclmgrd

100644100755
Lines changed: 117 additions & 125 deletions
Large diffs are not rendered by default.

‎scripts/hostcfgd

100644100755
Lines changed: 63 additions & 77 deletions
Large diffs are not rendered by default.

‎scripts/procdockerstatsd

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ from datetime import datetime
1313

1414
from sonic_py_common import daemon_base
1515
from swsscommon import swsscommon
16-
from sonic_py_common.general import getstatusoutput_noshell_pipe
1716

1817
VERSION = '1.0'
1918

@@ -30,7 +29,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
3029
self.state_db.connect("STATE_DB")
3130

3231
def run_command(self, cmd):
33-
proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
32+
proc = subprocess.Popen(cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE)
3433
(stdout, stderr) = proc.communicate()
3534
if proc.returncode != 0:
3635
self.log_error("Error running command '{}'".format(cmd))
@@ -119,7 +118,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
119118
return dockerdict
120119

121120
def update_dockerstats_command(self):
122-
cmd = ["docker", "stats", "--no-stream", "-a"]
121+
cmd = "docker stats --no-stream -a"
123122
data = self.run_command(cmd)
124123
if not data:
125124
self.log_error("'{}' returned null output".format(cmd))
@@ -136,13 +135,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
136135
return True
137136

138137
def update_processstats_command(self):
139-
cmd0 = ["ps", "-eo", "uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd", "--sort", "-%cpu"]
140-
cmd1 = ["head", "-1024"]
141-
cmd = ' | '.join([' '.join(cmd0), ' '.join(cmd1)])
142-
exitcode, data = getstatusoutput_noshell_pipe(cmd0, cmd1)
143-
if any(exitcode):
144-
self.log_error("Error running command '{}'".format(cmd))
145-
data = None
138+
data = self.run_command("ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024")
146139
processdata = self.format_process_cmd_output(data)
147140
value = ""
148141
# wipe out all data before updating with new values

‎tests/caclmgrd/caclmgrd_bfd_test.py

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,17 @@ def test_caclmgrd_bfd(self, test_name, test_data, fs):
3434

3535
MockConfigDb.set_config_db(test_data["config_db"])
3636

37-
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
38-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
39-
popen_mock = mock.Mock()
40-
popen_attrs = test_data["popen_attributes"]
41-
popen_mock.configure_mock(**popen_attrs)
42-
mocked_subprocess.Popen.return_value = popen_mock
43-
mocked_subprocess.PIPE = -1
44-
45-
call_rc = test_data["call_rc"]
46-
mocked_subprocess.call.return_value = call_rc
47-
48-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49-
caclmgrd_daemon.allow_bfd_protocol('')
50-
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
37+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38+
popen_mock = mock.Mock()
39+
popen_attrs = test_data["popen_attributes"]
40+
popen_mock.configure_mock(**popen_attrs)
41+
mocked_subprocess.Popen.return_value = popen_mock
42+
mocked_subprocess.PIPE = -1
43+
44+
call_rc = test_data["call_rc"]
45+
mocked_subprocess.call.return_value = call_rc
46+
47+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48+
caclmgrd_daemon.allow_bfd_protocol('')
49+
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
5150

‎tests/caclmgrd/caclmgrd_dhcp_test.py

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,21 @@ def test_caclmgrd_dhcp(self, test_name, test_data, fs):
3333

3434
MockConfigDb.set_config_db(test_data["config_db"])
3535

36-
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
37-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38-
popen_mock = mock.Mock()
39-
popen_attrs = test_data["popen_attributes"]
40-
popen_mock.configure_mock(**popen_attrs)
41-
mocked_subprocess.Popen.return_value = popen_mock
36+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
37+
popen_mock = mock.Mock()
38+
popen_attrs = test_data["popen_attributes"]
39+
popen_mock.configure_mock(**popen_attrs)
40+
mocked_subprocess.Popen.return_value = popen_mock
4241

43-
call_rc = test_data["call_rc"]
44-
mocked_subprocess.call.return_value = call_rc
42+
call_rc = test_data["call_rc"]
43+
mocked_subprocess.call.return_value = call_rc
4544

46-
mark = test_data["mark"]
45+
mark = test_data["mark"]
4746

48-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49-
mux_update = test_data["mux_update"]
47+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48+
mux_update = test_data["mux_update"]
5049

51-
for key,data in mux_update:
52-
caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
50+
for key,data in mux_update:
51+
caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
5352

54-
mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)
53+
mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)

‎tests/caclmgrd/caclmgrd_external_client_acl_test.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,8 @@ def test_caclmgrd_external_client_acl(self, test_name, test_data, fs):
4141
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
4242

4343
iptables_rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands('')
44-
test_data['return'] = [tuple(i) for i in test_data['return']]
45-
iptables_rules_ret = [tuple(i) for i in iptables_rules_ret]
4644
self.assertEqual(set(test_data["return"]).issubset(set(iptables_rules_ret)), True)
47-
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = ['ip', 'netns', 'exec', 'asic0']
45+
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = 'ip netns exec asic0'
4846
caclmgrd_daemon.namespace_docker_mgmt_ip['asic0'] = '1.1.1.1'
4947
caclmgrd_daemon.namespace_mgmt_ip = '2.2.2.2'
5048
caclmgrd_daemon.namespace_docker_mgmt_ipv6['asic0'] = 'fd::01'

‎tests/caclmgrd/caclmgrd_feature_test.py

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,17 @@ def test_feature_present(self, test_name, test_data, fs):
3434

3535
MockConfigDb.set_config_db(test_data["config_db"])
3636

37-
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
38-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
39-
popen_mock = mock.Mock()
40-
popen_attrs = test_data["popen_attributes"]
41-
popen_mock.configure_mock(**popen_attrs)
42-
mocked_subprocess.Popen.return_value = popen_mock
43-
mocked_subprocess.PIPE = -1
44-
45-
call_rc = test_data["call_rc"]
46-
mocked_subprocess.call.return_value = call_rc
47-
48-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49-
caclmgrd_daemon.update_feature_present()
50-
self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
51-
self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)
37+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38+
popen_mock = mock.Mock()
39+
popen_attrs = test_data["popen_attributes"]
40+
popen_mock.configure_mock(**popen_attrs)
41+
mocked_subprocess.Popen.return_value = popen_mock
42+
mocked_subprocess.PIPE = -1
43+
44+
call_rc = test_data["call_rc"]
45+
mocked_subprocess.call.return_value = call_rc
46+
47+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48+
caclmgrd_daemon.update_feature_present()
49+
self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
50+
self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)

‎tests/caclmgrd/caclmgrd_soc_rules_test.py

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,20 +36,19 @@ def test_caclmgrd_soc(self, test_name, test_data, fs):
3636

3737
MockConfigDb.set_config_db(test_data["config_db"])
3838

39-
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
40-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
41-
popen_mock = mock.Mock()
42-
popen_attrs = test_data["popen_attributes"]
43-
popen_mock.configure_mock(**popen_attrs)
44-
mocked_subprocess.Popen.return_value = popen_mock
45-
mocked_subprocess.PIPE = -1
46-
47-
call_rc = test_data["call_rc"]
48-
mocked_subprocess.call.return_value = call_rc
49-
50-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
51-
caclmgrd_daemon.update_control_plane_nat_acls('', {})
52-
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
39+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
40+
popen_mock = mock.Mock()
41+
popen_attrs = test_data["popen_attributes"]
42+
popen_mock.configure_mock(**popen_attrs)
43+
mocked_subprocess.Popen.return_value = popen_mock
44+
mocked_subprocess.PIPE = -1
45+
46+
call_rc = test_data["call_rc"]
47+
mocked_subprocess.call.return_value = call_rc
48+
49+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
50+
caclmgrd_daemon.update_control_plane_nat_acls('', {})
51+
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
5352

5453
def test_get_ip_from_interface_table(self):
5554
if not os.path.exists(DBCONFIG_PATH):

‎tests/caclmgrd/caclmgrd_test.py

Lines changed: 0 additions & 33 deletions
This file was deleted.

‎tests/caclmgrd/test_bfd_vectors.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@
2323
},
2424
},
2525
"expected_subprocess_calls": [
26-
call(['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE),
27-
call(['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE)
26+
call("iptables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE),
27+
call("ip6tables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE)
2828
],
2929
"popen_attributes": {
3030
'communicate.return_value': ('output', 'error'),

0 commit comments

Comments
 (0)