GNU bug report logs - #51878
[PATCH] installer: Rework installation device detection

Previous Next

Package: guix-patches;

Reported by: Josselin Poiret <dev <at> jpoiret.xyz>

Date: Mon, 15 Nov 2021 21:05:02 UTC

Severity: normal

Tags: patch

Done: Mathieu Othacehe <othacehe <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51878 in the body.
You can then email your comments to 51878 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Mon, 15 Nov 2021 21:05:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Josselin Poiret <dev <at> jpoiret.xyz>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Mon, 15 Nov 2021 21:05:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: guix-patches <at> gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>
Subject: [PATCH] installer: Rework installation device detection
Date: Mon, 15 Nov 2021 21:04:04 +0000
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)))
 
 
 ;;
-- 
2.33.1





Information forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Mon, 15 Nov 2021 21:34:02 GMT) Full text and rfc822 format available.

Message #8 received at 51878 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 51878 <at> debbugs.gnu.org
Subject: [PATCH v2] installer: Rework installation device detection
Date: Mon, 15 Nov 2021 21:32:32 +0000
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)))
 
 
 ;;
-- 
2.33.1





Information forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Wed, 17 Nov 2021 14:44:01 GMT) Full text and rfc822 format available.

Message #11 received at 51878 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 51878 <at> debbugs.gnu.org
Subject: Re: bug#51878: [PATCH] installer: Rework installation device detection
Date: Wed, 17 Nov 2021 14:43:11 +0000
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




Information forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Tue, 23 Nov 2021 22:20:01 GMT) Full text and rfc822 format available.

Message #14 received at 51878 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Mathieu Othacehe <othacehe <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, 51878 <at> debbugs.gnu.org
Subject: [PATCH v2] installer: Rework installation device detection
Date: Tue, 23 Nov 2021 22:19:09 +0000
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)))
 
 
 ;;
-- 
2.33.1





Information forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Thu, 25 Nov 2021 09:21:02 GMT) Full text and rfc822 format available.

Message #17 received at 51878 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 51878 <at> debbugs.gnu.org
Subject: Re: bug#51878: [PATCH] installer: Rework installation device detection
Date: Thu, 25 Nov 2021 09:20:30 +0000
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




Information forwarded to guix-patches <at> gnu.org:
bug#51878; Package guix-patches. (Fri, 26 Nov 2021 09:49:02 GMT) Full text and rfc822 format available.

Message #20 received at 51878 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Mathieu Othacehe <othacehe <at> gnu.org>
Cc: dev <at> jpoiret.xyz, 51878 <at> debbugs.gnu.org
Subject: Re: bug#51878: [PATCH] installer: Rework installation device detection
Date: Fri, 26 Nov 2021 09:48:37 +0000
Mathieu Othacehe <othacehe <at> 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




Reply sent to Mathieu Othacehe <othacehe <at> gnu.org>:
You have taken responsibility. (Fri, 26 Nov 2021 10:54:01 GMT) Full text and rfc822 format available.

Notification sent to Josselin Poiret <dev <at> jpoiret.xyz>:
bug acknowledged by developer. (Fri, 26 Nov 2021 10:54:01 GMT) Full text and rfc822 format available.

Message #25 received at 51878-done <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Othacehe <othacehe <at> gnu.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 51878-done <at> debbugs.gnu.org
Subject: Re: bug#51878: [PATCH] installer: Rework installation device detection
Date: Fri, 26 Nov 2021 10:53:07 +0000
Hey,

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

Pushed as b90504cdb5ce3d1981c8d7bc8a9cc918b0d60af7.

Thanks,

Mathieu




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 24 Dec 2021 12:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 85 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.