From c3cd2e789e7278e9b58ca65ed830a1bdd2ee2093 Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Wed, 22 Sep 2021 05:40:21 +0000 Subject: [PATCH 1/7] Use devicelist to check instead of machine.conf --- sonic_installer/bootloader/grub.py | 44 +++++++++++++----------------- sonic_installer/main.py | 4 +-- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index 0202da76bd..cf91ccc2ec 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -18,7 +18,7 @@ from .onie import OnieInstallerBootloader from .onie import default_sigpipe -MACHINE_CONF = "installer/machine.conf" +DEVICES_DIR = "installer/devices" class GrubBootloader(OnieInstallerBootloader): @@ -85,35 +85,29 @@ def remove_image(self, image): run_command('grub-set-default --boot-directory=' + HOST_PATH + ' 0') click.echo('Image removed') + def platform_in_devices_list(self, platform, image_path): + """ + For those images that don't have devices list builtin, check_output will raise an exception. + In this case, we simply return True to make it worked compatible as before. + Otherwise, we need to check if platform is inside the supported target devices list. + """ + try: + with open(os.devnull, 'w') as fnull: + p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) + output = subprocess.check_output(["tar", "tf", "-", DEVICES_DIR], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) + return platform in output + except subprocess.CalledProcessError: + return True + def verify_image_platform(self, image_path): if not os.path.isfile(image_path): return False - # Get running platform's ASIC - try: - version_info = device_info.get_sonic_version_info() - if version_info: - asic_type = version_info['asic_type'] - else: - asic_type = None - except (KeyError, TypeError) as e: - click.echo("Caught an exception: " + str(e)) - - # Get installing image's ASIC - p1 = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) - p2 = subprocess.Popen(["tar", "xf", "-", MACHINE_CONF, "-O"], stdin=p1.stdout, stdout=subprocess.PIPE, preexec_fn=default_sigpipe) - p3 = subprocess.Popen(["sed", "-n", r"s/^machine=\(.*\)/\1/p"], stdin=p2.stdout, stdout=subprocess.PIPE, preexec_fn=default_sigpipe, text=True) - - stdout = p3.communicate()[0] - image_asic = stdout.rstrip('\n') + # Get running platform + platform = device_info.get_platform() - # Return false if machine is not found or unexpected issue occur - if not image_asic: - return False - - if asic_type == image_asic: - return True - return False + # Check if platform is inside image's target platforms + return self.platform_in_devices_list(platform, image_path) @classmethod def detect(cls): diff --git a/sonic_installer/main.py b/sonic_installer/main.py index 72646531a9..4add57fe44 100644 --- a/sonic_installer/main.py +++ b/sonic_installer/main.py @@ -533,14 +533,14 @@ def install(url, force, skip_platform_check=False, skip_migration=False, skip_pa raise click.Abort() else: # Verify not installing non-secure image in a secure running image - if not bootloader.verify_secureboot_image(image_path) and not force: + if not force and not bootloader.verify_secureboot_image(image_path): echo_and_log("Image file '{}' is of a different type than running image.\n".format(url) + "If you are sure you want to install this image, use -f|--force|--skip-secure-check.\n" + "Aborting...", LOG_ERR) raise click.Abort() # Verify that the binary image is of the same platform type as running platform - if not bootloader.verify_image_platform(image_path) and not skip_platform_check: + if not skip_platform_check and not bootloader.verify_image_platform(image_path): echo_and_log("Image file '{}' is of a different platform type than running platform.\n".format(url) + "If you are sure you want to install this image, use --skip-platform-check.\n" + "Aborting...", LOG_ERR) From 653fdb2a48e49a6f367613a1427e99cef384cfee Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Thu, 23 Sep 2021 08:57:35 +0000 Subject: [PATCH 2/7] 1. Change platforms list into one file; 2. Add Aboot support --- sonic_installer/bootloader/aboot.py | 17 ++++++++++++++++- sonic_installer/bootloader/grub.py | 6 +++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/sonic_installer/bootloader/aboot.py b/sonic_installer/bootloader/aboot.py index a44c44fdd7..3c5d4dc273 100644 --- a/sonic_installer/bootloader/aboot.py +++ b/sonic_installer/bootloader/aboot.py @@ -15,6 +15,7 @@ from M2Crypto import X509 +from sonic_py_common import device_info from ..common import ( HOST_PATH, IMAGE_DIR_PREFIX, @@ -164,7 +165,21 @@ def get_binary_image_version(self, image_path): return IMAGE_PREFIX + version.strip() def verify_image_platform(self, image_path): - return os.path.isfile(image_path) + if not os.path.isfile(image_path): + return False + + # Get running platform + platform = device_info.get_platform() + + # If .platforms_asic is not existed, we simply return True for backward + # compatibility. Otherwise, we check if current platform is inside the + # supported target platforms list. + try: + output = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], text=True) + except subprocess.CalledProcessError: + return True + + return platform in output def verify_secureboot_image(self, image_path): try: diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index cf91ccc2ec..7a1537785d 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -18,7 +18,7 @@ from .onie import OnieInstallerBootloader from .onie import default_sigpipe -DEVICES_DIR = "installer/devices" +PLATFORMS_LIST = "installer/devices/platforms_asic" class GrubBootloader(OnieInstallerBootloader): @@ -89,12 +89,12 @@ def platform_in_devices_list(self, platform, image_path): """ For those images that don't have devices list builtin, check_output will raise an exception. In this case, we simply return True to make it worked compatible as before. - Otherwise, we need to check if platform is inside the supported target devices list. + Otherwise, we need to check if platform is inside the supported target platforms list. """ try: with open(os.devnull, 'w') as fnull: p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) - output = subprocess.check_output(["tar", "tf", "-", DEVICES_DIR], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) + output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_LIST, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) return platform in output except subprocess.CalledProcessError: return True From 830ab3882990322a136b023d7b147cff8dc46f7d Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Thu, 23 Sep 2021 09:45:48 +0000 Subject: [PATCH 3/7] Use pass instead of return True directly --- sonic_installer/bootloader/aboot.py | 5 +++-- sonic_installer/bootloader/grub.py | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/sonic_installer/bootloader/aboot.py b/sonic_installer/bootloader/aboot.py index 3c5d4dc273..5e482b8441 100644 --- a/sonic_installer/bootloader/aboot.py +++ b/sonic_installer/bootloader/aboot.py @@ -176,10 +176,11 @@ def verify_image_platform(self, image_path): # supported target platforms list. try: output = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], text=True) + return platform in output except subprocess.CalledProcessError: - return True + pass - return platform in output + return True def verify_secureboot_image(self, image_path): try: diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index 7a1537785d..f06df0677e 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -97,7 +97,9 @@ def platform_in_devices_list(self, platform, image_path): output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_LIST, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) return platform in output except subprocess.CalledProcessError: - return True + pass + + return True def verify_image_platform(self, image_path): if not os.path.isfile(image_path): From bc483c0632bb29209cc7b7bcc2388ab0c898a4a3 Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Sun, 26 Sep 2021 02:26:06 +0000 Subject: [PATCH 4/7] Change naming of platforms list --- sonic_installer/bootloader/grub.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index f06df0677e..39ab4bcb1f 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -18,7 +18,7 @@ from .onie import OnieInstallerBootloader from .onie import default_sigpipe -PLATFORMS_LIST = "installer/devices/platforms_asic" +PLATFORMS_ASIC = "installer/devices/platforms_asic" class GrubBootloader(OnieInstallerBootloader): @@ -85,7 +85,7 @@ def remove_image(self, image): run_command('grub-set-default --boot-directory=' + HOST_PATH + ' 0') click.echo('Image removed') - def platform_in_devices_list(self, platform, image_path): + def platform_in_platforms_asic(self, platform, image_path): """ For those images that don't have devices list builtin, check_output will raise an exception. In this case, we simply return True to make it worked compatible as before. @@ -94,7 +94,7 @@ def platform_in_devices_list(self, platform, image_path): try: with open(os.devnull, 'w') as fnull: p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) - output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_LIST, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) + output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) return platform in output except subprocess.CalledProcessError: pass @@ -109,7 +109,7 @@ def verify_image_platform(self, image_path): platform = device_info.get_platform() # Check if platform is inside image's target platforms - return self.platform_in_devices_list(platform, image_path) + return self.platform_in_platforms_asic(platform, image_path) @classmethod def detect(cls): From 5fbb34cbd939b77b90c5afb170d27c67d03f86a0 Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Wed, 29 Sep 2021 06:54:55 +0000 Subject: [PATCH 5/7] Use Popen instead of check_output to save memory --- sonic_installer/bootloader/aboot.py | 24 ++++++++++++++++-------- sonic_installer/bootloader/grub.py | 27 +++++++++++++++------------ sonic_installer/bootloader/onie.py | 7 +------ sonic_installer/common.py | 7 +++++++ sonic_installer/main.py | 2 +- 5 files changed, 40 insertions(+), 27 deletions(-) diff --git a/sonic_installer/bootloader/aboot.py b/sonic_installer/bootloader/aboot.py index 5e482b8441..f4882c3b46 100644 --- a/sonic_installer/bootloader/aboot.py +++ b/sonic_installer/bootloader/aboot.py @@ -23,6 +23,7 @@ ROOTFS_NAME, run_command, run_command_or_raise, + default_sigpipe, ) from .bootloader import Bootloader @@ -30,6 +31,8 @@ DEFAULT_SWI_IMAGE = 'sonic.swi' KERNEL_CMDLINE_NAME = 'kernel-cmdline' +UNZIP_MISSING_FILE = 11 + # For the signature format, see: https://github.com/aristanetworks/swi-tools/tree/master/switools SWI_SIG_FILE_NAME = 'swi-signature' SWIX_SIG_FILE_NAME = 'swix-signature' @@ -171,16 +174,21 @@ def verify_image_platform(self, image_path): # Get running platform platform = device_info.get_platform() - # If .platforms_asic is not existed, we simply return True for backward - # compatibility. Otherwise, we check if current platform is inside the + # If .platforms_asic is not existed, unzip will return code 11. + # Return True for backward compatibility. + # Otherwise, we grep to see if current platform is inside the # supported target platforms list. - try: - output = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], text=True) - return platform in output - except subprocess.CalledProcessError: - pass + with open(os.devnull, 'w') as fnull: + p1 = subprocess.Popen(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], stdout=subprocess.PIPE, stderr=fnull, preexec_fn=default_sigpipe) + p2 = subprocess.Popen(['grep', '-Fxq', '-m 1', platform], stdin=p1.stdout, preexec_fn=default_sigpipe) - return True + p1.wait() + if p1.returncode == UNZIP_MISSING_FILE: + return True + + # Code 0 is returned by grep as a result of found + p2.wait() + return p2.returncode == 0 def verify_secureboot_image(self, image_path): try: diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index 39ab4bcb1f..dd0fd19fb4 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -14,9 +14,9 @@ IMAGE_DIR_PREFIX, IMAGE_PREFIX, run_command, + default_sigpipe, ) from .onie import OnieInstallerBootloader -from .onie import default_sigpipe PLATFORMS_ASIC = "installer/devices/platforms_asic" @@ -87,19 +87,22 @@ def remove_image(self, image): def platform_in_platforms_asic(self, platform, image_path): """ - For those images that don't have devices list builtin, check_output will raise an exception. + For those images that don't have devices list builtin, 'tar' will have non-zero returncode. In this case, we simply return True to make it worked compatible as before. - Otherwise, we need to check if platform is inside the supported target platforms list. + Otherwise, we can grep to check if platform is inside the supported target platforms list. """ - try: - with open(os.devnull, 'w') as fnull: - p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) - output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True) - return platform in output - except subprocess.CalledProcessError: - pass - - return True + with open(os.devnull, 'w') as fnull: + p1 = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe) + p2 = subprocess.Popen(["tar", "xf", "-", PLATFORMS_ASIC, "-O"], stdin=p1.stdout, stdout=subprocess.PIPE, stderr=fnull, preexec_fn=default_sigpipe) + p3 = subprocess.Popen(["grep", "-Fxq", "-m 1", platform], stdin=p2.stdout, preexec_fn=default_sigpipe) + + p2.wait() + if p2.returncode != 0: + return True + + # Code 0 is returned by grep as a result of found + p3.wait() + return p3.returncode ==0 def verify_image_platform(self, image_path): if not os.path.isfile(image_path): diff --git a/sonic_installer/bootloader/onie.py b/sonic_installer/bootloader/onie.py index aa23c347a2..be17ba5619 100644 --- a/sonic_installer/bootloader/onie.py +++ b/sonic_installer/bootloader/onie.py @@ -4,20 +4,15 @@ import os import re -import signal import subprocess from ..common import ( IMAGE_DIR_PREFIX, IMAGE_PREFIX, + default_sigpipe, ) from .bootloader import Bootloader -# Needed to prevent "broken pipe" error messages when piping -# output of multiple commands using subprocess.Popen() -def default_sigpipe(): - signal.signal(signal.SIGPIPE, signal.SIG_DFL) - class OnieInstallerBootloader(Bootloader): # pylint: disable=abstract-method DEFAULT_IMAGE_PATH = '/tmp/sonic_image' diff --git a/sonic_installer/common.py b/sonic_installer/common.py index 5e36cedb8c..685063587c 100644 --- a/sonic_installer/common.py +++ b/sonic_installer/common.py @@ -5,6 +5,7 @@ import subprocess import sys +import signal import click @@ -41,3 +42,9 @@ def run_command_or_raise(argv, raise_exception=True): raise SonicRuntimeException("Failed to run command '{0}'".format(argv)) return out.rstrip("\n") + +# Needed to prevent "broken pipe" error messages when piping +# output of multiple commands using subprocess.Popen() +def default_sigpipe(): + signal.signal(signal.SIGPIPE, signal.SIG_DFL) + diff --git a/sonic_installer/main.py b/sonic_installer/main.py index 4add57fe44..1aaec8054e 100644 --- a/sonic_installer/main.py +++ b/sonic_installer/main.py @@ -541,7 +541,7 @@ def install(url, force, skip_platform_check=False, skip_migration=False, skip_pa # Verify that the binary image is of the same platform type as running platform if not skip_platform_check and not bootloader.verify_image_platform(image_path): - echo_and_log("Image file '{}' is of a different platform type than running platform.\n".format(url) + + echo_and_log("Image file '{}' is of a different platform ASIC type than running platform's.\n".format(url) + "If you are sure you want to install this image, use --skip-platform-check.\n" + "Aborting...", LOG_ERR) raise click.Abort() From 908c742490c007e4f91c095135519129b790847d Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Fri, 8 Oct 2021 03:12:22 +0000 Subject: [PATCH 6/7] Cover wait() to be safer --- sonic_installer/bootloader/aboot.py | 12 ++++++------ sonic_installer/bootloader/grub.py | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/sonic_installer/bootloader/aboot.py b/sonic_installer/bootloader/aboot.py index f4882c3b46..ab4c0ff38c 100644 --- a/sonic_installer/bootloader/aboot.py +++ b/sonic_installer/bootloader/aboot.py @@ -182,13 +182,13 @@ def verify_image_platform(self, image_path): p1 = subprocess.Popen(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], stdout=subprocess.PIPE, stderr=fnull, preexec_fn=default_sigpipe) p2 = subprocess.Popen(['grep', '-Fxq', '-m 1', platform], stdin=p1.stdout, preexec_fn=default_sigpipe) - p1.wait() - if p1.returncode == UNZIP_MISSING_FILE: - return True + p1.wait() + if p1.returncode == UNZIP_MISSING_FILE: + return True - # Code 0 is returned by grep as a result of found - p2.wait() - return p2.returncode == 0 + # Code 0 is returned by grep as a result of found + p2.wait() + return p2.returncode == 0 def verify_secureboot_image(self, image_path): try: diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index dd0fd19fb4..ac24056a56 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -96,13 +96,13 @@ def platform_in_platforms_asic(self, platform, image_path): p2 = subprocess.Popen(["tar", "xf", "-", PLATFORMS_ASIC, "-O"], stdin=p1.stdout, stdout=subprocess.PIPE, stderr=fnull, preexec_fn=default_sigpipe) p3 = subprocess.Popen(["grep", "-Fxq", "-m 1", platform], stdin=p2.stdout, preexec_fn=default_sigpipe) - p2.wait() - if p2.returncode != 0: - return True + p2.wait() + if p2.returncode != 0: + return True - # Code 0 is returned by grep as a result of found - p3.wait() - return p3.returncode ==0 + # Code 0 is returned by grep as a result of found + p3.wait() + return p3.returncode ==0 def verify_image_platform(self, image_path): if not os.path.isfile(image_path): From 383f826f3a0b0dc138d4f5a8d252dbcf116adbb6 Mon Sep 17 00:00:00 2001 From: Jingwen Xie Date: Wed, 13 Oct 2021 02:19:36 +0000 Subject: [PATCH 7/7] platfroms dir change --- sonic_installer/bootloader/grub.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic_installer/bootloader/grub.py b/sonic_installer/bootloader/grub.py index ac24056a56..11ee3de1f4 100644 --- a/sonic_installer/bootloader/grub.py +++ b/sonic_installer/bootloader/grub.py @@ -18,7 +18,7 @@ ) from .onie import OnieInstallerBootloader -PLATFORMS_ASIC = "installer/devices/platforms_asic" +PLATFORMS_ASIC = "installer/platforms_asic" class GrubBootloader(OnieInstallerBootloader):