Repository navigation
fix: report a disk configuration that cannot be carried out - #4863
xander-lin wants to merge 3 commits into
Conversation
The configuration is read key by key, so a key name that is not spelled exactly right, or one from an older release, was not an error anywhere: the option it was meant to set kept its default and the installation ran with something other than what the file asks for. Name the keys that were not understood. The `version` field of the file was never read at all - it was overwritten with the running version right after parsing - so a file written by an incompatible release was applied as if it were current. Report the mismatch.
`DiskLayoutConfiguration.parse_arg()` dropped a device entry it could not find and returned a layout describing no disk and no partition at all - still truthy, so every later "is a layout set?" guard passed and the installation went on to do nothing. Raise instead, and report the devices that were probed. An EXIST or MODIFY partition entry was taken as-is even when its `dev_path` belongs to another disk, so the blueprint pointed at a partition nothing had seen. Require it to be one of the partitions of the listed device. Combining an encryption type that has no LVM recipe (LUKS, or no encryption) with `lvm_config` built no volume group and no volume, and the installation continued into a mount phase with nothing to mount; reject the combination both when parsing the configuration and in `_setup_lvm_encrypted()`. Encrypted swap is never activated - the mount code has no swap branch for a mapper device - so a hand written configuration describing it finished with no swap and no fstab entry for it. Reject it while the configuration is being read.
The unknown-key check only looked at the top level of the file, so a misspelled key one level down - where the layout applied to the disks is described - was still dropped without a word. Walk the serialisation types so the message can name the path (device_modifications[0].partitions[0].mountpiont) instead of guessing, and so the check cannot drift away from what is parsed.
|
AI_POLICY, first bullet point:
I don't mind merging the contributions, but I wonder how much of this is actually verified as sane and needed vs solved in a different way? |
* fix(disk): skip deleted partitions when mounting and booting A DELETE entry keeps its dev_path, mountpoint and flags. The menu sorts deletes first, so on a reinstall the ESP/boot/root lookups returned the old partitions, and the mount stage mounted or swapon'd them. Filter is_delete() there and when preparing fs packages. Port of archlinux/archinstall#4861. Signed-off-by: h8d13 <hadean-eon-dev@proton.me> * fix(disk): never offer or accept a zero-length partition A free gap between off-grain neighbours can round below 1 MiB once snapped, and a config could carry a 0 B create. Keep only the aligned span in FreeSpace so the menu's buffer check drops such gaps, and reject a create without a length. Port of archlinux/archinstall#4862. Signed-off-by: h8d13 <hadean-eon-dev@proton.me> * fix(disk): swapoff and unmount by device during cleanup [SWAP] is dropped from lsblk mountpoints for the menu, so umount() never saw an active swap and left it on before partitioning. Luks2.lock() passed mountpoints to umount(), which lsblk rejects, so any mounted mapper failed with 'Failed to read disk'. Swapoff swap devices and unmount the mapper by its path. Port of archlinux/archinstall#4864 and #4865. Signed-off-by: h8d13 <hadean-eon-dev@proton.me> * fix(disk): reject lvm with plain luks, no_encryption is plain lvm perform_lvm_operations took the encrypted path for any enc config, which only handles lvm_on_luks/luks_on_lvm, so luks or no_encryption with lvm_config built no volume group. Reject luks at parse time and route no_encryption to the plain lvm setup. Port of archlinux/archinstall#4863 (that part only). Signed-off-by: h8d13 <hadean-eon-dev@proton.me> --------- Signed-off-by: h8d13 <hadean-eon-dev@proton.me>
|
Closing for now. I checked every entry point this PR touches, and none of them is reachable from the TUI:
All five new So the main line of attack for this project is the interactive path, and I do not have a If you want the configuration path hardened, say so and I will bring it back as a single |
Summary
The configuration file is read key by key, and nothing checks that what it describes can actually
be carried out. Three separate cases end with the installation doing something other than what the
file asked for, and none of them says a word.
1. A device entry that cannot be found is dropped
DiskLayoutConfiguration.parse_arg()skipped anydevice_modificationsentry whose device was notprobed:
The result is a layout that describes no disk and no partition at all and is still truthy, so every
later "is a layout set?" guard passes and the installation continues with nothing to install onto.
2. An
existingpartition entry is trusted to belong to the device it is listed underNothing checked that its
dev_pathis one of that device's partitions, so a hand-written ormachine-translated file could point the blueprint at a partition nothing had probed.
3. An encryption type with no LVM recipe is accepted together with
lvm_configFilesystemHandler._setup_lvm_encrypted()only builds a volume group forLVM_ON_LUKSandLUKS_ON_LVM. Withluksorno_encryptionpluslvm_configno volume group and no volume arecreated, and the installation goes on into a mount phase with nothing to mount.
Reproducer
Needs root only for the loop device named in the configurations. Each case prints what the loader
does with it.
On master:
The first line is the one that matters. The layout that comes back describes no disk and no
partition at all, and it is still a truthy object, so every later "is a layout set?" guard passes and
the installation carries on with nothing to install onto.
With the patch:
The third case needs a password to get past
DiskEncryption.parse_arg(), which returnsNonewithout one, so it is easier to call the loader directly:
On master:
The first two describe a volume group that
_setup_lvm_encrypted()will never build, so theinstallation continues into a mount phase with nothing to mount. With the patch:
4. Encrypted swap is accepted but never activated
The mount code has no swap branch for a mapper device, so an encrypted swap area is never
swaponed: the installation finishes without swap and without an fstab entry for it. The menu doesnot offer swap for encryption, but a hand-written configuration can ask for it:
This is a rejection rather than support - archinstall does not allow encrypted swap, and this patch
does not change that; it says so instead of dropping it silently.
A key that is not spelled right is not an error either
The same "read key by key" property means a misspelled key is not reported anywhere: the option it
was meant to set keeps its default. The existing check only looked at the top level of the file, so
a misspelled key one level down - where the layout that is applied to the disks is described - was
still dropped without a word.
This patch walks the serialisation types themselves, so the message can name the path and the check
cannot drift away from what is parsed:
The file may also record the release that wrote it. That value was never read, so a file written by
an incompatible release was applied as if it were current; it is now compared with the running
version:
Two keys that are documented but only exist as command line flags (
debug,silent,offline,no_pkg_lookups) are named as such rather than reported as misspellings, because pointing at theuser's file would be misleading when the option is simply not wired up to it.
Why warnings and not errors
Everything above is something the installation can detect but not correct on its own, and the file
may be perfectly usable with that one key fixed. A warning names it and the run continues with the
same behaviour as before, so no configuration that works today stops working.
The three
parse_arg()cases are different in kind: they describe something that cannot be carriedout at all, so they raise.
Tests
tests/test_config_validation.py,tests/test_config_keys.py, and additions totests/test_args.py:existingentry whosedev_pathbelongs to another device raises;luksandno_encryptioncombined withlvm_configraise,lvm_on_luksandluks_on_lvmdo not;device_modifications[0].partitions[0], and onenested under a further level are each reported with their path;