all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / Atom feed
* [bug#51878] [PATCH] installer: Rework installation device detection
@ 2021-11-15 21:04 Josselin Poiret via Guix-patches via
  2021-11-15 21:32 ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
  2021-11-17 14:43 ` [bug#51878] [PATCH] " Mathieu Othacehe
  0 siblings, 2 replies; 7+ messages in thread
From: Josselin Poiret via Guix-patches via @ 2021-11-15 21:04 UTC (permalink / raw)
  To: 51878; +Cc: Josselin Poiret

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [bug#51878] [PATCH v2] installer: Rework installation device detection
  2021-11-15 21:04 [bug#51878] [PATCH] installer: Rework installation device detection Josselin Poiret via Guix-patches via
@ 2021-11-15 21:32 ` Josselin Poiret via Guix-patches via
  2021-11-17 14:43 ` [bug#51878] [PATCH] " Mathieu Othacehe
  1 sibling, 0 replies; 7+ messages in thread
From: Josselin Poiret via Guix-patches via @ 2021-11-15 21:32 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 51878

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [bug#51878] [PATCH] installer: Rework installation device detection
  2021-11-15 21:04 [bug#51878] [PATCH] installer: Rework installation device detection Josselin Poiret via Guix-patches via
  2021-11-15 21:32 ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
@ 2021-11-17 14:43 ` Mathieu Othacehe
  2021-11-23 22:19   ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
  1 sibling, 1 reply; 7+ messages in thread
From: Mathieu Othacehe @ 2021-11-17 14:43 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 51878


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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [bug#51878] [PATCH v2] installer: Rework installation device detection
  2021-11-17 14:43 ` [bug#51878] [PATCH] " Mathieu Othacehe
@ 2021-11-23 22:19   ` Josselin Poiret via Guix-patches via
  2021-11-25  9:20     ` [bug#51878] [PATCH] " Mathieu Othacehe
  0 siblings, 1 reply; 7+ messages in thread
From: Josselin Poiret via Guix-patches via @ 2021-11-23 22:19 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: Josselin Poiret, 51878

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* [bug#51878] [PATCH] installer: Rework installation device detection
  2021-11-23 22:19   ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
@ 2021-11-25  9:20     ` Mathieu Othacehe
  2021-11-26  9:48       ` Josselin Poiret via Guix-patches via
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Othacehe @ 2021-11-25  9:20 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 51878


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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* [bug#51878] [PATCH] installer: Rework installation device detection
  2021-11-25  9:20     ` [bug#51878] [PATCH] " Mathieu Othacehe
@ 2021-11-26  9:48       ` Josselin Poiret via Guix-patches via
  2021-11-26 10:53         ` bug#51878: " Mathieu Othacehe
  0 siblings, 1 reply; 7+ messages in thread
From: Josselin Poiret via Guix-patches via @ 2021-11-26  9:48 UTC (permalink / raw)
  To: Mathieu Othacehe; +Cc: dev, 51878

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#51878: [PATCH] installer: Rework installation device detection
  2021-11-26  9:48       ` Josselin Poiret via Guix-patches via
@ 2021-11-26 10:53         ` Mathieu Othacehe
  0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Othacehe @ 2021-11-26 10:53 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 51878-done


Hey,

> I agree that this reads better!  Feel free to push with those changes.

Pushed as b90504cdb5ce3d1981c8d7bc8a9cc918b0d60af7.

Thanks,

Mathieu




^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-11-26 10:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 21:04 [bug#51878] [PATCH] installer: Rework installation device detection Josselin Poiret via Guix-patches via
2021-11-15 21:32 ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
2021-11-17 14:43 ` [bug#51878] [PATCH] " Mathieu Othacehe
2021-11-23 22:19   ` [bug#51878] [PATCH v2] " Josselin Poiret via Guix-patches via
2021-11-25  9:20     ` [bug#51878] [PATCH] " Mathieu Othacehe
2021-11-26  9:48       ` Josselin Poiret via Guix-patches via
2021-11-26 10:53         ` bug#51878: " Mathieu Othacehe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.