From 0b2d9ff51a58c6f1007e4dd4f41fffe46b678927 Mon Sep 17 00:00:00 2001 From: maipbui Date: Mon, 29 Aug 2022 17:09:22 +0000 Subject: [PATCH 01/10] Mitigate security vulnerabilities in subprocess Signed-off-by: maipbui --- .../x86_64-mlnx_msn2700-r0/plugins/fanutil.py | 3 ++- .../x86_64-mlnx_msn2700-r0/plugins/psuutil.py | 3 ++- .../x86_64-mlnx_msn2700-r0/plugins/sfputil.py | 13 +++++++------ .../x86_64-mlnx_msn2700-r0/plugins/thermalutil.py | 3 ++- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py index 6d54455af408..4306d5551340 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py @@ -28,6 +28,7 @@ import syslog import subprocess from glob import glob + from shlex import split from sonic_fan.fan_base import FanBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -72,7 +73,7 @@ def __init__(self): self.num_of_fan, self.num_of_drawer = self._extract_num_of_fans_and_fan_drawers() def _get_sku_name(self): - p = subprocess.Popen(self.GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py index 915c90bf25a6..1bb0be0db3fd 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py @@ -26,6 +26,7 @@ import os.path import syslog import subprocess + from shlex import split from sonic_psu.psu_base import PsuBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -65,7 +66,7 @@ def __init__(self): self.fan_speed = "thermal/psu{}_fan1_speed_get" def _get_sku_name(self): - p = subprocess.Popen(self.GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py index 9423de8dd75b..2931d9626b60 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py @@ -21,6 +21,7 @@ try: import subprocess + from shlex import split from sonic_sfp.sfputilbase import * import syslog except ImportError as e: @@ -110,7 +111,7 @@ def port_to_eeprom_mapping(self): raise Exception() def get_port_position_tuple_by_platform_name(self): - p = subprocess.Popen(GET_PLATFORM_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(split(GET_PLATFORM_CMD), universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() position_tuple = port_position_tuple_list[platform_dict[out.rstrip('\n')]] return position_tuple @@ -138,7 +139,7 @@ def get_presence(self, port_num): ethtool_cmd = "ethtool -m {} 2>/dev/null".format(sfpname) try: - proc = subprocess.Popen(ethtool_cmd, stdout=subprocess.PIPE, shell=True, universal_newlines=True, stderr=subprocess.STDOUT) + proc = subprocess.Popen(split(ethtool_cmd), stdout=subprocess.PIPE, universal_newlines=True, stderr=subprocess.STDOUT) stdout = proc.communicate()[0] proc.wait() result = stdout.rstrip('\n') @@ -158,7 +159,7 @@ def get_low_power_mode(self, port_num): lpm_cmd = "docker exec syncd python /usr/share/sonic/platform/plugins/sfplpmget.py {}".format(port_num) try: - output = subprocess.check_output(lpm_cmd, shell=True, universal_newlines=True) + output = subprocess.run(split(lpm_cmd), universal_newlines=True, capture_output=True, check=True).stdout if 'LPM ON' in output: return True except subprocess.CalledProcessError as e: @@ -182,7 +183,7 @@ def set_low_power_mode(self, port_num, lpmode): # Set LPM try: - subprocess.check_output(lpm_cmd, shell=True, universal_newlines=True) + subprocess.run(split(lpm_cmd), check=True, universal_newlines=True, capture_output=True) except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) return False @@ -197,7 +198,7 @@ def reset(self, port_num): lpm_cmd = "docker exec syncd python /usr/share/sonic/platform/plugins/sfpreset.py {}".format(port_num) try: - subprocess.check_output(lpm_cmd, shell=True, universal_newlines=True) + subprocess.run(split(lpm_cmd), check=True, universal_newlines=True, capture_output=True) return True except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) @@ -269,7 +270,7 @@ def _read_eeprom_specific_bytes_via_ethtool(self, port_num, offset, num_bytes): eeprom_raw = [] ethtool_cmd = "ethtool -m {} hex on offset {} length {}".format(sfpname, offset, num_bytes) try: - output = subprocess.check_output(ethtool_cmd, shell=True, universal_newlines=True) + output = subprocess.run(split(ethtool_cmd), check=True, universal_newlines=True, capture_output=True) output_lines = output.splitlines() first_line_raw = output_lines[0] if "Offset" in first_line_raw: diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py index 8ee172feacdd..160dd925ab28 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py @@ -26,6 +26,7 @@ from os.path import join import syslog import subprocess + from shlex import split from sonic_thermal.thermal_base import ThermalBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -380,7 +381,7 @@ class ThermalUtil(ThermalBase): thermal_list = [] def _get_sku_name(self): - p = subprocess.Popen(self.GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') From 3953ca2f9672a64821c698c70c492827df612af4 Mon Sep 17 00:00:00 2001 From: maipbui Date: Tue, 30 Aug 2022 20:52:36 +0000 Subject: [PATCH 02/10] Convert command string to command array Signed-off-by: maipbui --- .../x86_64-mlnx_msn2700-r0/plugins/fanutil.py | 5 ++-- .../x86_64-mlnx_msn2700-r0/plugins/psuutil.py | 5 ++-- .../x86_64-mlnx_msn2700-r0/plugins/sfputil.py | 25 +++++++++---------- .../plugins/thermalutil.py | 5 ++-- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py index 4306d5551340..710584352bd5 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/fanutil.py @@ -28,7 +28,6 @@ import syslog import subprocess from glob import glob - from shlex import split from sonic_fan.fan_base import FanBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -45,7 +44,7 @@ class FanUtil(FanBase): PWM_MAX = 255 MAX_FAN_PER_DRAWER = 2 - GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" + GET_HWSKU_CMD = ["sonic-cfggen", "-d", "-v", "DEVICE_METADATA.localhost.hwsku"] sku_without_fan_direction = ['ACS-MSN2010', 'ACS-MSN2100', 'ACS-MSN2410', 'ACS-MSN2700', 'Mellanox-SN2700', 'Mellanox-SN2700-D48C8', 'LS-SN2700', 'ACS-MSN2740'] sku_with_unpluggable_fan = ['ACS-MSN2010', 'ACS-MSN2100'] @@ -73,7 +72,7 @@ def __init__(self): self.num_of_fan, self.num_of_drawer = self._extract_num_of_fans_and_fan_drawers() def _get_sku_name(self): - p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(self.GET_HWSKU_CMD, universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py index 1bb0be0db3fd..67cc92a4146a 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/psuutil.py @@ -26,7 +26,6 @@ import os.path import syslog import subprocess - from shlex import split from sonic_psu.psu_base import PsuBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -43,7 +42,7 @@ class PsuUtil(PsuBase): MAX_PSU_FAN = 1 MAX_NUM_PSU = 2 - GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" + GET_HWSKU_CMD = ["sonic-cfggen", "-d", "-v", "DEVICE_METADATA.localhost.hwsku"] # for spectrum1 switches with plugable PSUs, the output voltage file is psuX_volt # for spectrum2 switches the output voltage file is psuX_volt_out2 sku_spectrum1_with_plugable_psu = ['ACS-MSN2410', 'ACS-MSN2700', @@ -66,7 +65,7 @@ def __init__(self): self.fan_speed = "thermal/psu{}_fan1_speed_get" def _get_sku_name(self): - p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(self.GET_HWSKU_CMD, universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py index 2931d9626b60..01dc2baa74d7 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py @@ -21,7 +21,6 @@ try: import subprocess - from shlex import split from sonic_sfp.sfputilbase import * import syslog except ImportError as e: @@ -49,7 +48,7 @@ SYSTEM_READY = 'system_become_ready' SYSTEM_FAIL = 'system_fail' -GET_PLATFORM_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.platform" +GET_PLATFORM_CMD = ["sonic-cfggen", "-d", "-v", "DEVICE_METADATA.localhost.platform"] # Ethernet <=> sfp SFP_PORT_NAME_OFFSET = 0 @@ -111,7 +110,7 @@ def port_to_eeprom_mapping(self): raise Exception() def get_port_position_tuple_by_platform_name(self): - p = subprocess.Popen(split(GET_PLATFORM_CMD), universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(GET_PLATFORM_CMD, universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() position_tuple = port_position_tuple_list[platform_dict[out.rstrip('\n')]] return position_tuple @@ -137,9 +136,9 @@ def get_presence(self, port_num): port_num += SFP_PORT_NAME_OFFSET sfpname = SFP_PORT_NAME_CONVENTION.format(port_num) - ethtool_cmd = "ethtool -m {} 2>/dev/null".format(sfpname) + ethtool_cmd = ["ethtool", "-m", sfpname] try: - proc = subprocess.Popen(split(ethtool_cmd), stdout=subprocess.PIPE, universal_newlines=True, stderr=subprocess.STDOUT) + proc = subprocess.Popen(ethtool_cmd, stdout=subprocess.PIPE, universal_newlines=True, stderr=subprocess.DEVNULL) stdout = proc.communicate()[0] proc.wait() result = stdout.rstrip('\n') @@ -156,10 +155,10 @@ def get_low_power_mode(self, port_num): if port_num < self.port_start or port_num > self.port_end: return False - lpm_cmd = "docker exec syncd python /usr/share/sonic/platform/plugins/sfplpmget.py {}".format(port_num) + lpm_cmd = ["docker", "exec", "syncd", "python", "/usr/share/sonic/platform/plugins/sfplpmget.py", str(port_num)] try: - output = subprocess.run(split(lpm_cmd), universal_newlines=True, capture_output=True, check=True).stdout + output = subprocess.run(lpm_cmd, universal_newlines=True, capture_output=True, check=True).stdout if 'LPM ON' in output: return True except subprocess.CalledProcessError as e: @@ -179,11 +178,11 @@ def set_low_power_mode(self, port_num, lpmode): # Compose LPM command lpm = 'on' if lpmode else 'off' - lpm_cmd = "docker exec syncd python /usr/share/sonic/platform/plugins/sfplpmset.py {} {}".format(port_num, lpm) + lpm_cmd = ["docker", "exec", "syncd", "python", "/usr/share/sonic/platform/plugins/sfplpmset.py", str(port_num), lpm] # Set LPM try: - subprocess.run(split(lpm_cmd), check=True, universal_newlines=True, capture_output=True) + subprocess.run(lpm_cmd, check=True, universal_newlines=True, capture_output=True).stdout except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) return False @@ -195,10 +194,10 @@ def reset(self, port_num): if port_num < self.port_start or port_num > self.port_end: return False - lpm_cmd = "docker exec syncd python /usr/share/sonic/platform/plugins/sfpreset.py {}".format(port_num) + lpm_cmd = ["docker", "exec", "syncd", "python", "/usr/share/sonic/platform/plugins/sfpreset.py", str(port_num)] try: - subprocess.run(split(lpm_cmd), check=True, universal_newlines=True, capture_output=True) + subprocess.run(lpm_cmd, check=True, universal_newlines=True, capture_output=True).stdout return True except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) @@ -268,9 +267,9 @@ def _read_eeprom_specific_bytes_via_ethtool(self, port_num, offset, num_bytes): sfpname = SFP_PORT_NAME_CONVENTION.format(port_num) eeprom_raw = [] - ethtool_cmd = "ethtool -m {} hex on offset {} length {}".format(sfpname, offset, num_bytes) + ethtool_cmd = ["ethtool", "-m", sfpname, "hex", "on", "offset", str(offset), "length", str(num_bytes)] try: - output = subprocess.run(split(ethtool_cmd), check=True, universal_newlines=True, capture_output=True) + output = subprocess.run(ethtool_cmd, check=True, universal_newlines=True, capture_output=True).stdout output_lines = output.splitlines() first_line_raw = output_lines[0] if "Offset" in first_line_raw: diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py index 160dd925ab28..ea7e5f702147 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/thermalutil.py @@ -26,7 +26,6 @@ from os.path import join import syslog import subprocess - from shlex import split from sonic_thermal.thermal_base import ThermalBase except ImportError as e: raise ImportError(str(e) + "- required module not found") @@ -376,12 +375,12 @@ class ThermalUtil(ThermalBase): MAX_PSU_FAN = 1 MAX_NUM_PSU = 2 - GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" + GET_HWSKU_CMD = ["sonic-cfggen", "-d", "-v", "DEVICE_METADATA.localhost.hwsku"] number_of_thermals = 0 thermal_list = [] def _get_sku_name(self): - p = subprocess.Popen(split(self.GET_HWSKU_CMD), universal_newlines=True, stdout=subprocess.PIPE) + p = subprocess.Popen(self.GET_HWSKU_CMD, universal_newlines=True, stdout=subprocess.PIPE) out, err = p.communicate() return out.rstrip('\n') From d273d60aa582dd53163efaafd332ddf7eded871f Mon Sep 17 00:00:00 2001 From: maipbui Date: Thu, 15 Sep 2022 18:47:41 +0000 Subject: [PATCH 03/10] Replace shell=True and os.system in mlnx-platform-api Signed-off-by: maipbui --- .../sonic_platform/component.py | 141 ++++++++---------- .../mlnx-platform-api/sonic_platform/psu.py | 2 +- .../mlnx-platform-api/sonic_platform/sfp.py | 28 ++-- .../mlnx-platform-api/sonic_platform/utils.py | 5 +- .../mlnx-platform-api/tests/test_chassis.py | 8 +- .../mlnx-platform-api/tests/test_fan_api.py | 3 +- .../mlnx-platform-api/tests/test_utils.py | 2 +- 7 files changed, 89 insertions(+), 100 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py index 6f482f497194..f75fcfa6e323 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py @@ -52,8 +52,8 @@ class MPFAManager(object): MPFA_EXTENSION = '.mpfa' - MPFA_EXTRACT_COMMAND = 'tar xzf {} -C {}' - MPFA_CLEANUP_COMMAND = 'rm -rf {}' + MPFA_EXTRACT_COMMAND = ['tar', 'xzf', '', '-C', ''] + MPFA_CLEANUP_COMMAND = ['rm', '-rf', ''] def __init__(self, mpfa_path): self.__mpfa_path = mpfa_path @@ -78,8 +78,9 @@ def __validate_path(self, mpfa_path): def __extract_contents(self, mpfa_path): contents_path = tempfile.mkdtemp(prefix='mpfa-') - cmd = self.MPFA_EXTRACT_COMMAND.format(mpfa_path, contents_path) - subprocess.check_call(cmd.split(), universal_newlines=True) + self.MPFA_EXTRACT_COMMAND[2] = mpfa_path + self.MPFA_EXTRACT_COMMAND[4] = contents_path + subprocess.check_call(self.MPFA_EXTRACT_COMMAND, universal_newlines=True) self.__contents_path = contents_path @@ -105,8 +106,8 @@ def extract(self): def cleanup(self): if os.path.exists(self.__contents_path): - cmd = self.MPFA_CLEANUP_COMMAND.format(self.__contents_path) - subprocess.check_call(cmd.split(), universal_newlines=True) + self.MPFA_CLEANUP_COMMAND[2] = self.__contents_path + subprocess.check_call(self.MPFA_CLEANUP_COMMAND, universal_newlines=True) self.__contents_path = None self.__metadata = None @@ -122,11 +123,11 @@ def is_extracted(self): class ONIEUpdater(object): - ONIE_FW_UPDATE_CMD_ADD = '/usr/bin/mlnx-onie-fw-update.sh add {}' - ONIE_FW_UPDATE_CMD_REMOVE = '/usr/bin/mlnx-onie-fw-update.sh remove {}' - ONIE_FW_UPDATE_CMD_UPDATE = '/usr/bin/mlnx-onie-fw-update.sh update' - ONIE_FW_UPDATE_CMD_INSTALL = '/usr/bin/mlnx-onie-fw-update.sh update --no-reboot' - ONIE_FW_UPDATE_CMD_SHOW_PENDING = '/usr/bin/mlnx-onie-fw-update.sh show-pending' + ONIE_FW_UPDATE_CMD_ADD = ['/usr/bin/mlnx-onie-fw-update.sh', 'add', ''] + ONIE_FW_UPDATE_CMD_REMOVE = ['/usr/bin/mlnx-onie-fw-update.sh', 'remove', ''] + ONIE_FW_UPDATE_CMD_UPDATE = ['/usr/bin/mlnx-onie-fw-update.sh', 'update'] + ONIE_FW_UPDATE_CMD_INSTALL = ['/usr/bin/mlnx-onie-fw-update.sh', 'update', '--no-reboot'] + ONIE_FW_UPDATE_CMD_SHOW_PENDING = ['/usr/bin/mlnx-onie-fw-update.sh', 'show-pending'] ONIE_VERSION_PARSE_PATTERN = '([0-9]{4})\.([0-9]{2})-([0-9]+)\.([0-9]+)\.([0-9]+)-([0-9]+)' ONIE_VERSION_BASE_PARSE_PATTERN = '([0-9]+)\.([0-9]+)\.([0-9]+)' @@ -135,7 +136,7 @@ class ONIEUpdater(object): ONIE_VERSION_ATTR = 'onie_version' ONIE_NO_PENDING_UPDATES_ATTR = 'No pending firmware updates present' - ONIE_IMAGE_INFO_COMMAND = '/bin/bash {} -q -i' + ONIE_IMAGE_INFO_COMMAND = ['/bin/bash', '', '-q', '-i'] # Upgrading fireware from ONIE is not supported from the beginning on some platforms, like SN2700. # There is a logic to check the ONIE version in order to know whether it is supported. @@ -167,14 +168,24 @@ def __mount_onie_fs(self): self.__umount_onie_fs() cmd = "fdisk -l | grep 'ONIE boot' | awk '{print $1}'" - fs_path = subprocess.check_output(cmd, - stderr=subprocess.STDOUT, - shell=True, - universal_newlines=True).rstrip('\n') + with subprocess.Popen(['fdisk', '-l'], universal_newlines=True, stdout=subprocess.PIPE) as p1: + with subprocess.Popen(['grep', 'ONIE boot'], universal_newlines=True, stdin=p1.stdout, stdout=subprocess.PIPE) as p2: + with subprocess.Popen(['awk', '{print $1}'], universal_newlines=True, stdin=p2.stdout, stdout=subprocess.PIPE) as p3: + fs_path = p3.communicate()[0].rstrip('\n') + p1.wait() + p2.wait() + if p1.returncode != 0 and p2.returncode != 0 and p3.returncode != 0: + if p1.returncode != 0: + returncode = p1.returncode + elif p2.returncode != 0: + returncode = p2.returncode + elif p3.returncode != 0: + returncode = p3.returncode + raise subprocess.CalledProcessError(returncode=returncode, cmd=cmd, output=fs_path) os.mkdir(fs_mountpoint) - cmd = "mount -n -r -t ext4 {} {}".format(fs_path, fs_mountpoint) - subprocess.check_call(cmd, shell=True, universal_newlines=True) + cmd = ["mount", "-n", "-r", "-t", "ext4", fs_path, fs_mountpoint] + subprocess.check_call(cmd, universal_newlines=True) fs_onie_path = os.path.join(fs_mountpoint, 'onie/tools/lib/onie') os.symlink(fs_onie_path, onie_path) @@ -189,8 +200,8 @@ def __umount_onie_fs(self): os.unlink(onie_path) if os.path.ismount(fs_mountpoint): - cmd = "umount -rf {}".format(fs_mountpoint) - subprocess.check_call(cmd, shell=True, universal_newlines=True) + cmd = ["umount", "-rf", fs_mountpoint] + subprocess.check_call(cmd, universal_newlines=True) if os.path.exists(fs_mountpoint): os.rmdir(fs_mountpoint) @@ -198,20 +209,20 @@ def __umount_onie_fs(self): def __stage_update(self, image_path): rename_path = self.__add_prefix(image_path) - cmd = self.ONIE_FW_UPDATE_CMD_ADD.format(rename_path) + self.ONIE_FW_UPDATE_CMD_ADD[2] = rename_path try: - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(self.ONIE_FW_UPDATE_CMD_ADD, universal_newlines=True) except subprocess.CalledProcessError as e: raise RuntimeError("Failed to stage firmware update: {}".format(str(e))) def __unstage_update(self, image_path): rename_path = self.__add_prefix(image_path) - cmd = self.ONIE_FW_UPDATE_CMD_REMOVE.format(os.path.basename(rename_path)) + self.ONIE_FW_UPDATE_CMD_REMOVE[2] = os.path.basename(rename_path) try: - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(self.ONIE_FW_UPDATE_CMD_REMOVE, universal_newlines=True) except subprocess.CalledProcessError as e: raise RuntimeError("Failed to unstage firmware update: {}".format(str(e))) @@ -222,7 +233,7 @@ def __trigger_update(self, allow_reboot): cmd = self.ONIE_FW_UPDATE_CMD_INSTALL try: - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(cmd, universal_newlines=True) except subprocess.CalledProcessError as e: raise RuntimeError("Failed to trigger firmware update: {}".format(str(e))) @@ -230,7 +241,7 @@ def __is_update_staged(self, image_path): cmd = self.ONIE_FW_UPDATE_CMD_SHOW_PENDING try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(cmd, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -315,10 +326,10 @@ def get_onie_firmware_info(self, image_path): try: self.__mount_onie_fs() - cmd = self.ONIE_IMAGE_INFO_COMMAND.format(image_path) + self.ONIE_IMAGE_INFO_COMMAND[1] = image_path try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(self.ONIE_IMAGE_INFO_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -413,25 +424,6 @@ def _read_generic_file(filename, len, ignore_errors=False): return result - @staticmethod - def _get_command_result(cmdline): - try: - proc = subprocess.Popen(cmdline, - stdout=subprocess.PIPE, - shell=True, - stderr=subprocess.STDOUT, - universal_newlines=True) - stdout = proc.communicate()[0] - rc = proc.wait() - result = stdout.rstrip('\n') - if rc != 0: - raise RuntimeError("Failed to execute command {}, return code {}, message {}".format(cmdline, rc, stdout)) - - except OSError as e: - raise RuntimeError("Failed to execute command {} due to {}".format(cmdline, repr(e))) - - return result - def _check_file_validity(self, image_path): if not os.path.isfile(image_path): print("ERROR: File {} doesn't exist or is not a file".format(image_path)) @@ -502,10 +494,10 @@ class ComponentSSD(Component): POWER_CYCLE_REQUIRED_ATTR = 'Power Cycle Required' UPGRADE_REQUIRED_ATTR = 'Upgrade Required' - SSD_INFO_COMMAND = "/usr/bin/mlnx-ssd-fw-update.sh -q" - SSD_FIRMWARE_INFO_COMMAND = "/usr/bin/mlnx-ssd-fw-update.sh -q -i {}" - SSD_FIRMWARE_INSTALL_COMMAND = "/usr/bin/mlnx-ssd-fw-update.sh --no-power-cycle -y -u -i {}" - SSD_FIRMWARE_UPDATE_COMMAND = "/usr/bin/mlnx-ssd-fw-update.sh -y -u -i {}" + SSD_INFO_COMMAND = ["/usr/bin/mlnx-ssd-fw-update.sh", "-q"] + SSD_FIRMWARE_INFO_COMMAND = ["/usr/bin/mlnx-ssd-fw-update.sh", "-q", "-i", ""] + SSD_FIRMWARE_INSTALL_COMMAND = ["/usr/bin/mlnx-ssd-fw-update.sh", "--no-power-cycle", "-y", "-u", "-i", ""] + SSD_FIRMWARE_UPDATE_COMMAND = ["/usr/bin/mlnx-ssd-fw-update.sh", "-y", "-u", "-i", ""] def __init__(self): super(ComponentSSD, self).__init__() @@ -519,13 +511,15 @@ def __install_firmware(self, image_path, allow_reboot=True): return False if allow_reboot: - cmd = self.SSD_FIRMWARE_UPDATE_COMMAND.format(image_path) + self.SSD_FIRMWARE_UPDATE_COMMAND[4] = image_path + cmd = self.SSD_FIRMWARE_UPDATE_COMMAND else: - cmd = self.SSD_FIRMWARE_INSTALL_COMMAND.format(image_path) + self.SSD_FIRMWARE_INSTALL_COMMAND[5] = image_path + cmd = self.SSD_FIRMWARE_INSTALL_COMMAND try: print("INFO: Installing {} firmware update".format(self.name)) - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(cmd, universal_newlines=True) except subprocess.CalledProcessError as e: print("ERROR: Failed to update {} firmware: {}".format(self.name, str(e))) return False @@ -563,10 +557,8 @@ def auto_update_firmware(self, image_path, boot_action): return FW_AUTO_SCHEDULED def get_firmware_version(self): - cmd = self.SSD_INFO_COMMAND - try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(self.SSD_INFO_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -579,10 +571,10 @@ def get_firmware_version(self): raise RuntimeError("Failed to parse {} version".format(self.name)) def get_available_firmware_version(self, image_path): - cmd = self.SSD_FIRMWARE_INFO_COMMAND.format(image_path) + self.SSD_FIRMWARE_INFO_COMMAND[3] = image_path try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(self.SSD_FIRMWARE_INFO_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -614,10 +606,10 @@ def get_available_firmware_version(self, image_path): return available_firmware_version def get_firmware_update_notification(self, image_path): - cmd = self.SSD_FIRMWARE_INFO_COMMAND.format(image_path) + self.SSD_FIRMWARE_INFO_COMMAND[3] = image_path try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(self.SSD_FIRMWARE_INFO_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -660,7 +652,7 @@ class ComponentBIOS(Component): COMPONENT_DESCRIPTION = 'BIOS - Basic Input/Output System' COMPONENT_FIRMWARE_EXTENSION = '.rom' - BIOS_VERSION_COMMAND = 'dmidecode --oem-string 1' + BIOS_VERSION_COMMAND = ['dmidecode', '--oem-string', '1'] def __init__(self): super(ComponentBIOS, self).__init__() @@ -688,10 +680,8 @@ def __install_firmware(self, image_path, allow_reboot=True): return True def get_firmware_version(self): - cmd = self.BIOS_VERSION_COMMAND - try: - version = subprocess.check_output(cmd.split(), + version = subprocess.check_output(self.BIOS_VERSION_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -716,7 +706,7 @@ class ComponentBIOSSN2201(Component): COMPONENT_NAME = 'BIOS' COMPONENT_DESCRIPTION = 'BIOS - Basic Input/Output System' - BIOS_VERSION_COMMAND = 'dmidecode -t0' + BIOS_VERSION_COMMAND = ['dmidecode', '-t0'] def __init__(self): super(ComponentBIOSSN2201, self).__init__() @@ -725,10 +715,8 @@ def __init__(self): self.description = self.COMPONENT_DESCRIPTION def get_firmware_version(self): - cmd = self.BIOS_VERSION_COMMAND - try: - output = subprocess.check_output(cmd.split(), + output = subprocess.check_output(self.BIOS_VERSION_COMMAND, stderr=subprocess.STDOUT, universal_newlines=True).rstrip('\n') except subprocess.CalledProcessError as e: @@ -764,7 +752,7 @@ class ComponentCPLD(Component): CPLD_PART_NUMBER_DEFAULT = '0' CPLD_VERSION_MINOR_DEFAULT = '0' - CPLD_FIRMWARE_UPDATE_COMMAND = 'cpldupdate --dev {} --print-progress {}' + CPLD_FIRMWARE_UPDATE_COMMAND = ['cpldupdate', '--dev', '', '--print-progress', ''] def __init__(self, idx): super(ComponentCPLD, self).__init__() @@ -796,12 +784,13 @@ def _install_firmware(self, image_path): mst_dev = self.__get_mst_device() if mst_dev is None: return False - - cmd = self.CPLD_FIRMWARE_UPDATE_COMMAND.format(mst_dev, image_path) + self.CPLD_FIRMWARE_UPDATE_COMMAND[2] = mst_dev + self.CPLD_FIRMWARE_UPDATE_COMMAND[4] = image_path + cmd = self.CPLD_FIRMWARE_UPDATE_COMMAND try: print("INFO: Installing {} firmware update: path={}".format(self.name, image_path)) - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(cmd, universal_newlines=True) except subprocess.CalledProcessError as e: print("ERROR: Failed to update {} firmware: {}".format(self.name, str(e))) return False @@ -910,14 +899,14 @@ def get_component_list(cls): class ComponentCPLDSN2201(ComponentCPLD): - CPLD_FIRMWARE_UPDATE_COMMAND = 'cpldupdate --gpio {} --uncustomized --print-progress' + CPLD_FIRMWARE_UPDATE_COMMAND = ['cpldupdate', '--gpio', '', '--uncustomized', '--print-progress'] def _install_firmware(self, image_path): - cmd = self.CPLD_FIRMWARE_UPDATE_COMMAND.format(image_path) + self.CPLD_FIRMWARE_UPDATE_COMMAND[2] = image_path try: print("INFO: Installing {} firmware update: path={}".format(self.name, image_path)) - subprocess.check_call(cmd.split(), universal_newlines=True) + subprocess.check_call(self.CPLD_FIRMWARE_UPDATE_COMMAND, universal_newlines=True) except subprocess.CalledProcessError as e: print("ERROR: Failed to update {} firmware: {}".format(self.name, str(e))) return False diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/psu.py b/platform/mellanox/mlnx-platform-api/sonic_platform/psu.py index 05f38f62a0f1..e447bbb43565 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/psu.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/psu.py @@ -551,7 +551,7 @@ def run(cls, psu, threshold_value, threshold_file): return threshold_value # Run a sensors -s command to triger hardware to get the real threashold value - utils.run_command('sensors -s') + utils.run_command(['sensors', '-s']) # Wait for the threshold value change return cls.wait_set_done(threshold_file) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py index 617b4f33d636..404d7f9f563c 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py @@ -25,7 +25,6 @@ try: import subprocess import os - from sonic_platform_base.sonic_eeprom import eeprom_dts from sonic_py_common.logger import Logger from . import utils from .device_data import DeviceDataManager @@ -186,10 +185,11 @@ def write_mlxreg_eeprom(self, num_bytes, dword, device_address, page): return False try: - cmd = "mlxreg -d /dev/mst/{} --reg_name MCIA --indexes \ - slot_index={},module={},device_address={},page_number={},i2c_device_address=0x50,size={},bank_number=0 \ - --set {} -y".format(self.mst_pci_device, self.slot_id, self.sdk_index, device_address, page, num_bytes, dword) - subprocess.check_call(cmd, shell=True, universal_newlines=True, stdout=subprocess.DEVNULL) + cmd = ["mlxreg", "-d", "", "--reg_name", "MCIA", "--indexes", "", "--set", "", "-y"] + cmd[2] = "/dev/mst/" + self.mst_pci_device + cmd[6] = "slot_index={},module={},device_address={},page_number={},i2c_device_address=0x50,size={},bank_number=0".format(self.slot_id, self.sdk_index, device_address, page, num_bytes) + cmd[8] = dword + subprocess.check_call(cmd, universal_newlines=True, stdout=subprocess.DEVNULL) except subprocess.CalledProcessError as e: logger.log_error("Error! Unable to write data dword={} for {} port, page {} offset {}, rc = {}, err msg: {}".format(dword, self.sdk_index, page, device_address, e.returncode, e.output)) return False @@ -197,10 +197,11 @@ def write_mlxreg_eeprom(self, num_bytes, dword, device_address, page): def read_mlxred_eeprom(self, offset, page, num_bytes): try: - cmd = "mlxreg -d /dev/mst/{} --reg_name MCIA --indexes \ - slot_index={},module={},device_address={},page_number={},i2c_device_address=0x50,size={},bank_number=0 \ - --get".format(self.mst_pci_device, self.slot_id, self.sdk_index, offset, page, num_bytes) - result = subprocess.check_output(cmd, universal_newlines=True, shell=True) + + cmd = ["mlxreg", "-d", "", "--reg_name", "MCIA", "--indexes", "", "--get"] + cmd[2] = "/dev/mst/" + self.mst_pci_device + cmd[6] = "slot_index={},module={},device_address={},page_number={},i2c_device_address=0x50,size={},bank_number=0".format(self.slot_id, self.sdk_index, offset, page, num_bytes) + result = subprocess.check_output(cmd, universal_newlines=True) except subprocess.CalledProcessError as e: logger.log_error("Error! Unable to read data for {} port, page {} offset {}, rc = {}, err msg: {}".format(self.sdk_index, page, offset, e.returncode, e.output)) return None @@ -355,11 +356,12 @@ def _read_eeprom_specific_bytes(self, offset, num_bytes): return None eeprom_raw = [] - ethtool_cmd = "ethtool -m sfp{} hex on offset {} length {}".format(self.index, offset, num_bytes) + ethtool_cmd = ["ethtool", "-m", "", "hex", "on", "offset", "", "length", ""] + ethtool_cmd[2] = "sfp" + str(self.index) + ethtool_cmd[6] = str(offset) + ethtool_cmd[8] = str(num_bytes) try: - output = subprocess.check_output(ethtool_cmd, - shell=True, - universal_newlines=True) + output = subprocess.check_output(ethtool_cmd, universal_newlines=True) output_lines = output.splitlines() first_line_raw = output_lines[0] if "Offset" in first_line_raw: diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/utils.py b/platform/mellanox/mlnx-platform-api/sonic_platform/utils.py index 0db7e1e26e30..2bc312cd2234 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/utils.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/utils.py @@ -187,9 +187,8 @@ def is_host(): return True for host and False for docker """ try: - proc = subprocess.Popen("docker --version 2>/dev/null", + proc = subprocess.Popen(["docker", "--version"], stdout=subprocess.PIPE, - shell=True, stderr=subprocess.STDOUT, universal_newlines=True) stdout = proc.communicate()[0] @@ -221,7 +220,7 @@ def run_command(command): :return: Output of the shell command. """ try: - process = subprocess.Popen(command, shell=True, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + process = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) return process.communicate()[0].strip() except Exception: return None diff --git a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py index 8ae4ece8eec6..46760154849f 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py @@ -17,6 +17,7 @@ import os import sys +import subprocess from mock import MagicMock if sys.version_info.major == 3: @@ -172,7 +173,6 @@ def test_sfp(self): @mock.patch('sonic_platform.device_data.DeviceDataManager.get_sfp_count', MagicMock(return_value=3)) def test_change_event(self): from sonic_platform.sfp_event import sfp_event - from sonic_platform.sfp import SFP return_port_dict = {1: '1'} def mock_check_sfp_status(self, port_dict, error_dict, timeout): @@ -276,12 +276,12 @@ def test_revision_permission(self): #Override the dmi file sonic_platform.chassis.DMI_FILE = "/tmp/dmi_file" new_dmi_file = sonic_platform.chassis.DMI_FILE - os.system("touch " + new_dmi_file) - os.system("chmod -r " + new_dmi_file) + subprocess.run(["touch ", new_dmi_file]) + subprocess.run(["chmod", "-r", new_dmi_file]) chassis = Chassis() rev = chassis.get_revision() sonic_platform.chassis.DMI_FILE = old_dmi_file - os.system("rm -f " + new_dmi_file) + subprocess.run(["rm", "-f", new_dmi_file]) assert rev == "N/A" def test_get_port_or_cage_type(self): diff --git a/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py b/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py index 5845a1b2cadb..bf86d7540f93 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py @@ -15,7 +15,6 @@ # limitations under the License. # import os -import pytest import subprocess import sys from mock import MagicMock, patch @@ -143,7 +142,7 @@ def mock_read_str_from_file(file_path, default='', raise_exception=False): assert subprocess.check_call.call_count == 0 fan.get_presence = MagicMock(return_value=True) assert fan.set_speed(60) - subprocess.check_call.assert_called_with("i2cset -f -y {0} {1} {2} {3} wp".format('bus', 'addr', 'command', hex(60)), shell=True, universal_newlines=True) + subprocess.check_call.assert_called_with(["i2cset", "-f", "-y", "bus", "addr", "command", hex(60), "wp"], universal_newlines=True) subprocess.check_call = MagicMock(side_effect=subprocess.CalledProcessError('', '')) assert not fan.set_speed(60) subprocess.check_call = MagicMock() diff --git a/platform/mellanox/mlnx-platform-api/tests/test_utils.py b/platform/mellanox/mlnx-platform-api/tests/test_utils.py index e1052202d416..c4c8d0c000a9 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_utils.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_utils.py @@ -118,7 +118,7 @@ def func(): assert mock_log.call_count == 1 def test_run_command(self): - output = utils.run_command('ls') + output = utils.run_command(['ls']) assert output @mock.patch('sonic_py_common.device_info.get_path_to_hwsku_dir', mock.MagicMock(return_value='/tmp')) From 34ab55c56ebf407f982f25f2cee6978c7602d0ae Mon Sep 17 00:00:00 2001 From: maipbui Date: Thu, 15 Sep 2022 19:01:05 +0000 Subject: [PATCH 04/10] Replace shell=True Signed-off-by: maipbui --- .../mlnx-platform-api/sonic_platform/sfp.py | 19 ++++++++++--------- .../mlnx-platform-api/tests/test_psu.py | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py index 404d7f9f563c..ccb51a3f9755 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py @@ -26,6 +26,7 @@ import subprocess import os from sonic_py_common.logger import Logger + from sonic_py_common.general import check_output_pipe from . import utils from .device_data import DeviceDataManager from sonic_platform_base.sonic_xcvr.sfp_optoe_base import SfpOptoeBase @@ -316,7 +317,7 @@ def __init__(self, sfp_index, sfp_type=None, slot_id=0, linecard_port_count=0, l def get_mst_pci_device(self): device_name = None try: - device_name = subprocess.check_output("ls /dev/mst/ | grep pciconf", universal_newlines=True, shell=True).strip() + device_name = check_output_pipe(["ls", "/dev/mst/"], ["grep", "pciconf"], universal_newlines=True).strip() except subprocess.CalledProcessError as e: logger.log_error("Failed to find mst PCI device rc={} err.msg={}".format(e.returncode, e.output)) return device_name @@ -480,9 +481,9 @@ def get_lpmode(self): get_lpmode_code = 'from sonic_platform import sfp;\n' \ 'with sfp.SdkHandleContext() as sdk_handle:' \ 'print(sfp.SFP._get_lpmode(sdk_handle, {}, {}))'.format(self.sdk_index, self.slot_id) - lpm_cmd = "docker exec pmon python3 -c \"{}\"".format(get_lpmode_code) + lpm_cmd = ["docker", "exec", "pmon", "python3", "-c", get_lpmode_code] try: - output = subprocess.check_output(lpm_cmd, shell=True, universal_newlines=True) + output = subprocess.check_output(lpm_cmd, universal_newlines=True) return 'True' in output except subprocess.CalledProcessError as e: print("Error! Unable to get LPM for {}, rc = {}, err msg: {}".format(self.sdk_index, e.returncode, e.output)) @@ -521,10 +522,10 @@ def reset(self): 'with sfp.SdkHandleContext() as sdk_handle:' \ 'print(sfp.SFP._reset(sdk_handle, {}, {}))' \ .format(self.sdk_index, self.slot_id) - reset_cmd = "docker exec pmon python3 -c \"{}\"".format(reset_code) + reset_cmd = ["docker", "exec", "pmon", "python3", "-c", reset_code] try: - output = subprocess.check_output(reset_cmd, shell=True, universal_newlines=True) + output = subprocess.check_output(reset_cmd, universal_newlines=True) return 'True' in output except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(self.sdk_index, e.returncode, e.output)) @@ -679,11 +680,11 @@ def set_lpmode(self, lpmode): 'with sfp.SdkHandleContext() as sdk_handle:' \ 'print(sfp.SFP._set_lpmode({}, sdk_handle, {}, {}))' \ .format('True' if lpmode else 'False', self.sdk_index, self.slot_id) - lpm_cmd = "docker exec pmon python3 -c \"{}\"".format(set_lpmode_code) + lpm_cmd = ["docker", "exec", "pmon", "python3", "-c", set_lpmode_code] # Set LPM try: - output = subprocess.check_output(lpm_cmd, shell=True, universal_newlines=True) + output = subprocess.check_output(lpm_cmd, universal_newlines=True) return 'True' in output except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(self.sdk_index, e.returncode, e.output)) @@ -794,9 +795,9 @@ def get_presence(self): get_presence_code = 'from sonic_platform import sfp;\n' \ 'with sfp.SdkHandleContext() as sdk_handle:' \ 'print(sfp.RJ45Port._get_presence(sdk_handle, {}))'.format(self.sdk_index) - presence_cmd = "docker exec pmon python3 -c \"{}\"".format(get_presence_code) + presence_cmd = ["docker", "exec", "pmon", "python3", "-c", get_presence_code] try: - output = subprocess.check_output(presence_cmd, shell=True, universal_newlines=True) + output = subprocess.check_output(presence_cmd, universal_newlines=True) return 'True' in output except subprocess.CalledProcessError as e: print("Error! Unable to get presence for {}, rc = {}, err msg: {}".format(self.sdk_index, e.returncode, e.output)) diff --git a/platform/mellanox/mlnx-platform-api/tests/test_psu.py b/platform/mellanox/mlnx-platform-api/tests/test_psu.py index 5deb72bfb0e1..d92b8f716a6d 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_psu.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_psu.py @@ -160,4 +160,4 @@ def get_entry_value(key): # Normal vpd_info[InvalidPsuVolWA.CAPACITY_FIELD] = InvalidPsuVolWA.EXPECT_CAPACITY assert InvalidPsuVolWA.run(psu, InvalidPsuVolWA.INVALID_VOLTAGE_VALUE, '') == 9999 - mock_run_command.assert_called_with('sensors -s') + mock_run_command.assert_called_with(['sensors', '-s']) From 7e3d01e313553d2f0a288713baa3a0ca718b303c Mon Sep 17 00:00:00 2001 From: maipbui Date: Thu, 15 Sep 2022 20:36:44 +0000 Subject: [PATCH 05/10] Resolve conflict Signed-off-by: maipbui --- platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py index ccb51a3f9755..a817a0a5b4a0 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py @@ -362,7 +362,9 @@ def _read_eeprom_specific_bytes(self, offset, num_bytes): ethtool_cmd[6] = str(offset) ethtool_cmd[8] = str(num_bytes) try: - output = subprocess.check_output(ethtool_cmd, universal_newlines=True) + output = subprocess.check_output(ethtool_cmd, + universal_newlines=True, + stderr=subprocess.PIPE) output_lines = output.splitlines() first_line_raw = output_lines[0] if "Offset" in first_line_raw: From 98ef8c2970d53e285042c5014da4aa82d0c85a7f Mon Sep 17 00:00:00 2001 From: maipbui Date: Fri, 16 Sep 2022 00:05:56 +0000 Subject: [PATCH 06/10] Add pytest import Signed-off-by: maipbui --- platform/mellanox/mlnx-platform-api/tests/test_fan_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py b/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py index bf86d7540f93..75d1cbe707fe 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_fan_api.py @@ -15,6 +15,7 @@ # limitations under the License. # import os +import pytest import subprocess import sys from mock import MagicMock, patch From 20e2cd77420a20b07a6a943bfaf1776914e56038 Mon Sep 17 00:00:00 2001 From: maipbui Date: Fri, 16 Sep 2022 18:51:40 +0000 Subject: [PATCH 07/10] Fix command Signed-off-by: maipbui --- .../sonic_platform/component.py | 16 +++++++++------- .../mlnx-platform-api/sonic_platform/sfp.py | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py index f75fcfa6e323..22e7b99a72c6 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py @@ -168,20 +168,22 @@ def __mount_onie_fs(self): self.__umount_onie_fs() cmd = "fdisk -l | grep 'ONIE boot' | awk '{print $1}'" - with subprocess.Popen(['fdisk', '-l'], universal_newlines=True, stdout=subprocess.PIPE) as p1: - with subprocess.Popen(['grep', 'ONIE boot'], universal_newlines=True, stdin=p1.stdout, stdout=subprocess.PIPE) as p2: - with subprocess.Popen(['awk', '{print $1}'], universal_newlines=True, stdin=p2.stdout, stdout=subprocess.PIPE) as p3: + cmd1 = ['fdisk', '-l'] + cmd2 = ['grep', 'ONIE boot'] + cmd3 = ['awk', '{print $1}'] + with subprocess.Popen(cmd1, universal_newlines=True, stdout=subprocess.PIPE) as p1: + with subprocess.Popen(cmd2, universal_newlines=True, stdin=p1.stdout, stdout=subprocess.PIPE) as p2: + with subprocess.Popen(cmd3, universal_newlines=True, stdin=p2.stdout, stdout=subprocess.PIPE) as p3: fs_path = p3.communicate()[0].rstrip('\n') p1.wait() p2.wait() if p1.returncode != 0 and p2.returncode != 0 and p3.returncode != 0: if p1.returncode != 0: - returncode = p1.returncode + raise subprocess.CalledProcessError(returncode=p1.returncode, cmd=cmd1, output=p1.stdout) elif p2.returncode != 0: - returncode = p2.returncode + raise subprocess.CalledProcessError(returncode=p2.returncode, cmd=cmd2, output=p2.stdout) elif p3.returncode != 0: - returncode = p3.returncode - raise subprocess.CalledProcessError(returncode=returncode, cmd=cmd, output=fs_path) + raise subprocess.CalledProcessError(returncode=p3.returncode, cmd=cmd, output=fs_path) os.mkdir(fs_mountpoint) cmd = ["mount", "-n", "-r", "-t", "ext4", fs_path, fs_mountpoint] diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py index acc0747dea93..3605d3971283 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py @@ -317,7 +317,7 @@ def __init__(self, sfp_index, sfp_type=None, slot_id=0, linecard_port_count=0, l def get_mst_pci_device(self): device_name = None try: - device_name = check_output_pipe(["ls", "/dev/mst/"], ["grep", "pciconf"], universal_newlines=True).strip() + device_name = check_output_pipe(["ls", "/dev/mst/"], ["grep", "pciconf"]).strip() except subprocess.CalledProcessError as e: logger.log_error("Failed to find mst PCI device rc={} err.msg={}".format(e.returncode, e.output)) return device_name From a16ed293a113c1cb10bf67b7c7afac3d0b9b1a4a Mon Sep 17 00:00:00 2001 From: maipbui Date: Sun, 18 Sep 2022 02:13:17 +0000 Subject: [PATCH 08/10] Use common lib function Signed-off-by: maipbui --- .../mlnx-platform-api/sonic_platform/component.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py index 22e7b99a72c6..3b9ce86ac901 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/component.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/component.py @@ -30,6 +30,7 @@ import tempfile import subprocess from sonic_py_common import device_info + from sonic_py_common.general import check_output_pipe if sys.version_info[0] > 2: import configparser else: @@ -171,19 +172,7 @@ def __mount_onie_fs(self): cmd1 = ['fdisk', '-l'] cmd2 = ['grep', 'ONIE boot'] cmd3 = ['awk', '{print $1}'] - with subprocess.Popen(cmd1, universal_newlines=True, stdout=subprocess.PIPE) as p1: - with subprocess.Popen(cmd2, universal_newlines=True, stdin=p1.stdout, stdout=subprocess.PIPE) as p2: - with subprocess.Popen(cmd3, universal_newlines=True, stdin=p2.stdout, stdout=subprocess.PIPE) as p3: - fs_path = p3.communicate()[0].rstrip('\n') - p1.wait() - p2.wait() - if p1.returncode != 0 and p2.returncode != 0 and p3.returncode != 0: - if p1.returncode != 0: - raise subprocess.CalledProcessError(returncode=p1.returncode, cmd=cmd1, output=p1.stdout) - elif p2.returncode != 0: - raise subprocess.CalledProcessError(returncode=p2.returncode, cmd=cmd2, output=p2.stdout) - elif p3.returncode != 0: - raise subprocess.CalledProcessError(returncode=p3.returncode, cmd=cmd, output=fs_path) + fs_path = check_output_pipe(cmd1, cmd2, cmd3) os.mkdir(fs_mountpoint) cmd = ["mount", "-n", "-r", "-t", "ext4", fs_path, fs_mountpoint] From 1e62cff8ab80bc920db8cbc90c77b513005619cb Mon Sep 17 00:00:00 2001 From: maipbui Date: Wed, 5 Oct 2022 20:20:47 +0000 Subject: [PATCH 09/10] Fix build Signed-off-by: maipbui --- platform/mellanox/mlnx-platform-api/sonic_platform/fan.py | 4 ++-- platform/mellanox/mlnx-platform-api/tests/test_chassis.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/fan.py b/platform/mellanox/mlnx-platform-api/sonic_platform/fan.py index 8c40e2b61809..ec205fe60431 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/fan.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/fan.py @@ -218,8 +218,8 @@ def set_speed(self, speed): addr = utils.read_str_from_file(self.psu_i2c_addr_path, raise_exception=True) command = utils.read_str_from_file(self.psu_i2c_command_path, raise_exception=True) speed = self.PSU_FAN_SPEED[int(speed // 10)] - command = "i2cset -f -y {0} {1} {2} {3} wp".format(bus, addr, command, speed) - subprocess.check_call(command, shell = True, universal_newlines=True) + command = ["i2cset", "-f", "-y", bus, addr, command, speed, "wp"] + subprocess.check_call(command, universal_newlines=True) return True except subprocess.CalledProcessError as ce: logger.log_error('Failed to call command {}, return code={}, command output={}'.format(ce.cmd, ce.returncode, ce.output)) diff --git a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py index 46760154849f..09c619a7ed1e 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py @@ -276,7 +276,7 @@ def test_revision_permission(self): #Override the dmi file sonic_platform.chassis.DMI_FILE = "/tmp/dmi_file" new_dmi_file = sonic_platform.chassis.DMI_FILE - subprocess.run(["touch ", new_dmi_file]) + subprocess.run(["touch", new_dmi_file]) subprocess.run(["chmod", "-r", new_dmi_file]) chassis = Chassis() rev = chassis.get_revision() From c35ec09ace7a7cd4b5f577208220fdba56612e62 Mon Sep 17 00:00:00 2001 From: maipbui Date: Wed, 5 Oct 2022 22:52:37 +0000 Subject: [PATCH 10/10] Remove run Signed-off-by: maipbui --- device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py | 8 ++++---- platform/mellanox/mlnx-platform-api/tests/test_chassis.py | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py index 01dc2baa74d7..d461cc30f323 100644 --- a/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py +++ b/device/mellanox/x86_64-mlnx_msn2700-r0/plugins/sfputil.py @@ -158,7 +158,7 @@ def get_low_power_mode(self, port_num): lpm_cmd = ["docker", "exec", "syncd", "python", "/usr/share/sonic/platform/plugins/sfplpmget.py", str(port_num)] try: - output = subprocess.run(lpm_cmd, universal_newlines=True, capture_output=True, check=True).stdout + output = subprocess.check_output(lpm_cmd, universal_newlines=True) if 'LPM ON' in output: return True except subprocess.CalledProcessError as e: @@ -182,7 +182,7 @@ def set_low_power_mode(self, port_num, lpmode): # Set LPM try: - subprocess.run(lpm_cmd, check=True, universal_newlines=True, capture_output=True).stdout + subprocess.check_output(lpm_cmd, universal_newlines=True) except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) return False @@ -197,7 +197,7 @@ def reset(self, port_num): lpm_cmd = ["docker", "exec", "syncd", "python", "/usr/share/sonic/platform/plugins/sfpreset.py", str(port_num)] try: - subprocess.run(lpm_cmd, check=True, universal_newlines=True, capture_output=True).stdout + subprocess.check_output(lpm_cmd, universal_newlines=True) return True except subprocess.CalledProcessError as e: print("Error! Unable to set LPM for {}, rc = {}, err msg: {}".format(port_num, e.returncode, e.output)) @@ -269,7 +269,7 @@ def _read_eeprom_specific_bytes_via_ethtool(self, port_num, offset, num_bytes): eeprom_raw = [] ethtool_cmd = ["ethtool", "-m", sfpname, "hex", "on", "offset", str(offset), "length", str(num_bytes)] try: - output = subprocess.run(ethtool_cmd, check=True, universal_newlines=True, capture_output=True).stdout + output = subprocess.check_output(ethtool_cmd, universal_newlines=True) output_lines = output.splitlines() first_line_raw = output_lines[0] if "Offset" in first_line_raw: diff --git a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py index 09c619a7ed1e..2aa4f78855f3 100644 --- a/platform/mellanox/mlnx-platform-api/tests/test_chassis.py +++ b/platform/mellanox/mlnx-platform-api/tests/test_chassis.py @@ -276,12 +276,12 @@ def test_revision_permission(self): #Override the dmi file sonic_platform.chassis.DMI_FILE = "/tmp/dmi_file" new_dmi_file = sonic_platform.chassis.DMI_FILE - subprocess.run(["touch", new_dmi_file]) - subprocess.run(["chmod", "-r", new_dmi_file]) + subprocess.call(["touch", new_dmi_file]) + subprocess.call(["chmod", "-r", new_dmi_file]) chassis = Chassis() rev = chassis.get_revision() sonic_platform.chassis.DMI_FILE = old_dmi_file - subprocess.run(["rm", "-f", new_dmi_file]) + subprocess.call(["rm", "-f", new_dmi_file]) assert rev == "N/A" def test_get_port_or_cage_type(self):