From e780aba87c7628bae5e0952071f8772abc294844 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Thu, 24 Jul 2025 15:33:58 +0100 Subject: [PATCH 1/9] bootloader: Parse baud as int When reading a GRUB2 config, parse the baud rate as an int for consistency with the other bootloader types. Signed-off-by: Ross Lagerwall --- xcp/bootloader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcp/bootloader.py b/xcp/bootloader.py index 2cadd5c4..bb7e160e 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -371,7 +371,7 @@ def parse_boot_entry(line): if match: serial = { 'port': int(match.group(1)), - 'baud': match.group(2) + 'baud': int(match.group(2)), } elif l.startswith('terminal_'): pass From 77c23d06291ae49b0407efebe8008915635178cb Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 22 Jul 2025 10:43:18 +0100 Subject: [PATCH 2/9] bootloader: Remove extlinux and GRUB1 code extlinux and GRUB1 are no longer used for installed systems so remove the code that handles them. Signed-off-by: Ross Lagerwall --- tests/test_bootloader.py | 37 +---- xcp/bootloader.py | 282 +-------------------------------------- 2 files changed, 5 insertions(+), 314 deletions(-) diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index 82500336..fa1ba797 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -8,28 +8,6 @@ from xcp.compat import open_with_codec_handling -def test_writeGrubWithTempFile(tmpdir): - """Test xcp.bootloader.{writeGrub,writeExtLinux}.open_with_codec_handling with tmpdir fixture""" - bootloader = Bootloader.readGrub2("tests/data/grub.cfg") - filename = str(tmpdir.mkdir("grub").join("menu.lst")) - bootloader.writeGrub(filename) - Bootloader.readGrub(filename) - bootloader.writeExtLinux(filename) - Bootloader.readExtLinux(filename) - - -def test_GrubLegacyExtLinuxWithTempFile(tmpdir): - """Note: GRUB-Legacy and EXTLINUX code can likely be removed (pending approval)""" - bootloader = Bootloader.readGrub2("tests/data/grub-linux.cfg") - filename = str(tmpdir.mkdir("grub").join("menu.lst")) - bootloader.writeExtLinux(filename) - Bootloader.readExtLinux(filename) - bootloader.menu.pop("xe-tboot") - bootloader.menu_order.remove("xe-tboot") - bootloader.writeGrub(filename) - Bootloader.readGrub(filename) - - class TestBootloader(unittest.TestCase): def test_grub2(self): bl = Bootloader.readGrub2("tests/data/grub.cfg") @@ -100,24 +78,15 @@ def setUp(self): self.bl = Bootloader.readGrub2("tests/data/grub.cfg") check_config(self.bl) - def test_grub(self): + def test_grub2(self): with NamedTemporaryFile("w", delete=False) as temp: - self.bl.writeGrub(temp) - bl2 = Bootloader.readGrub(temp.name) + self.bl.writeGrub2(temp) + bl2 = Bootloader.readGrub2(temp.name) # Check config from tests/data/grub.cfg: os.unlink(temp.name) assert bl2.serial == {"port": 0, "baud": 115200} check_config(bl2) - def test_extlinux(self): - with NamedTemporaryFile("w", delete=False) as temp: - self.bl.writeExtLinux(temp) - bl2 = Bootloader.readExtLinux(temp.name) - os.unlink(temp.name) - # readExtLinux tries to read flow-control (there is none in tests/data/grub.cfg): - assert bl2.serial == {"port": 0, "baud": 115200, "flow": None} - check_config(bl2) - def check_config(bl): # Check config from tests/data/grub.cfg: diff --git a/xcp/bootloader.py b/xcp/bootloader.py index bb7e160e..6b8967ed 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -106,200 +106,6 @@ def remove(self, label): del self.menu[label] self.menu_order.remove(label) - @classmethod - def readExtLinux(cls, src_file): - # type:(str) -> Bootloader - menu = {} # type: dict[str, MenuEntry | dict[str, str]] - menu_order = [] - default = None - timeout = None - location = None - serial = None - label = None - title = None - kernel = None - - fh = open_textfile(src_file, "r") - try: - for line in fh: - l = line.strip() - els = l.split(None, 2) - if len(els) == 0: - continue - keywrd = els[0].lower() - - # header - if l.startswith('# location ') and len(els) == 3 and els[2] in ['mbr', 'partition']: - location = els[2] - elif keywrd == 'serial' and len(els) > 1: - baud = '9600' - flow = None - if len(els) > 2: - if ' ' in els[2]: - baud, flow = els[2].split(None, 1) - else: - baud = els[2] - serial = {'port': int(els[1]), 'baud': int(baud), 'flow': flow} - elif keywrd == 'default' and len(els) == 2: - default = els[1] - elif keywrd == 'timeout' and len(els) == 2: - timeout = int(els[1]) - - # menu - elif keywrd == 'label' and len(els) == 2: - label = els[1] - menu[label] = {} - menu_order.append(label) - title = None - elif label: - if keywrd == '#': - title = l[1:].lstrip() - elif keywrd == 'kernel' and len(els) > 1: - kernel = els[1] - elif keywrd == 'append' and len(els) > 1 and kernel == 'mboot.c32': - if 'tboot' in els[1]: - # els[2] contains tboot args, hypervisor, - # hypervisor args, kernel, - # kernel args & initrd - args = [x.strip() for x in els[2].split('---')] - if len(args) == 4: - hypervisor = args[1].split(None, 1) - kernel = args[2].split(None, 1) # type:ignore[assignment] # mypy - if len(hypervisor) == 2 and len(kernel) == 2: - menu[label] = MenuEntry(tboot = els[1], - tboot_args = args[0], - hypervisor = hypervisor[0], - hypervisor_args = hypervisor[1], - kernel = kernel[0], - kernel_args = kernel[1], - initrd = args[3], - title = title) - elif 'xen' in els[1]: - # els[2] contains hypervisor args, kernel, - # kernel args & initrd - args = [x.strip() for x in els[2].split('---')] - if len(args) == 3: - kernel = args[1].split(None, 1) # type:ignore[assignment] # mypy - if len(kernel) == 2: - menu[label] = MenuEntry(hypervisor = els[1], - hypervisor_args = args[0], - kernel = kernel[0], - kernel_args = kernel[1], - initrd = args[2], - title = title) - finally: - fh.close() - - return cls('extlinux', src_file, menu, menu_order, default, timeout, - serial, location) - - @classmethod - def readGrub(cls, src_file): - # type: (str) -> Bootloader - menu = {} - menu_order = [] - default = 0 - timeout = None - location = None - serial = None - label = None - title = None - hypervisor = None - hypervisor_args = None - kernel = None - kernel_args = None - - def create_label(title): - global COUNTER - - if title == branding.PRODUCT_BRAND: - return 'xe' - - if title.endswith('(Serial)'): - return 'xe-serial' - if title.endswith('Safe Mode'): - return 'safe' - if ' / ' in title: - if '(Serial,' in title: - return 'fallback-serial' - else: - return 'fallback' - COUNTER += 1 - return "label%d" % COUNTER - - fh = open_textfile(src_file, "r") - try: - for line in fh: - l = line.strip() - els = l.split(None, 2) - if len(els) == 0: - continue - - # header - if l.startswith('# location ') and len(els) == 3 and els[2] in ['mbr', 'partition']: - location = els[2] - elif els[0] == 'serial' and len(els) > 1: - port = 0 - baud = 9600 - for arg in l.split(None, 1)[1].split(): - if '=' in arg: - opt, val = arg.split('=') - if opt == '--unit': - port = int(val) - elif opt == '--speed': - baud = int(val) - serial = {'port': port, 'baud': baud} - elif els[0] == 'default' and len(els) == 2: - # default is index into menu list, fixup later - default = int(els[1]) - elif els[0] == 'timeout' and len(els) == 2: - timeout = int(els[1]) * 10 - - # menu - elif els[0] == 'title' and len(els) > 1: - title = l.split(None, 1)[1] - elif title: - if els[0] == 'kernel' and len(els) > 2: - hypervisor, hypervisor_args = (l.split(None, 1) - [1].split(None, 1)) - elif els[0] == 'module' and len(els) > 1: - if kernel and hypervisor: - # second module == initrd - label = create_label(title) - menu_order.append(label) - menu[label] = MenuEntry(hypervisor = hypervisor, - hypervisor_args = hypervisor_args, - kernel = kernel, - kernel_args = kernel_args, - initrd = els[1], title = title) - hypervisor = None - kernel = None - else: - kernel, kernel_args = (l.split(None, 1) - [1].split(None, 1)) - elif els[0] == 'initrd' and len(els) > 1: - # not multiboot - kernel = hypervisor - kernel_args = hypervisor_args - label = create_label(title) - menu_order.append(label) - menu[label] = MenuEntry(None, - None, - kernel = kernel, - kernel_args = kernel_args, - initrd = els[1], title = title) - hypervisor = None - hypervisor_args = None - - # fixup default - if len(menu_order) > default: - default = menu_order[default] - finally: - fh.close() - - return cls('grub', src_file, menu, menu_order, default, - timeout, serial, location) - @classmethod def readGrub2(cls, src_file): # type:(str) -> Bootloader @@ -472,89 +278,9 @@ def loadExisting(cls, root = '/'): return cls.readGrub2(os.path.join(root, "boot/grub/grub.cfg")) elif os.path.exists(os.path.join(root, "boot/grub2/grub.cfg")): return cls.readGrub2(os.path.join(root, "boot/grub2/grub.cfg")) - elif os.path.exists(os.path.join(root, "boot/extlinux.conf")): - return cls.readExtLinux(os.path.join(root, "boot/extlinux.conf")) - elif os.path.exists(os.path.join(root, "boot/grub/menu.lst")): - return cls.readGrub(os.path.join(root, "boot/grub/menu.lst")) else: raise RuntimeError("No existing bootloader configuration found") - def writeExtLinux(self, dst_file = None): - if dst_file and hasattr(dst_file, 'name'): - fh = dst_file - else: - fh = open_textfile(cast(str, dst_file), "w") - print("# location " + self.location, file=fh) - - if self.serial: - if self.serial.get("flow", None) is None: - print("serial %s %s" % (self.serial['port'], - self.serial['baud']), file=fh) - else: - print("serial %s %s %s" % (self.serial['port'], - self.serial['baud'], - self.serial['flow']), file=fh) - if self.default: - print("default " + self.default, file=fh) - print("prompt 1", file=fh) - if self.timeout: - print("timeout %d" % self.timeout, file=fh) - - for label in self.menu_order: - print("\nlabel " + label, file=fh) - m = self.menu[label] - if m.title: - print(" # " + m.title, file=fh) - if m.tboot: - print(" kernel mboot.c32", file=fh) - print(" append %s %s --- %s %s --- %s %s --- %s" % - (m.tboot, m.tboot_args, m.hypervisor, m.hypervisor_args, - m.kernel, m.kernel_args, m.initrd), file=fh) - elif m.hypervisor: - print(" kernel mboot.c32", file=fh) - print(" append %s %s --- %s %s --- %s" % - (m.hypervisor, m.hypervisor_args, m.kernel, m.kernel_args, m.initrd), file=fh) - else: - print(" kernel " + m.kernel, file=fh) - print(" append " + m.kernel_args, file=fh) - print(" initrd " + m.initrd, file=fh) - if not hasattr(dst_file, 'name'): - fh.close() - - def writeGrub(self, dst_file = None): - if dst_file and hasattr(dst_file, 'name'): - fh = dst_file - else: - fh = open_textfile(cast(str, dst_file), "w") - print("# location " + self.location, file=fh) - - if self.serial: - print("serial --unit=%s --speed=%s" % - (self.serial['port'], self.serial['baud']), file=fh) - print("terminal --timeout=10 console serial", file=fh) - else: - print("terminal console", file=fh) - if self.default: - for i in range(len(self.menu_order)): - if self.menu_order[i] == self.default: - print("default %d" % i, file=fh) - break - if self.timeout: - print("timeout %d" % (self.timeout // 10), file=fh) - - for label in self.menu_order: - m = self.menu[label] - print("\ntitle " + m.title, file=fh) - if m.hypervisor: - print(" kernel " + m.hypervisor + " " + m.hypervisor_args, file=fh) - print(" module " + m.kernel + " " + m.kernel_args, file=fh) - print(" module " + m.initrd, file=fh) - else: - print(" kernel " + m.kernel + " " + m.kernel_args, file=fh) - print(" initrd " + m.initrd, file=fh) - if not hasattr(dst_file, 'name'): - fh.close() - def writeGrub2(self, dst_file = None): if dst_file and hasattr(dst_file, 'name'): fh = dst_file @@ -626,12 +352,8 @@ def commit(self, dst_file = None): # write to temp file in final destination directory fd, tmp_file = tempfile.mkstemp(dir = os.path.dirname(dst_file)) - if self.src_fmt == 'extlinux': - self.writeExtLinux(tmp_file) - elif self.src_fmt == 'grub': - self.writeGrub(tmp_file) - elif self.src_fmt == 'grub2': - self.writeGrub2(tmp_file) + assert self.src_fmt == 'grub2' + self.writeGrub2(tmp_file) # atomically replace destination file os.close(fd) From aae5681fe69c5e31cd376ce8ca556b1b4e92325a Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 22 Jul 2025 10:56:47 +0100 Subject: [PATCH 3/9] bootloader: Remove tboot support tboot is no longer supported so remove the code that handles it. Signed-off-by: Ross Lagerwall --- tests/data/grub-linux.cfg | 7 ------- tests/test_bootloader.py | 5 ----- xcp/bootloader.py | 35 +++++------------------------------ 3 files changed, 5 insertions(+), 42 deletions(-) diff --git a/tests/data/grub-linux.cfg b/tests/data/grub-linux.cfg index ae7a104f..bb0372b9 100644 --- a/tests/data/grub-linux.cfg +++ b/tests/data/grub-linux.cfg @@ -3,10 +3,3 @@ menuentry 'Linux - Safe Mode' { linux /boot/vmlinuz-1 ro initrd /boot/initrd.img-1 } -menuentry 'XCP-ng (Trusted Boot)' { - search --label --set root root-vgdorj - multiboot2 /boot/tboot.gz logging=serial,memory - module2 /boot/xen.gz dom0_mem=7584M,max:7584M watchdog crashkernel=256M,below=4G - module2 /boot/vmlinuz-6.1-xen root=LABEL=root-vgdorj ro console=hvc0 console=tty0 - module2 /boot/initrd-6.1-xen.img -} \ No newline at end of file diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index fa1ba797..6f9daafc 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -58,7 +58,6 @@ def test_grub2_newdefault(self): "", ], [], - [], ] assert str(bl.default).startswith("safe") assert bl.location == "mbr" @@ -68,10 +67,6 @@ def test_grub2_newdefault(self): assert bl.menu["safe"].kernel == "/boot/vmlinuz-2" assert bl.menu["safe"].kernel_args == "ro" assert bl.menu["safe"].initrd == "/boot/initrd.img-2" - assert bl.menu["xe-tboot"].title == "XCP-ng (Trusted Boot)" - assert bl.menu["xe-tboot"].hypervisor == "/boot/xen.gz" - assert bl.menu["xe-tboot"].tboot == "/boot/tboot.gz" - assert bl.menu["xe-tboot"].getTbootArgs() == ["logging=serial,memory"] class TestBootloaderAdHoc(unittest.TestCase): def setUp(self): diff --git a/xcp/bootloader.py b/xcp/bootloader.py index 6b8967ed..6f3e249e 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -43,13 +43,11 @@ COUNTER = 0 class MenuEntry(object): + # pylint: disable=too-many-positional-arguments def __init__(self, hypervisor, hypervisor_args, kernel, kernel_args, - initrd, title = None, tboot = None, tboot_args = None, - root = None): + initrd, title = None, root = None): self.extra = None self.contents = [] - self.tboot = tboot - self.tboot_args = tboot_args self.hypervisor = hypervisor self.hypervisor_args = hypervisor_args self.kernel = kernel @@ -58,12 +56,6 @@ def __init__(self, hypervisor, hypervisor_args, kernel, kernel_args, self.title = title self.root = root - def getTbootArgs(self): - return re.findall(r'\S[^ "]*(?:"[^"]*")?\S*', cast(str, self.tboot_args)) - - def setTbootArgs(self, args): - self.tboot_args = ' '.join(args) - def getHypervisorArgs(self): return re.findall(r'\S[^ "]*(?:"[^"]*")?\S*', self.hypervisor_args) @@ -115,8 +107,6 @@ def readGrub2(cls, src_file): timeout = None serial = None title = None - tboot = None - tboot_args = None hypervisor = None hypervisor_args = None kernel = None @@ -133,10 +123,6 @@ def create_label(title): if title == branding.PRODUCT_BRAND: return 'xe' - if title.endswith('(Serial) (Trusted Boot)'): - return 'xe-serial-tboot' - if title.endswith('(Trusted Boot)'): - return 'xe-tboot' if title.endswith('(Serial)'): return 'xe-serial' if title.endswith('Safe Mode'): @@ -206,10 +192,7 @@ def parse_boot_entry(line): boilerplate = [] elif title: if l.startswith("multiboot2"): - if "tboot" in l: - tboot, tboot_args = parse_boot_entry(l) - else: - hypervisor, hypervisor_args = parse_boot_entry(l) + hypervisor, hypervisor_args = parse_boot_entry(l) elif l.startswith("module2"): if not hypervisor: hypervisor, hypervisor_args = parse_boot_entry(l) @@ -226,9 +209,7 @@ def parse_boot_entry(line): elif l == "}": label = create_label(title) menu_order.append(label) - menu[label] = MenuEntry(tboot = tboot, - tboot_args = tboot_args, - hypervisor = hypervisor, + menu[label] = MenuEntry(hypervisor = hypervisor, hypervisor_args = hypervisor_args, kernel = kernel, kernel_args = kernel_args, @@ -238,8 +219,6 @@ def parse_boot_entry(line): menu[label].contents = menu_entry_contents title = None - tboot = None - tboot_args = None hypervisor = None hypervisor_args = None kernel = None @@ -327,11 +306,7 @@ def writeGrub2(self, dst_file = None): print("\tsearch --label --set root %s" % m.root, file=fh) if m.hypervisor: - if m.tboot: - print("\tmultiboot2 %s %s" % (m.tboot, m.tboot_args), file=fh) - print("\tmodule2 %s %s" % (m.hypervisor, m.hypervisor_args), file=fh) - else: - print("\tmultiboot2 %s %s" % (m.hypervisor, m.hypervisor_args), file=fh) + print("\tmultiboot2 %s %s" % (m.hypervisor, m.hypervisor_args), file=fh) if m.kernel: print("\tmodule2 %s %s" % (m.kernel, m.kernel_args), file=fh) if m.initrd: From c9e6c901a8ef9f6b41545fd74233eb6837a2d3de Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 22 Jul 2025 11:08:07 +0100 Subject: [PATCH 4/9] bootloader: Reject module2 without multiboot2 GRUB2 will fail if module2 is used without a previous call to multiboot2 so raise an exception instead of pretending that it is actually a hypervisor. Signed-off-by: Ross Lagerwall --- tests/data/grub-no-multiboot.cfg | 10 ++++++++++ tests/test_bootloader.py | 5 +++++ xcp/bootloader.py | 4 ++-- 3 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 tests/data/grub-no-multiboot.cfg diff --git a/tests/data/grub-no-multiboot.cfg b/tests/data/grub-no-multiboot.cfg new file mode 100644 index 00000000..afac3d18 --- /dev/null +++ b/tests/data/grub-no-multiboot.cfg @@ -0,0 +1,10 @@ +serial --unit=0 --speed=115200 +terminal_input serial console +terminal_output serial console +set default=0 +set timeout=5 +menuentry 'XCP-ng' { + search --label --set root root-vgdorj + module2 /boot/vmlinuz-4.19-xen root=LABEL=root-vgdorj ro nolvm hpet=disable console=hvc0 console=tty0 quiet vga=785 splash plymouth.ignore-serial-consoles + module2 /boot/initrd-4.19-xen.img +} diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index 6f9daafc..9519cc1a 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -25,6 +25,11 @@ def test_grub2(self): proc.stdout.close() proc.wait() + def test_no_multiboot(self): + # A module2 line without a multiboot2 line is an error + with self.assertRaises(RuntimeError): + Bootloader.readGrub2("tests/data/grub-no-multiboot.cfg") + class TestLinuxBootloader(unittest.TestCase): def setUp(self): self.tmpdir = mkdtemp(prefix="testbl") diff --git a/xcp/bootloader.py b/xcp/bootloader.py index 6f3e249e..3f214e03 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -195,8 +195,8 @@ def parse_boot_entry(line): hypervisor, hypervisor_args = parse_boot_entry(l) elif l.startswith("module2"): if not hypervisor: - hypervisor, hypervisor_args = parse_boot_entry(l) - elif kernel: + raise RuntimeError("Need a multiboot2 kernel") + if kernel: initrd = l.split(None, 1)[1] else: kernel, kernel_args = parse_boot_entry(l) From 6b9678495cd660855eff96f086227266af33b4c0 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 22 Jul 2025 11:12:36 +0100 Subject: [PATCH 5/9] bootloader: Reject initrd without kernel Fail when parsing a menu entry that specifies an initrd without a preceding kernel since it is invalid according to GRUB2. Signed-off-by: Ross Lagerwall --- tests/data/grub-linux-no-kernel.cfg | 3 +++ tests/test_bootloader.py | 7 ++++++- xcp/bootloader.py | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 tests/data/grub-linux-no-kernel.cfg diff --git a/tests/data/grub-linux-no-kernel.cfg b/tests/data/grub-linux-no-kernel.cfg new file mode 100644 index 00000000..6f0a6df2 --- /dev/null +++ b/tests/data/grub-linux-no-kernel.cfg @@ -0,0 +1,3 @@ +menuentry 'Linux - Safe Mode' { + initrd /boot/initrd.img-1 +} diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index 9519cc1a..b338a0de 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -29,7 +29,6 @@ def test_no_multiboot(self): # A module2 line without a multiboot2 line is an error with self.assertRaises(RuntimeError): Bootloader.readGrub2("tests/data/grub-no-multiboot.cfg") - class TestLinuxBootloader(unittest.TestCase): def setUp(self): self.tmpdir = mkdtemp(prefix="testbl") @@ -73,6 +72,12 @@ def test_grub2_newdefault(self): assert bl.menu["safe"].kernel_args == "ro" assert bl.menu["safe"].initrd == "/boot/initrd.img-2" + def test_no_kernel(self): + # An initrd line without a kernel line is an error + with self.assertRaises(RuntimeError): + Bootloader.readGrub2("tests/data/grub-linux-no-kernel.cfg") + + class TestBootloaderAdHoc(unittest.TestCase): def setUp(self): self.bl = Bootloader.readGrub2("tests/data/grub.cfg") diff --git a/xcp/bootloader.py b/xcp/bootloader.py index 3f214e03..a6392346 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -203,6 +203,8 @@ def parse_boot_entry(line): elif l.startswith("linux"): kernel, kernel_args = parse_boot_entry(l) elif l.startswith("initrd"): + if not kernel: + raise RuntimeError("Need a kernel") initrd = l.split(None, 1)[1] elif l.startswith("search --label --set root"): root = l.split()[4] From 21e9fb978dc6b52dec06024838d861fafb33397c Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 22 Jul 2025 11:22:56 +0100 Subject: [PATCH 6/9] CP-308428: bootloader: Add support for xen_boot entry type A GRUB2 menu entry may boot different OSes using different loaders. Add a new menu entry parameter to allow specifying the entry type: 1) multiboot2 - used for booting xen.gz 2) linux - used for booting native Linux 3) xen_boot - used for booting xen.efi, newly introduced here The entry type can be specified when first creating the menu entry (defaulting to multiboot2) and it is subsequently maintained when reading/writing the config file. Signed-off-by: Ross Lagerwall --- tests/test_bootloader.py | 89 +++++++++++++++++++++++++++++++++++++++- xcp/bootloader.py | 39 ++++++++++++++++-- 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index b338a0de..e33bd10c 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -4,7 +4,7 @@ import subprocess from tempfile import NamedTemporaryFile, mkdtemp -from xcp.bootloader import Bootloader +from xcp.bootloader import Bootloader, Grub2Format, MenuEntry from xcp.compat import open_with_codec_handling @@ -29,6 +29,93 @@ def test_no_multiboot(self): # A module2 line without a multiboot2 line is an error with self.assertRaises(RuntimeError): Bootloader.readGrub2("tests/data/grub-no-multiboot.cfg") + + +class TestMenuEntry(unittest.TestCase): + def setUp(self): + self.tmpdir = mkdtemp(prefix="testbl") + self.fn = os.path.join(self.tmpdir, 'grub.cfg') + self.bl = Bootloader('grub2', self.fn) + + def tearDown(self): + shutil.rmtree(self.tmpdir) + + def test_new_multiboot(self): + # No format specified, default to multiboot2 + e = MenuEntry(hypervisor='xen.efi', hypervisor_args='xarg1 xarg2', + kernel='vmlinuz', kernel_args='karg1 karg2', + initrd='initrd.img', title='xe') + self.bl.append('xe', e) + + e = MenuEntry(hypervisor='xen.efi', hypervisor_args='xarg1 xarg2', + kernel='vmlinuz', kernel_args='karg1 karg2', + initrd='initrd.img', title='xe-serial') + e.entry_format = Grub2Format.MULTIBOOT2 + self.bl.append('xe-serial', e) + + self.bl.commit() + + with open_with_codec_handling(self.fn, 'r') as f: + content = f.read() + + self.assertEqual(content, '''menuentry 'xe' { + multiboot2 xen.efi xarg1 xarg2 + module2 vmlinuz karg1 karg2 + module2 initrd.img +} +menuentry 'xe-serial' { + multiboot2 xen.efi xarg1 xarg2 + module2 vmlinuz karg1 karg2 + module2 initrd.img +} +''') + + def test_new_xen_boot(self): + e = MenuEntry(hypervisor='xen.efi', hypervisor_args='xarg1 xarg2', + kernel='vmlinuz', kernel_args='karg1 karg2', + initrd='initrd.img', title='xe') + e.entry_format = Grub2Format.XEN_BOOT + self.bl.append('xe', e) + self.bl.commit() + + with open_with_codec_handling(self.fn, 'r') as f: + content = f.read() + + self.assertEqual(content, '''menuentry 'xe' { + xen_hypervisor xen.efi xarg1 xarg2 + xen_module vmlinuz karg1 karg2 + xen_module initrd.img +} +''') + + def test_new_linux(self): + e = MenuEntry(hypervisor='', hypervisor_args='', + kernel='vmlinuz', kernel_args='karg1 karg2', + initrd='initrd.img', title='linux') + self.bl.append('linux', e) + self.bl.commit() + + e = MenuEntry(hypervisor='', hypervisor_args='', + kernel='vmlinuz2', kernel_args='karg3 karg4', + initrd='initrd2.img', title='linux2') + e.entry_format = Grub2Format.LINUX + self.bl.append('linux2', e) + self.bl.commit() + + with open_with_codec_handling(self.fn, 'r') as f: + content = f.read() + + self.assertEqual(content, '''menuentry 'linux' { + linux vmlinuz karg1 karg2 + initrd initrd.img +} +menuentry 'linux2' { + linux vmlinuz2 karg3 karg4 + initrd initrd2.img +} +''') + + class TestLinuxBootloader(unittest.TestCase): def setUp(self): self.tmpdir = mkdtemp(prefix="testbl") diff --git a/xcp/bootloader.py b/xcp/bootloader.py index a6392346..0ba6c8e9 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -24,6 +24,7 @@ from __future__ import division, print_function import copy +from enum import Enum import os import os.path import re @@ -42,6 +43,11 @@ COUNTER = 0 +class Grub2Format(Enum): + MULTIBOOT2 = 0 + LINUX = 1 + XEN_BOOT = 2 + class MenuEntry(object): # pylint: disable=too-many-positional-arguments def __init__(self, hypervisor, hypervisor_args, kernel, kernel_args, @@ -55,6 +61,7 @@ def __init__(self, hypervisor, hypervisor_args, kernel, kernel_args, self.initrd = initrd self.title = title self.root = root + self.entry_format = None def getHypervisorArgs(self): return re.findall(r'\S[^ "]*(?:"[^"]*")?\S*', self.hypervisor_args) @@ -117,6 +124,7 @@ def readGrub2(cls, src_file): menu_entry_contents = [] # type: list[str] boilerplate = [] # type: list[str] boilerplates = [] # type: list[list[str]] + entry_format = Grub2Format.MULTIBOOT2 def create_label(title): global COUNTER @@ -193,6 +201,9 @@ def parse_boot_entry(line): elif title: if l.startswith("multiboot2"): hypervisor, hypervisor_args = parse_boot_entry(l) + elif l.startswith("xen_hypervisor"): + entry_format = Grub2Format.XEN_BOOT + hypervisor, hypervisor_args = parse_boot_entry(l) elif l.startswith("module2"): if not hypervisor: raise RuntimeError("Need a multiboot2 kernel") @@ -200,7 +211,15 @@ def parse_boot_entry(line): initrd = l.split(None, 1)[1] else: kernel, kernel_args = parse_boot_entry(l) + elif l.startswith("xen_module"): + if not hypervisor: + raise RuntimeError("Need a hypervisor") + if kernel: + initrd = l.split(None, 1)[1] + else: + kernel, kernel_args = parse_boot_entry(l) elif l.startswith("linux"): + entry_format = Grub2Format.LINUX kernel, kernel_args = parse_boot_entry(l) elif l.startswith("initrd"): if not kernel: @@ -219,6 +238,7 @@ def parse_boot_entry(line): root = root) menu[label].extra = menu_entry_extra menu[label].contents = menu_entry_contents + menu[label].entry_format = entry_format title = None hypervisor = None @@ -229,6 +249,7 @@ def parse_boot_entry(line): root = None menu_entry_extra = None menu_entry_contents = [] + entry_format = Grub2Format.MULTIBOOT2 else: menu_entry_contents.append(line.rstrip()) @@ -307,17 +328,27 @@ def writeGrub2(self, dst_file = None): if m.root: print("\tsearch --label --set root %s" % m.root, file=fh) - if m.hypervisor: + if ((m.entry_format is None and m.hypervisor) or + m.entry_format == Grub2Format.MULTIBOOT2): print("\tmultiboot2 %s %s" % (m.hypervisor, m.hypervisor_args), file=fh) if m.kernel: print("\tmodule2 %s %s" % (m.kernel, m.kernel_args), file=fh) if m.initrd: print("\tmodule2 %s" % m.initrd, file=fh) - else: - if m.kernel: - print("\tlinux %s %s" % (m.kernel, m.kernel_args), file=fh) + elif ((m.entry_format is None and not m.hypervisor) or + m.entry_format == Grub2Format.LINUX): + print("\tlinux %s %s" % (m.kernel, m.kernel_args), file=fh) if m.initrd: print("\tinitrd %s" % m.initrd, file=fh) + elif m.entry_format == Grub2Format.XEN_BOOT: + print("\txen_hypervisor %s %s" % (m.hypervisor, m.hypervisor_args), file=fh) + if m.kernel: + print("\txen_module %s %s" % (m.kernel, m.kernel_args), file=fh) + if m.initrd: + print("\txen_module %s" % m.initrd, file=fh) + else: + raise AssertionError("Unreachable") + print("}", file=fh) if not hasattr(dst_file, 'name'): fh.close() From 8fbc668e39d54bca9a3b1a9391dd83c1c940801a Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Mon, 28 Jul 2025 16:37:23 +0100 Subject: [PATCH 7/9] test_bootloader: Fix sporadic failure This test is broken in multiple ways. Either: 1) Execution of diff races against the named temporary file being removed when the context manager is left, resulting in no output from diff and the test "passes" (diff exits with status 2). 2) diff generates output and the call to assertRegexpMatches() fails because it was renamed in Python 3.2. Fix this by ensuring diff finishes before leaving the context manager, asserting the exact diff expected, and checking the return code. Signed-off-by: Ross Lagerwall --- tests/test_bootloader.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index e33bd10c..dc075d50 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -17,13 +17,20 @@ def test_grub2(self): proc = subprocess.Popen(["diff", "tests/data/grub.cfg", temp.name], stdout = subprocess.PIPE, universal_newlines=True) - assert proc.stdout - for line in proc.stdout: - # pylint: disable-next=deprecated-method - self.assertRegexpMatches(line, r"^(5a6,13$|>)") - proc.stdout.close() - proc.wait() + self.assertEqual(proc.stdout.read(), '''5a6,13 +> if [ -s $prefix/grubenv ]; then +> load_env +> fi +> +> if [ -n "$override_entry" ]; then +> set default=$override_entry +> fi +> +''') + proc.stdout.close() + proc.wait() + self.assertEqual(proc.returncode, 1) def test_no_multiboot(self): # A module2 line without a multiboot2 line is an error From 48452e68794abaf8d08aa347710a200fe6a4c5a0 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Mon, 28 Jul 2025 16:52:48 +0100 Subject: [PATCH 8/9] bootloader: Cleanup pylint complaints Ignore the warning about too many positional arguments since this is part of the existing API. Clean up trailing whitespace. Signed-off-by: Ross Lagerwall --- xcp/bootloader.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xcp/bootloader.py b/xcp/bootloader.py index 0ba6c8e9..695c3413 100644 --- a/xcp/bootloader.py +++ b/xcp/bootloader.py @@ -76,6 +76,7 @@ def setKernelArgs(self, args): self.kernel_args = ' '.join(args) class Bootloader(object): + # pylint: disable=too-many-positional-arguments def __init__(self, src_fmt, src_file, menu = None, menu_order = None, default = None, timeout = None, serial = None, location = None, env_block = None): @@ -150,7 +151,7 @@ def parse_boot_entry(line): entry = parts[1] if len(parts) > 1 else "" args = parts[2] if len(parts) > 2 else "" return entry, args - + fh = open_textfile(src_file, "r") try: for line in fh: From 5f3b037d364948be4c1db3febda52300ff8ab981 Mon Sep 17 00:00:00 2001 From: Ross Lagerwall Date: Tue, 29 Jul 2025 09:58:17 +0100 Subject: [PATCH 9/9] bootloader: Add more test coverage Signed-off-by: Ross Lagerwall --- tests/data/grub-no-hypervisor.cfg | 10 +++++++++ tests/data/grub-xen-boot.cfg | 35 +++++++++++++++++++++++++++++++ tests/test_bootloader.py | 19 ++++++++++++++--- 3 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 tests/data/grub-no-hypervisor.cfg create mode 100644 tests/data/grub-xen-boot.cfg diff --git a/tests/data/grub-no-hypervisor.cfg b/tests/data/grub-no-hypervisor.cfg new file mode 100644 index 00000000..348e8457 --- /dev/null +++ b/tests/data/grub-no-hypervisor.cfg @@ -0,0 +1,10 @@ +serial --unit=0 --speed=115200 +terminal_input serial console +terminal_output serial console +set default=0 +set timeout=5 +menuentry 'XCP-ng' { + search --label --set root root-vgdorj + xen_module /boot/vmlinuz-4.19-xen root=LABEL=root-vgdorj ro nolvm hpet=disable console=hvc0 console=tty0 quiet vga=785 splash plymouth.ignore-serial-consoles + xen_module /boot/initrd-4.19-xen.img +} diff --git a/tests/data/grub-xen-boot.cfg b/tests/data/grub-xen-boot.cfg new file mode 100644 index 00000000..88ce234f --- /dev/null +++ b/tests/data/grub-xen-boot.cfg @@ -0,0 +1,35 @@ +serial --unit=0 --speed=115200 +terminal_input serial console +terminal_output serial console +set default=0 +set timeout=5 +menuentry 'XCP-ng' { + search --label --set root root-vgdorj + xen_hypervisor /boot/xen.gz dom0_mem=7584M,max:7584M watchdog ucode=scan dom0_max_vcpus=1-16 crashkernel=256M,below=4G console=vga vga=mode-0x0311 + xen_module /boot/vmlinuz-4.19-xen root=LABEL=root-vgdorj ro nolvm hpet=disable console=hvc0 console=tty0 quiet vga=785 splash plymouth.ignore-serial-consoles + xen_module /boot/initrd-4.19-xen.img +} +menuentry 'XCP-ng (Serial)' { + search --label --set root root-vgdorj + xen_hypervisor /boot/xen.gz com1=115200,8n1 console=com1,vga dom0_mem=7584M,max:7584M watchdog ucode=scan dom0_max_vcpus=1-16 crashkernel=256M,below=4G + xen_module /boot/vmlinuz-4.19-xen root=LABEL=root-vgdorj ro nolvm hpet=disable console=tty0 console=hvc0 + xen_module /boot/initrd-4.19-xen.img +} +menuentry 'XCP-ng in Safe Mode' { + search --label --set root root-vgdorj + xen_hypervisor /boot/xen.gz nosmp noreboot noirqbalance no-mce no-bootscrub no-numa no-hap no-mmcfg max_cstate=0 nmi=ignore allow_unsafe dom0_mem=7584M,max:7584M com1=115200,8n1 console=com1,vga + xen_module /boot/vmlinuz-4.19-xen earlyprintk=xen root=LABEL=root-vgdorj ro nolvm hpet=disable console=tty0 console=hvc0 + xen_module /boot/initrd-4.19-xen.img +} +menuentry 'XCP-ng (Xen 4.13.1 / Linux 4.19.0+1)' { + search --label --set root root-vgdorj + xen_hypervisor /boot/xen-fallback.gz dom0_mem=7584M,max:7584M watchdog ucode=scan dom0_max_vcpus=1-16 crashkernel=256M,below=4G + xen_module /boot/vmlinuz-fallback root=LABEL=root-vgdorj ro nolvm hpet=disable console=hvc0 console=tty0 + xen_module /boot/initrd-fallback.img +} +menuentry 'XCP-ng (Serial, Xen 4.13.1 / Linux 4.19.0+1)' { + search --label --set root root-vgdorj + xen_hypervisor /boot/xen-fallback.gz com1=115200,8n1 console=com1,vga dom0_mem=7584M,max:7584M watchdog ucode=scan dom0_max_vcpus=1-16 crashkernel=256M,below=4G + xen_module /boot/vmlinuz-fallback root=LABEL=root-vgdorj ro nolvm hpet=disable console=tty0 console=hvc0 + xen_module /boot/initrd-fallback.img +} diff --git a/tests/test_bootloader.py b/tests/test_bootloader.py index dc075d50..7a0a7a0a 100644 --- a/tests/test_bootloader.py +++ b/tests/test_bootloader.py @@ -9,12 +9,12 @@ class TestBootloader(unittest.TestCase): - def test_grub2(self): - bl = Bootloader.readGrub2("tests/data/grub.cfg") + def _test_cfg(self, cfg): + bl = Bootloader.readGrub2(cfg) with NamedTemporaryFile("w") as temp: bl.writeGrub2(temp.name) # get a diff - proc = subprocess.Popen(["diff", "tests/data/grub.cfg", temp.name], + proc = subprocess.Popen(["diff", cfg, temp.name], stdout = subprocess.PIPE, universal_newlines=True) @@ -32,11 +32,24 @@ def test_grub2(self): proc.wait() self.assertEqual(proc.returncode, 1) + def test_grub2(self): + '''Test read/write roundtrip of GRUB2 multiboot config''' + self._test_cfg("tests/data/grub.cfg") + + def test_grub2_xen_boot(self): + '''Test read/write roundtrip of GRUB2 xen_boot config''' + self._test_cfg("tests/data/grub-xen-boot.cfg") + def test_no_multiboot(self): # A module2 line without a multiboot2 line is an error with self.assertRaises(RuntimeError): Bootloader.readGrub2("tests/data/grub-no-multiboot.cfg") + def test_no_hypervisor(self): + # A xen_module line without a xen_hypervisor line is an error + with self.assertRaises(RuntimeError): + Bootloader.readGrub2("tests/data/grub-no-hypervisor.cfg") + class TestMenuEntry(unittest.TestCase): def setUp(self):