Hello, While testing the installer to add LUKS2 support in a VM, the installer was having trouble detecting which device was the installation one, so I decided to rewrite that part using guile-parted. The old code could not properly detect the device as it was comparing a disk block device path (eg `/dev/sda`) with a partition block device path (`/dev/sda2`). Instead, this patch just iterates over all partitions of each device and tries to find if the root partition is among one of them. Best, Josselin Poiret -- >8 -- * gnu/installer/parted.scm (installation-device): Remove it. * gnu/installer/parted.scm (installer-root-partition-path): Add it. * gnu/installer/parted.scm (non-install-devices): Add installation-device? predicate. --- gnu/installer/parted.scm | 51 ++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm index 00de0a30fa..ea2e26ddad 100644 --- a/gnu/installer/parted.scm +++ b/gnu/installer/parted.scm @@ -26,6 +26,7 @@ (define-module (gnu installer parted) #:use-module ((gnu build file-systems) #:select (canonicalize-device-spec find-partition-by-label + find-partition-by-uuid read-partition-uuid read-luks-partition-uuid)) #:use-module ((gnu build linux-boot) @@ -345,35 +346,39 @@ (define (remove-logical-devices) (with-null-output-ports (invoke "dmsetup" "remove_all"))) -(define (installation-device) - "Return the installation device path." +(define (installer-root-partition-path) + "Return the root partition path, or #f if it could not be detected." (let* ((cmdline (linux-command-line)) (root (find-long-option "--root" cmdline))) (and root - (canonicalize-device-spec (uuid root))))) + (or (and (access? root F_OK) root) + (find-partition-by-label root) + (and=> (uuid root) + find-partition-by-uuid))))) (define (non-install-devices) "Return all the available devices, except the install device." - (define (read-only? device) - (dynamic-wind - (lambda () - (device-open device)) - (lambda () - (device-read-only? device)) - (lambda () - (device-close device)))) - - ;; If parted reports that a device is read-only it is probably the - ;; installation device. However, as this detection does not always work, - ;; compare the device path to the installation device path read from the - ;; command line. - (let ((install-device (installation-device))) - (remove (lambda (device) - (let ((file-name (device-path device))) - (or (read-only? device) - (and install-device - (string=? file-name install-device))))) - (devices)))) + + (define the-intaller-root-partition-path + (installer-root-partition-path)) + + ;; Read partition table of device and compare each path to the one + ;; we're booting from to determine if it is the installation + ;; device. + (define (installation-device? device) + (let ((disk (disk-new device))) + (and disk + (let loop ((partition #f)) + (let ((next-partition (disk-next-partition disk + #:partition + partition))) + (and next-partition + (or (string=? the-installer-root-partition-path + (partition-get-path + next-partition)) + (loop next-partition)))))))) + + (remove installation-device? (devices))) \f ;; -- 2.33.1
Sorry for the noise, Josselin Poiret -- >8 -- * gnu/installer/parted.scm (installation-device): Remove it. * gnu/installer/parted.scm (installer-root-partition-path): Add it. * gnu/installer/parted.scm (non-install-devices): Add installation-device? predicate. --- gnu/installer/parted.scm | 51 ++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm index 00de0a30fa..42456a1d18 100644 --- a/gnu/installer/parted.scm +++ b/gnu/installer/parted.scm @@ -26,6 +26,7 @@ (define-module (gnu installer parted) #:use-module ((gnu build file-systems) #:select (canonicalize-device-spec find-partition-by-label + find-partition-by-uuid read-partition-uuid read-luks-partition-uuid)) #:use-module ((gnu build linux-boot) @@ -345,35 +346,39 @@ (define (remove-logical-devices) (with-null-output-ports (invoke "dmsetup" "remove_all"))) -(define (installation-device) - "Return the installation device path." +(define (installer-root-partition-path) + "Return the root partition path, or #f if it could not be detected." (let* ((cmdline (linux-command-line)) (root (find-long-option "--root" cmdline))) (and root - (canonicalize-device-spec (uuid root))))) + (or (and (access? root F_OK) root) + (find-partition-by-label root) + (and=> (uuid root) + find-partition-by-uuid))))) (define (non-install-devices) "Return all the available devices, except the install device." - (define (read-only? device) - (dynamic-wind - (lambda () - (device-open device)) - (lambda () - (device-read-only? device)) - (lambda () - (device-close device)))) - - ;; If parted reports that a device is read-only it is probably the - ;; installation device. However, as this detection does not always work, - ;; compare the device path to the installation device path read from the - ;; command line. - (let ((install-device (installation-device))) - (remove (lambda (device) - (let ((file-name (device-path device))) - (or (read-only? device) - (and install-device - (string=? file-name install-device))))) - (devices)))) + + (define the-installer-root-partition-path + (installer-root-partition-path)) + + ;; Read partition table of device and compare each path to the one + ;; we're booting from to determine if it is the installation + ;; device. + (define (installation-device? device) + (let ((disk (disk-new device))) + (and disk + (let loop ((partition #f)) + (let ((next-partition (disk-next-partition disk + #:partition + partition))) + (and next-partition + (or (string=? the-installer-root-partition-path + (partition-get-path + next-partition)) + (loop next-partition)))))))) + + (remove installation-device? (devices))) \f ;; -- 2.33.1
Hello Josselin, > properly detect the device as it was comparing a disk block device path (eg > `/dev/sda`) with a partition block device path (`/dev/sda2`). Instead, this When using an ISO installer image, > - (or (read-only? device) > - (and install-device > - (string=? file-name install-device))))) > - (devices)))) file-name and install-device both equal "/dev/sr0" for the cdrom device, which means it will be correctly filtered out. Is it handled correctly with your patch? When using a raw disk image, we may indeed compare devices and partitions currently. > + > + (define the-intaller-root-partition-path > + (installer-root-partition-path)) > + > + ;; Read partition table of device and compare each path to the one > + ;; we're booting from to determine if it is the installation > + ;; device. > + (define (installation-device? device) > + (let ((disk (disk-new device))) > + (and disk > + (let loop ((partition #f)) > + (let ((next-partition (disk-next-partition disk > + #:partition > + partition))) > + (and next-partition > + (or (string=? the-installer-root-partition-path > + (partition-get-path > + next-partition)) > + (loop next-partition)))))))) Filtering the "(devices)" list can cause extra iterations compared to your implementation, but is easier to read I think. Thanks, Mathieu
Hello, You're right, I didn't take CDs into account. Here is a new version which compares the device path itself to the --root argument as well, which is the case for CDs. I checked both iso9660 and qcow2 in qemu, and both only list other devices. I didn't quite get your comment about filtering the `(devices)` list. In both cases, we use `remove`, but here I've factored out the predicate used for it. Best, Josselin -- >8 -- * gnu/installer/parted.scm (installation-device): Remove it. * gnu/installer/parted.scm (installer-root-partition-path): Add it. * gnu/installer/parted.scm (non-install-devices): Add installation-device? predicate. --- gnu/installer/parted.scm | 53 +++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm index 00de0a30fa..f665e67b35 100644 --- a/gnu/installer/parted.scm +++ b/gnu/installer/parted.scm @@ -26,6 +26,7 @@ (define-module (gnu installer parted) #:use-module ((gnu build file-systems) #:select (canonicalize-device-spec find-partition-by-label + find-partition-by-uuid read-partition-uuid read-luks-partition-uuid)) #:use-module ((gnu build linux-boot) @@ -345,35 +346,41 @@ (define (remove-logical-devices) (with-null-output-ports (invoke "dmsetup" "remove_all"))) -(define (installation-device) - "Return the installation device path." +(define (installer-root-partition-path) + "Return the root partition path, or #f if it could not be detected." (let* ((cmdline (linux-command-line)) (root (find-long-option "--root" cmdline))) (and root - (canonicalize-device-spec (uuid root))))) + (or (and (access? root F_OK) root) + (find-partition-by-label root) + (and=> (uuid root) + find-partition-by-uuid))))) (define (non-install-devices) "Return all the available devices, except the install device." - (define (read-only? device) - (dynamic-wind - (lambda () - (device-open device)) - (lambda () - (device-read-only? device)) - (lambda () - (device-close device)))) - - ;; If parted reports that a device is read-only it is probably the - ;; installation device. However, as this detection does not always work, - ;; compare the device path to the installation device path read from the - ;; command line. - (let ((install-device (installation-device))) - (remove (lambda (device) - (let ((file-name (device-path device))) - (or (read-only? device) - (and install-device - (string=? file-name install-device))))) - (devices)))) + + (define the-installer-root-partition-path + (installer-root-partition-path)) + + ;; Read partition table of device and compare each path to the one + ;; we're booting from to determine if it is the installation + ;; device. + (define (installation-device? device) + (or (string=? the-installer-root-partition-path + (device-path device)) + (let ((disk (disk-new device))) + (and disk + (let loop ((partition #f)) + (let ((next-partition (disk-next-partition disk + #:partition + partition))) + (and next-partition + (or (string=? the-installer-root-partition-path + (partition-get-path + next-partition)) + (loop next-partition))))))))) + + (remove installation-device? (devices))) \f ;; -- 2.33.1
Hello Josselin, Thanks for the v2! > I didn't quite get your comment about filtering the `(devices)` list. > In both cases, we use `remove`, but here I've factored out the predicate > used for it. I was not very clear sorry about that. I meant something like that seems a little more concise: --8<---------------cut here---------------start------------->8--- diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm index 82b01d2ce1..ad7dd6bf91 100644 --- a/gnu/installer/parted.scm +++ b/gnu/installer/parted.scm @@ -366,19 +366,16 @@ (define the-installer-root-partition-path ;; we're booting from to determine if it is the installation ;; device. (define (installation-device? device) + ;; When using CDROM based installation, the root partition path may be the + ;; device path. (or (string=? the-installer-root-partition-path (device-path device)) (let ((disk (disk-new device))) (and disk - (let loop ((partition #f)) - (let ((next-partition (disk-next-partition disk - #:partition - partition))) - (and next-partition - (or (string=? the-installer-root-partition-path - (partition-get-path - next-partition)) - (loop next-partition))))))))) + (any (lambda (partition) + (string=? the-installer-root-partition-path + (partition-get-path partition))) + (disk-partitions disk)))))) (remove installation-device? (devices))) --8<---------------cut here---------------end--------------->8--- WDYT? Thanks, Mathieu
Mathieu Othacehe <othacehe@gnu.org> writes:
> Hello Josselin,
> I was not very clear sorry about that. I meant something like that seems
> a little more concise:
>
> --8<---------------cut here---------------start------------->8---
> snip
> --8<---------------cut here---------------end--------------->8---
>
> WDYT?
I agree that this reads better! Feel free to push with those changes.
Best,
Josselin Poiret
Hey,
> I agree that this reads better! Feel free to push with those changes.
Pushed as b90504cdb5ce3d1981c8d7bc8a9cc918b0d60af7.
Thanks,
Mathieu