diff mbox series

[bug#51878] installer: Rework installation device detection

Message ID 20211115210404.10680-1-dev@jpoiret.xyz
State Accepted
Headers show
Series [bug#51878] installer: Rework installation device detection | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Josselin Poiret Nov. 15, 2021, 9:04 p.m. UTC 
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(-)

Comments

Mathieu Othacehe Nov. 17, 2021, 2:43 p.m. UTC | #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
diff mbox series

Patch

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)))
 
 
 ;;