Re: [PATCH] PRELIMINARY: Add support for hibernation.

[Ludovic Courtès]

Hello!

Mark H Weaver <address@hidden> skribis:

> Here's a preliminary patch to add support for hibernation.  To enable
> it, you'll also need to add a line like this to your 'operating-system'
> definition.
>
>   (kernel-arguments '("resume=/dev/sda2"))
>
> Where the device named is a swap partition.

Awesome!  I missed this.

> It may be that we should add a dedicated 'resume-device' field to the
> 'operating-system'.  Thoughts?

I think it’s OK this way (though we should give an example in the
manual), but not strong opinion.

> From ad1d583eb6f009a2dfbc284cb8368608caf27a05 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <address@hidden>
> Date: Sun, 14 Aug 2016 05:33:12 -0400
> Subject: [PATCH] PRELIMINARY: Add support for hibernation.
>
> * gnu/build/linux-boot.scm (boot-system): Look for a resume=<device-name>
> argument on the linux command line, and if present, attempt to resume from
> hibernation.
> * gnu/services/desktop.scm (<elogind-configuration>): Change the default
> value of the 'handle-hibernate-key' key to 'hibernate'.

[...]

> +            (resume-device (find-long-option "resume" args)))

Could you mention this argument under “Initial RAM Disk”?

> +       ;;
> +       ;; Attempt to resume from hibernation.
> +       ;;
> +       ;; IMPORTANT: This *must* happen before we mount any filesystems on
> +       ;; disk.  Quoting linux-libre/Documentation/swsusp.txt:
> +       ;; 
> +       ;; * BIG FAT WARNING 
> **************************************************
> +       ;; *
> +       ;; * If you touch anything on disk between suspend and resume...
> +       ;; *                          ...kiss your data goodbye.
> +       ;; *
> +       ;; * If you do resume from initrd after your filesystems are 
> mounted...
> +       ;; *                          ...bye bye root partition.
> +       ;; *                  [this is actually same case as above]
> +       ;; *
> +       (when (and resume-device
> +                  (file-exists? resume-device)
> +                  (file-exists? "/sys/power/resume"))
> +         (false-if-exception
> +          (let* ((device-base-name
> +                  ;; The base name of the device file, after resolving
> +                  ;; symlinks.
> +                  (let loop ((file resume-device))
> +                    (match (stat:type (lstat file))
> +                      ('symlink
> +                       (let ((target (readlink file)))
> +                         (if (string-prefix? "/" target)
> +                             (loop target)
> +                             (loop (string-append (dirname file) "/" 
> target)))))
> +                      (_ (basename file)))))
> +                 (major+minor
> +                  ;; The major:minor string (e.g. "8:2") corresponding
> +                  ;; to the resume device.
> +                  (call-with-input-file (string-append "/sys/class/block/"
> +                                                       device-base-name
> +                                                       "/dev")
> +                    read-line)))
> +            ;; Write the major:minor string to /sys/power/resume
> +            ;; to attempt resume from hibernation.
> +            (when major+minor
> +              (call-with-output-file "/sys/power/resume"
> +                (cut display major+minor <>))))))

Could you turn the (false-if-exception …) expression into a separate
procedure?

I would perhaps remove ‘false-if-exception’: that way, if something goes
wrong, we’ll be able to see what the error is and get a REPL.  It might
also be useful to display a message like “resuming from device /dev/xyz
(8:2)…”

Apart from that it seems alright!

It’d be interesting to see if we can write a test.  The existing tests
do not use a swap partition, but adding one shouldn’t be too hard.

Thank you,
Ludo’.