bug#57232: [installer] ENTER in guided partitioner destroys partition table

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Tobias Geerinckx-Rice via Bug reports for GNU Guix]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

Hi all,

Whilst testing the graphical installer, I selected ‘guided 
partitioning’ and pressed ENTER on the main drive (sda) to see 
what that does.

What that does is immediately and without confirmation wipe the 
on-disc partition table.  And its back-up.

Testdisk is great,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]


Hey Tobias,

> What that does is immediately and without confirmation wipe the on-disc
> partition table.  And its back-up.

Oops, glad you were able to recover, I was also bitten in the past. The
attached patch adds an extra confirmation page before wiping everything,
WDYT?

Mathieu

[-- Attachment #2: 0001-installer-partition-Add-a-confirmation-page-before-f.patch --]
[-- Type: text/x-patch, Size: 2111 bytes --]

From 4a9c1fb1fe7f9a65b2b7d1f9e4419b1d28a8082e Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <othacehe@gnu.org>
Date: Tue, 16 Aug 2022 10:49:07 +0200
Subject: [PATCH 1/1] installer: partition: Add a confirmation page before
 formatting.

Fixes: <https://issues.guix.gnu.org/57232>.

* gnu/installer/newt/partition.scm (run-label-confirmation-page): New
procedure.
(run-label-page): Call the above procedure before proceeding.
---
 gnu/installer/newt/partition.scm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/gnu/installer/newt/partition.scm b/gnu/installer/newt/partition.scm
index e7a97810ac..f11a644f92 100644
--- a/gnu/installer/newt/partition.scm
+++ b/gnu/installer/newt/partition.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2018, 2019 Mathieu Othacehe <m.othacehe@gmail.com>
+;;; Copyright © 2018, 2019, 2022 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;;
@@ -92,6 +92,15 @@ (define (device-items)
          (device (car result)))
     device))
 
+(define (run-label-confirmation-page callback)
+  (lambda (item)
+    (and (run-confirmation-page
+          (format #f (G_ "This will create a new ~a partition table, \
+all data on disk will be lost, are you sure you want to proceed?") item)
+          (G_ "Format disk?")
+          #:exit-button-procedure callback)
+         item)))
+
 (define (run-label-page button-text button-callback)
   "Run a page asking the user to select a partition table label."
   ;; Force the GPT label if UEFI is supported.
@@ -103,6 +112,8 @@ (define (run-label-page button-text button-callback)
        #:title (G_ "Partition table")
        #:listbox-items '("msdos" "gpt")
        #:listbox-item->text identity
+       #:listbox-callback-procedure
+       (run-label-confirmation-page button-callback)
        #:button-text button-text
        #:button-callback-procedure button-callback)))
 
-- 
2.37.1

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Tobias Geerinckx-Rice via Bug reports for GNU Guix]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Hi Mathieu!

Mathieu Othacehe 写道：
>> What that does is immediately and without confirmation wipe the 
>> on-disc
>> partition table.  And its back-up.
>
> Oops, glad you were able to recover, I was also bitten in the 
> past.

My mistake for insisting on a bare metal test without a throwaway 
machine handy :-)

> The attached patch adds an extra confirmation page before wiping
> everything, WDYT?

Code looks all right.  I'll try it out.  Thanks!

The help text for users reads, in part:

  “You can change a disk's partition table by selecting it and 
  pressing ENTER.”

Er, I was… expecting that to mean it would pop up a pretty window 
or something.  Is this really a feature?  Should it be?

I have to be honest: I was extremely let down by the installer UX, 
*because* I read a lot of the code and can see how much effort 
went into it.  I hate pointing out that the partitioner is at once 
less useful and more dangerous than (system "fdisk").

:-/,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Mathieu Othacehe]

Hey,

>  “You can change a disk's partition table by selecting it and   pressing
> ENTER.”
>
> Er, I was… expecting that to mean it would pop up a pretty window or
> something.  Is this really a feature?  Should it be?

I'm not sure what's the point here.

> I have to be honest: I was extremely let down by the installer UX, *because* I
> read a lot of the code and can see how much effort went into it.  I hate
> pointing out that the partitioner is at once less useful and more dangerous
> than (system "fdisk").

Many people have contributed to the installer over the years. Your
remark is both harsh and unconstructive. I don't think that the fact
that you, Tobias, are extremely let down matters much to the project.

Calling "fdisk" or "parted" directly would not provide the
auto-partitioning feature, and would be less convenient when it comes to
encryption and partition mount points selection.

It's no secret that the partitioning code can be improved like many
other Guix areas. If you feel like you can refine it, in term of
stability and general UX, we would be glad to get your support.

Mathieu

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Tobias Geerinckx-Rice via Bug reports for GNU Guix]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Hi Mathieu,

Mathieu Othacehe 写道：
>> Er, I was… expecting that to mean it would pop up a pretty 
>> window or
>> something.  Is this really a feature?  Should it be?
>
> I'm not sure what's the point here.

I'm not sure whose point you're referring to, so I'll clarify 
mine.

Currently, pressing ENTER on a whole disc device wipes its 
partition table, or at  least did so for me (MBR).  That's an 
unexpected way ‘to change a disk's partition table’ even if it's 
technically true.

It wasn't clear to me whether it was intentional, or a bug, or 
perhaps a bit of both.  Hence my question.  I agree that we should 
warn before writing anything, but that's somewhat orthogonal.

I had rewritten the dialogue before encountering this issue, but 
it doesn't seem like I understand the intention behind the design 
very well.  I'll drop the UI-related patches and leave that effort 
to someone who does.

> we would be glad to get your support.

That wasn't the impression I got in response to some mild 
criticism, but thanks.  I'll keep working on it when I get that 
spare machine :-)

(I do consider the installer important to the project, even if I 
never claimed to be its target audience.)

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

bug#57232: [installer] ENTER in guided partitioner destroys partition table

[Mathieu Othacehe]

Hey,

> Fixes: <https://issues.guix.gnu.org/57232>.
>
> * gnu/installer/newt/partition.scm (run-label-confirmation-page): New
> procedure.
> (run-label-page): Call the above procedure before proceeding.

I pushed a slightly edited version of this patch.

Thanks,

Mathieu