[Merge] ~waveform/ubuntu-release-upgrader:remove-uboot into ubuntu-release-upgrader:ubuntu/master
Dave Jones
mp+406663 at code.launchpad.net
Tue Aug 24 10:34:11 UTC 2021
Added responses to each review comment. Will upload a commit to address the outstanding points.
Diff comments:
> diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
> index 712c98e..40f9f97 100644
> --- a/DistUpgrade/DistUpgradeQuirks.py
> +++ b/DistUpgrade/DistUpgradeQuirks.py
> @@ -1205,3 +1208,133 @@ class DistUpgradeQuirks(object):
> except IOError as exc:
> logging.error("unable to write new boot config to %s: %s; %s",
> boot_config_filename, exc, failure_action)
> +
> + def _remove_uboot_on_rpi(self, boot_dir='/boot/firmware'):
> + failure_action = (
> + "you may need to replace u_boot_* with vmlinuz, and add "
Good point (I'll similarly correct the corresponding message in _replace_fkms_overlay while I'm at it).
> + "'initramfs initrd.img followkernel' to config.txt on your boot "
> + "partition; see LP: #1936401 for further details")
I've expanded the description in LP: #1936401 to include the manual instructions (and some justifications to clarify the more subtle aspects of the transition).
> + change_prefix = '# commented by do-release-upgrade (LP: #1936401)'
> + added_prefix = '# added by do-release-upgrade (LP: #1936401)'
> + merge_prefix = '# merged from {} by do-release-upgrade (LP: #1936401)'
> +
> + try:
> + boot_config_filename = os.path.join(boot_dir, 'config.txt')
> + with open(boot_config_filename, 'r', encoding='utf-8') as f:
> + boot_config = f.read()
> + except FileNotFoundError:
> + logging.error("failed to open boot configuration in %s; %s",
> + boot_config_filename, failure_action)
> + return
> +
> + def replace_uboot(lines):
> + result = []
> + removed_uboot = added_kernel = False
> + for line in lines:
> + if line == '[all]':
> + # Add the vmlinuz kernel and initrd.img to the first
> + # encountered [all] section, just in case the user has any
> + # custom kernel overrides later in the sequence
> + result.append(line)
> + if not added_kernel:
> + result.append(added_prefix)
> + result.append('kernel=vmlinuz')
> + result.append('initramfs initrd.img followkernel')
> + added_kernel = True
> + elif line.startswith('device_tree_address='):
> + # Disable any device_tree_address= line (leave the boot
> + # firmware to place this rather than risk trampling over it
In this case, no. The Focal (and earlier) configurations force the native bootloader to place the device-tree at a specific address (0x3000000) because the early u-boot environments simply assumed a fixed address for the device-tree. They also assumed fixed addresses for the kernel and the initrd, which caused some fun when one of those (the initrd, IIRC) grew to such a degree it wound up stomping all over the device-tree's memory.
It's generally much safer (although a little more complex) to let the bootloader figure out where to place the device-tree in memory itself. After all, it knows the size of everything it's loading so it's best placed to know where to stick things. The only extra complexity is that whatever gets called next has to know to look for the device-tree at a location specified by the bootloader in a register.
> + # with kernel/initrd)
> + result.append(change_prefix)
> + result.append('#' + line)
> + elif line.startswith('kernel=uboot_rpi_'):
> + # Disable all kernel=uboot_rpi_* lines found in the
> + # configuration
> + removed_uboot = True
> + result.append(change_prefix)
> + result.append('#' + line)
> + else:
> + result.append(line)
> + # We don't *want* to touch the config unless we absolutely have to,
> + # and the user may have already performed this conversion (e.g. to
> + # take advantage of USB MSD boot), or otherwise customized their
> + # boot configuration. Hence, if we didn't find (and remove) any
> + # u-boot entries, assume the kernel/initrd config was already fine
> + # and return the verbatim config
> + if removed_uboot:
> + if not added_kernel:
> + result.append(added_prefix)
> + result.append('[all]')
> + result.append('kernel=vmlinuz')
> + result.append('initramfs initrd.img followkernel')
I've pulled that out into a variable though I do suspect the chances of that changing are vanishingly small: the only two things that could change are the name of the initrd.img file (plausible it could change, but begs the question why when there's no need?), or the location it's loaded (followkernel meaning "stick it directly after the Linux kernel in memory, or a specific address -- the latter of which would cause the same issues as those caused by device_tree_address as mentioned earlier).
> + return result
> + else:
> + return lines
> +
> + def merge_includes(lines):
> + result = []
> + skip_comments = True
> + found_includes = False
> + for line in lines:
> + # Skip the initial comment block warning that config.txt is not
> + # to be edited (the usercfg.txt and syscfg.txt files were
> + # merged in the groovy release, but not for upgraders)
> + if line.startswith('#') and skip_comments:
> + continue
> + skip_comments = False
> + if line in ('include syscfg.txt', 'include usercfg.txt'):
> + # Merge the usercfg.txt and syscfg.txt configuration files
> + # into config.txt, skipping the initial comments. Note that
The initial comments are skipped to clean up the mess of split configuration I made in Focal. In Focal, the start of config.txt reads (paraphrasing): "Don't touch this file on pain of stuff breaking", while syscfg.txt (an included file) starts with something akin to "Don't touch this file except via system tools" and usercfg.txt (another included file) starts with a comment along the lines of "Fiddle with stuff here if you want, but if you break it you get to keep the pieces".
By the time of Groovy's release I'd realized this was all a horrid mistake because (amongst a few other things) certainly configuration settings didn't work in included files (you can read the whole sorry saga at https://waldorf.waveform.org.uk/2020/boot-configuration-with-pibootctl.html). Anyway, that meant Groovy went back to having a single, much simpler, config.txt.
So, this removal of initial comments is a bit of a fudge. Because we're removing the split configuration (due partly to the aforementioned failure of certain settings in included files), those initial warnings are now entirely redundant (if not downright confusing if left in place).
However, what about the case of someone upgrading from bionic (which had no such comments and no configuration split), and then from focal to impish? In this case the user won't have a split configuration (they'll still have bionic's u-boot configuration), and we definitely *shouldn't* remove comments because whatever comments are there are probably written by the user for some useful purpose (bionic's configuration had no comments).
This is why the merge_includes sub-routine takes the (possibly confusing) step at the end of checking whether we merged any included files. If we did (Focal-style config), the result (including the removal of initial comments) is returned; if we didn't (Bionic/Groovy-style config), the original file *including its original comments* is returned instead. This is quite deliberate but, I admit, all horribly complicated!
> + # we don't bother with other includes the user has added
> + # themselves (as they presumably want to keep those)
> + found_includes = True
> + included_filename = line.split(maxsplit=1)[1]
> + result.append(merge_prefix.format(included_filename))
> + included_filename = os.path.join(boot_dir,
> + included_filename)
Not really: the filename used here is the filename as the bootloader itself will see it (prior to Linux starting and mounting various file-systems). The bootloader is (understandably) quite crude in its handling of filenames (such as those in the "include" statement or, for that matter, in the statements like "kernel" or "initramfs"): it simply concatenates the filename to the current "os_prefix" setting which defaults to "/" (but remember that the bootloader's notion of the root of the file-system is different: it's just the FAT boot partition that's mounted).
This is why we simply join "our" notion of the boot-directory (usually "/boot/firmware") to whatever is specified in the include command. Of note, the os_prefix handling is even more basic than that: if os_prefix is set to "/foo" and, say, "kernel=vmlinuz" then the bootloader will attempt to load "/foovmlinuz" rather than "/foo/vmlinuz", i.e. it is a *literal* concatenation rather than a path join.
Technically, we *should* use a literal concatenation here too, but firstly I'm not aware of anyone actually relying on such behaviour in practice, and secondly it also complicates the calling of the routine as callers would need to be aware that the boot_dir parameter *must* end with a "/" under most normal circumstances.
> + skip_comments = True
> + with open(included_filename, 'r', encoding='utf-8') as f:
> + for line in f:
> + if line.startswith('#') and skip_comments:
> + continue
> + skip_comments = False
> + result.append(line.rstrip())
> + target_filename = included_filename + '.distUpgrade'
> + try:
> + os.rename(included_filename, target_filename)
> + except IOError as exc:
> + logging.error("failed to move included configuration "
> + "from %s to %s; %s", included_filename,
> + target_filename, exc)
> + else:
> + result.append(line)
> + # Again, if we found no includes, return the original verbatim
> + # (i.e. with initial comments intact)
> + if found_includes:
> + return result
> + else:
> + return lines
> +
> + lines = [line.rstrip() for line in boot_config.splitlines()]
> + lines = merge_includes(replace_uboot(lines))
> + new_config = ''.join(line + '\n' for line in lines)
> +
> + if new_config == boot_config:
> + logging.warning("no u-boot removal performed in %s",
> + boot_config_filename)
> + return
> +
> + try:
> + boot_backup_filename = boot_config_filename + '.distUpgrade'
> + with open(boot_backup_filename, 'w', encoding='utf-8') as f:
> + f.write(boot_config)
> + except IOError as exc:
> + logging.error("unable to write boot config backup to %s: %s; %s",
> + boot_backup_filename, exc, failure_action)
> + return
> + try:
> + with open(boot_config_filename, 'w', encoding='utf-8') as f:
> + f.write(new_config)
> + except IOError as exc:
> + logging.error("unable to write new boot config to %s: %s; %s",
> + boot_config_filename, exc, failure_action)
> diff --git a/tests/test_quirks.py b/tests/test_quirks.py
> index cc4c8f1..0e92275 100644
> --- a/tests/test_quirks.py
> +++ b/tests/test_quirks.py
> @@ -519,6 +522,203 @@ class TestQuirks(unittest.TestCase):
> with open(os.path.join(boot_dir, 'config.txt')) as f:
> self.assertTrue(f.read() == expected_config)
>
> + def test_remove_uboot_no_config(self):
> + with tempfile.TemporaryDirectory() as boot_dir:
> + mock_controller = mock.Mock()
> + q = DistUpgradeQuirks(mock_controller, mock.Mock())
> + q._remove_uboot_on_rpi(boot_dir)
> +
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'config.txt.distUpgrade')))
> +
> + def test_remove_uboot_no_changes(self):
> + with tempfile.TemporaryDirectory() as boot_dir:
> + native_config = """\
> +# This is a demo boot config with a comment at the start that should not
> +# be removed
> +
> +[pi4]
> +max_framebuffers=2
> +
> +[all]
> +arm_64bit=1
> +kernel=vmlinuz
> +initramfs initrd.img followkernel
> +
> +# This is a user-added include that should not be merged
> +include custom.txt
> +"""
> + custom_config = """\
> +# This is the custom included configuration file
> +
> +hdmi_group=1
> +hdmi_mode=4
> +"""
> + with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
> + f.write(native_config)
> + with open(os.path.join(boot_dir, 'custom.txt'), 'w') as f:
> + f.write(custom_config)
> +
> + mock_controller = mock.Mock()
> + q = DistUpgradeQuirks(mock_controller, mock.Mock())
> + q._remove_uboot_on_rpi(boot_dir)
> +
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'config.txt.distUpgrade')))
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'custom.txt.distUpgrade')))
> + with open(os.path.join(boot_dir, 'config.txt')) as f:
> + self.assertTrue(f.read() == native_config)
> + with open(os.path.join(boot_dir, 'custom.txt')) as f:
> + self.assertTrue(f.read() == custom_config)
> +
> + def test_remove_uboot_with_changes(self):
> + with tempfile.TemporaryDirectory() as boot_dir:
> + config_txt = """\
> +# This is a warning that you should not edit this file. The upgrade should
> +# remove this comment
> +
> +[pi4]
> +# This is a comment that should be included
> +kernel=uboot_rpi_4.bin
> +max_framebuffers=2
> +
> +[pi2]
> +kernel=uboot_rpi_2.bin
> +
> +[pi3]
> +kernel=uboot_rpi_3.bin
> +
> +[all]
> +arm_64bit=1
> +device_tree_address=0x3000000
> +include syscfg.txt
> +include usercfg.txt
> +dtoverlay=vc4-fkms-v3d,cma-256
> +include custom.txt
> +"""
> + usercfg_txt = """\
> +# Another chunk of warning text that should be skipped
> +"""
> + syscfg_txt = """\
> +# Yet more warnings to exclude
> +dtparam=audio=on
> +dtparam=spi=on
> +enable_uart=1
> +"""
> + custom_txt = """\
> +# This is a user-added file that should be left alone by the upgrade
> +[gpio4=1]
> +kernel=custom
> +"""
> + expected_config_txt = """\
> +
> +[pi4]
> +# This is a comment that should be included
> +# commented by do-release-upgrade (LP: #1936401)
> +#kernel=uboot_rpi_4.bin
> +max_framebuffers=2
> +
> +[pi2]
> +# commented by do-release-upgrade (LP: #1936401)
> +#kernel=uboot_rpi_2.bin
> +
> +[pi3]
> +# commented by do-release-upgrade (LP: #1936401)
> +#kernel=uboot_rpi_3.bin
> +
> +[all]
> +# added by do-release-upgrade (LP: #1936401)
> +kernel=vmlinuz
> +initramfs initrd.img followkernel
> +arm_64bit=1
> +# commented by do-release-upgrade (LP: #1936401)
> +#device_tree_address=0x3000000
> +# merged from syscfg.txt by do-release-upgrade (LP: #1936401)
> +dtparam=audio=on
> +dtparam=spi=on
> +enable_uart=1
> +# merged from usercfg.txt by do-release-upgrade (LP: #1936401)
> +dtoverlay=vc4-fkms-v3d,cma-256
> +include custom.txt
> +"""
> + with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
> + f.write(config_txt)
> + with open(os.path.join(boot_dir, 'syscfg.txt'), 'w') as f:
> + f.write(syscfg_txt)
> + with open(os.path.join(boot_dir, 'usercfg.txt'), 'w') as f:
> + f.write(usercfg_txt)
> + with open(os.path.join(boot_dir, 'custom.txt'), 'w') as f:
> + f.write(custom_txt)
> +
> + mock_controller = mock.Mock()
> + q = DistUpgradeQuirks(mock_controller, mock.Mock())
> + q._remove_uboot_on_rpi(boot_dir)
> +
> + self.assertTrue(os.path.exists(os.path.join(
> + boot_dir, 'config.txt.distUpgrade')))
> + self.assertTrue(os.path.exists(os.path.join(
> + boot_dir, 'syscfg.txt.distUpgrade')))
> + self.assertTrue(os.path.exists(os.path.join(
> + boot_dir, 'usercfg.txt.distUpgrade')))
> + self.assertTrue(os.path.exists(os.path.join(
> + boot_dir, 'custom.txt')))
Fair point, will address in a further commit.
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'syscfg.txt')))
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'usercfg.txt')))
> + self.assertFalse(os.path.exists(os.path.join(
> + boot_dir, 'custom.txt.distUpgrade')))
> + with open(os.path.join(boot_dir, 'config.txt')) as f:
> + self.assertTrue(f.read() == expected_config_txt)
> + with open(os.path.join(boot_dir, 'custom.txt')) as f:
> + self.assertTrue(f.read() == custom_txt)
> +
> + def test_remove_uboot_no_all_section(self):
> + with tempfile.TemporaryDirectory() as boot_dir:
> + config_txt = """\
> +arm_64bit=1
> +device_tree_address=0x3000000
> +
> +[pi4]
> +# This is a comment that should be included
> +kernel=uboot_rpi_4.bin
> +max_framebuffers=2
> +
> +[pi3]
> +kernel=uboot_rpi_3.bin
> +"""
> + expected_config_txt = """\
> +arm_64bit=1
> +# commented by do-release-upgrade (LP: #1936401)
> +#device_tree_address=0x3000000
> +
> +[pi4]
> +# This is a comment that should be included
> +# commented by do-release-upgrade (LP: #1936401)
> +#kernel=uboot_rpi_4.bin
> +max_framebuffers=2
> +
> +[pi3]
> +# commented by do-release-upgrade (LP: #1936401)
> +#kernel=uboot_rpi_3.bin
> +# added by do-release-upgrade (LP: #1936401)
> +[all]
> +kernel=vmlinuz
> +initramfs initrd.img followkernel
> +"""
> + with open(os.path.join(boot_dir, 'config.txt'), 'w') as f:
> + f.write(config_txt)
> +
> + mock_controller = mock.Mock()
> + q = DistUpgradeQuirks(mock_controller, mock.Mock())
> + q._remove_uboot_on_rpi(boot_dir)
> +
> + self.assertTrue(os.path.exists(os.path.join(
> + boot_dir, 'config.txt.distUpgrade')))
> + with open(os.path.join(boot_dir, 'config.txt')) as f:
> + self.assertTrue(f.read() == expected_config_txt)
> +
>
> class TestSnapQuirks(unittest.TestCase):
>
--
https://code.launchpad.net/~waveform/ubuntu-release-upgrader/+git/ubuntu-release-upgrader/+merge/406663
Your team Ubuntu Core Development Team is subscribed to branch ubuntu-release-upgrader:ubuntu/master.
More information about the Ubuntu-reviews
mailing list