* [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 related [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 related [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 related [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 related [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
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
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
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.