[bug#29922] [PATCH] linux-boot: Improve error reporting.

[bug#29922] [PATCH] linux-boot: Improve error reporting.

[Danny Milosavljevic]

* gnu/build/linux-boot.scm (boot-system): Improve error reporting.
---
 gnu/build/linux-boot.scm | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index 4dd740174..85c855e40 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -507,7 +507,23 @@ to it are lost."
              (switch-root "/root")
              (format #t "loading '~a'...\n" to-load)
 
-             (primitive-load to-load)
+             (catch #t
+               (lambda ()
+                 (primitive-load to-load))
+               (lambda (key . args)
+                 (format (current-error-port) "Error: ~a: ~a\n" key args)
+                 (reboot))
+               (lambda (key . args)
+                 (let ((print-frames
+                        (@@ (system repl debug) print-frames))
+                       (frame->stack-vector
+                        (@@ (system repl debug) frame->stack-vector))
+                       (stack-ref
+                        (@@ (system repl debug) stack-ref)))
+                   (print-frames
+                    (frame->stack-vector
+                     (stack-ref (make-stack #t) 0))
+                    #:width 1000000))))
 
              (format (current-error-port)
                      "boot program '~a' terminated, rebooting~%"

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Danny Milosavljevic]

* gnu/build/linux-boot.scm (boot-system): Improve error reporting.
* gnu/system.scm (operating-system-user-kernel-arguments): Export.
* gnu/tests.scm (marionette-operating-system): Modify kernel-arguments.
---
 gnu/build/linux-boot.scm | 21 ++++++++++++++++++++-
 gnu/system.scm           |  1 +
 gnu/tests.scm            |  3 +++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index 4dd740174..bff2eb0f4 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -507,7 +507,26 @@ to it are lost."
              (switch-root "/root")
              (format #t "loading '~a'...\n" to-load)
 
-             (primitive-load to-load)
+             (catch #t
+               (lambda ()
+                 (primitive-load to-load))
+               (lambda (key proc format-string format-args . rest)
+                 (format (current-error-port) "~a: ~a~%" proc
+                  (apply format #f format-string format-args))
+                 (force-output (current-error-port))
+                 (exit 1)
+                 (reboot))
+               (lambda _
+                 (let ((print-frames
+                        (@@ (system repl debug) print-frames))
+                       (frame->stack-vector
+                        (@@ (system repl debug) frame->stack-vector))
+                       (stack-ref
+                        (@@ (system repl debug) stack-ref)))
+                   (print-frames
+                    (frame->stack-vector
+                     (stack-ref (make-stack #t) 0))
+                    #:width 1000000))))
 
              (format (current-error-port)
                      "boot program '~a' terminated, rebooting~%"
diff --git a/gnu/system.scm b/gnu/system.scm
index df89ca06d..a4407e8ab 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -73,6 +73,7 @@
             operating-system-hosts-file
             operating-system-kernel
             operating-system-kernel-file
+            operating-system-user-kernel-arguments
             operating-system-kernel-arguments
             operating-system-initrd
             operating-system-users
diff --git a/gnu/tests.scm b/gnu/tests.scm
index 0caa922fd..dc0188f1e 100644
--- a/gnu/tests.scm
+++ b/gnu/tests.scm
@@ -172,6 +172,9 @@ marionette service in the guest is started after the Shepherd services listed
 in REQUIREMENTS."
   (operating-system
     (inherit os)
+    ;; Make sure that the system doesn't hang on kernel panic.
+    (kernel-arguments (cons "panic=1"
+                            (operating-system-user-kernel-arguments os)))
     (services (cons (service marionette-service-type
                              (marionette-configuration
                               (requirements requirements)

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Danny Milosavljevic]

(I've also tried to adapt call-with-error-handling instead - but for some reason it never disables the repl prompt)

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Ludovic Courtès]

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> * gnu/build/linux-boot.scm (boot-system): Improve error reporting.
> * gnu/system.scm (operating-system-user-kernel-arguments): Export.
> * gnu/tests.scm (marionette-operating-system): Modify kernel-arguments.
> ---
>  gnu/build/linux-boot.scm | 21 ++++++++++++++++++++-
>  gnu/system.scm           |  1 +
>  gnu/tests.scm            |  3 +++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
> index 4dd740174..bff2eb0f4 100644
> --- a/gnu/build/linux-boot.scm
> +++ b/gnu/build/linux-boot.scm
> @@ -507,7 +507,26 @@ to it are lost."
>               (switch-root "/root")
>               (format #t "loading '~a'...\n" to-load)
>  
> -             (primitive-load to-load)
> +             (catch #t
> +               (lambda ()
> +                 (primitive-load to-load))
> +               (lambda (key proc format-string format-args . rest)
> +                 (format (current-error-port) "~a: ~a~%" proc
> +                  (apply format #f format-string format-args))
> +                 (force-output (current-error-port))
> +                 (exit 1)
> +                 (reboot))

The safe way (an exception could have fewer arguments that what’s above)
is to write something like:

      (catch #t
        (lambda ()
          (primitive-load to-load))
        (const #t)  ;nothing
        (lambda (key . args)
          (print-exception (current-error-port)
                           (stack-ref (make-stack #t) 1)
                           key args)
          (exit 1)))

Could you check if it works for you?

> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -73,6 +73,7 @@
>              operating-system-hosts-file
>              operating-system-kernel
>              operating-system-kernel-file
> +            operating-system-user-kernel-arguments
>              operating-system-kernel-arguments
>              operating-system-initrd
>              operating-system-users
> diff --git a/gnu/tests.scm b/gnu/tests.scm
> index 0caa922fd..dc0188f1e 100644
> --- a/gnu/tests.scm
> +++ b/gnu/tests.scm
> @@ -172,6 +172,9 @@ marionette service in the guest is started after the Shepherd services listed
>  in REQUIREMENTS."
>    (operating-system
>      (inherit os)
> +    ;; Make sure that the system doesn't hang on kernel panic.
> +    (kernel-arguments (cons "panic=1"
> +                            (operating-system-user-kernel-arguments os)))
>      (services (cons (service marionette-service-type
>                               (marionette-configuration
>                                (requirements requirements)

That’s a good idea.  Could you make it a separate commit?

Thanks,
Ludo’.

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Danny Milosavljevic]

Hi Ludo,

On Mon, 08 Jan 2018 10:05:10 +0100
ludo@gnu.org (Ludovic Courtès) wrote:
> Danny Milosavljevic <dannym@scratchpost.org> skribis:
> 
> > * gnu/build/linux-boot.scm (boot-system): Improve error reporting.
> > * gnu/system.scm (operating-system-user-kernel-arguments): Export.
> > * gnu/tests.scm (marionette-operating-system): Modify kernel-arguments.
> > ---
> >  gnu/build/linux-boot.scm | 21 ++++++++++++++++++++-
> >  gnu/system.scm           |  1 +
> >  gnu/tests.scm            |  3 +++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
> > index 4dd740174..bff2eb0f4 100644
> > --- a/gnu/build/linux-boot.scm
> > +++ b/gnu/build/linux-boot.scm
> > @@ -507,7 +507,26 @@ to it are lost."
> >               (switch-root "/root")
> >               (format #t "loading '~a'...\n" to-load)
> >  
> > -             (primitive-load to-load)
> > +             (catch #t
> > +               (lambda ()
> > +                 (primitive-load to-load))
> > +               (lambda (key proc format-string format-args . rest)
> > +                 (format (current-error-port) "~a: ~a~%" proc
> > +                  (apply format #f format-string format-args))
> > +                 (force-output (current-error-port))
> > +                 (exit 1)
> > +                 (reboot))  
> 
> The safe way (an exception could have fewer arguments that what’s above)
> is to write something like:

I think it's better if, instead, we give the initrd (at least) a new parameter enter-repl-on-errors?, defaulting to #t, which can be used by marionette-operating-system to disable the REPL.

I tried to do that like this:

diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index 4dd740174..92589e667 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -430,7 +430,8 @@ bailing out.~%root contents: ~s~%" (scandir "/"))
                       qemu-guest-networking?
                       volatile-root?
                       pre-mount
-                      (mounts '()))
+                      (mounts '())
+                      (enter-repl-on-error? #t))
   "This procedure is meant to be called from an initrd.  Boot a system by
 first loading LINUX-MODULES (a list of module names) from
 LINUX-MODULE-DIRECTORY, then setting up QEMU guest networking if
@@ -459,10 +460,28 @@ to it are lost."
     (string-append linux-module-directory "/"
                    (ensure-dot-ko name)))
 
+  (define (call-with-error-handling* thunk)
+    (if enter-repl-on-error?
+      (call-with-error-handling thunk)
+      (catch #t
+        (call-with-error-handling thunk)
+        (const #t)
+        (lambda (key . args)
+          (let ((print-frames
+                 (@@ (system repl debug) print-frames))
+                (frame->stack-vector
+                 (@@ (system repl debug) frame->stack-vector))
+                (stack-ref
+                 (@@ (system repl debug) stack-ref)))
+                (print-frames
+                 (frame->stack-vector
+                  (stack-ref (make-stack #t) 0))
+                  #:width 1000000)
+                (exit 1))))))
   (display "Welcome, this is GNU's early boot Guile.\n")
   (display "Use '--repl' for an initrd REPL.\n\n")
 
-  (call-with-error-handling
+  (call-with-error-handling*
    (lambda ()
      (mount-essential-file-systems)
      (let* ((args    (linux-command-line))
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index b592defa4..4da219db3 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -155,7 +155,8 @@ MODULES and taken from LINUX."
                       (mapped-devices '())
                       (helper-packages '())
                       qemu-networking?
-                      volatile-root?)
+                      volatile-root?
+                      (enter-repl-on-error? #t))
   "Return a monadic derivation that builds a raw initrd, with kernel
 modules taken from LINUX.  FILE-SYSTEMS is a list of file-systems to be
 mounted by the initrd, possibly in addition to the root file system specified
@@ -208,6 +209,8 @@ to it are lost."
              (set-path-environment-variable "PATH" '("bin" "sbin")
                                             '#$helper-packages)))
 
+         (display "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX")
+         (display enter-repl-on-error?)
          (boot-system #:mounts
                       (map spec->file-system
                            '#$(map file-system->spec file-systems))
@@ -216,7 +219,8 @@ to it are lost."
                       #:linux-modules '#$linux-modules
                       #:linux-module-directory '#$kodir
                       #:qemu-guest-networking? #$qemu-networking?
-                      #:volatile-root? '#$volatile-root?)))
+                      #:volatile-root? '#$volatile-root?
+                      #:enter-repl-on-error? #$enter-repl-on-error?)))
    #:name "raw-initrd"))
 
 (define* (file-system-packages file-systems #:key (volatile-root? #f))
@@ -242,6 +246,7 @@ FILE-SYSTEMS."
                       (mapped-devices '())
                       qemu-networking?
                       volatile-root?
+                      (enter-repl-on-error? #t)
                       (virtio? #t)
                       (extra-modules '()))
   "Return a monadic derivation that builds a generic initrd, with kernel
@@ -312,12 +317,16 @@ loaded at boot time in the order in which they appear."
   (define helper-packages
     (file-system-packages file-systems #:volatile-root? volatile-root?))
 
+         (display "YYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY")
+         (display enter-repl-on-error?)
+
   (raw-initrd file-systems
               #:linux linux
               #:linux-modules linux-modules
               #:mapped-devices mapped-devices
               #:helper-packages helper-packages
               #:qemu-networking? qemu-networking?
-              #:volatile-root? volatile-root?))
+              #:volatile-root? volatile-root?
+              #:enter-repl-on-error? enter-repl-on-error?))
 
 ;;; linux-initrd.scm ends here
diff --git a/gnu/tests.scm b/gnu/tests.scm
index 0caa922fd..1853bc269 100644
--- a/gnu/tests.scm
+++ b/gnu/tests.scm
@@ -172,6 +172,10 @@ marionette service in the guest is started after the Shepherd services listed
 in REQUIREMENTS."
   (operating-system
     (inherit os)
+    (initrd (lambda (fs . rest)
+              (apply (operating-system-initrd os) fs
+                     #:enter-repl-on-error? #f
+                     rest)))
     (services (cons (service marionette-service-type
                              (marionette-configuration
                               (requirements requirements)


But I get a resolve-variable error about enter-repl-on-error.  Why?

> > --- a/gnu/system.scm
> > +++ b/gnu/system.scm
> > @@ -73,6 +73,7 @@
> >              operating-system-hosts-file
> >              operating-system-kernel
> >              operating-system-kernel-file
> > +            operating-system-user-kernel-arguments
> >              operating-system-kernel-arguments
> >              operating-system-initrd
> >              operating-system-users
> > diff --git a/gnu/tests.scm b/gnu/tests.scm
> > index 0caa922fd..dc0188f1e 100644
> > --- a/gnu/tests.scm
> > +++ b/gnu/tests.scm
> > @@ -172,6 +172,9 @@ marionette service in the guest is started after the Shepherd services listed
> >  in REQUIREMENTS."
> >    (operating-system
> >      (inherit os)
> > +    ;; Make sure that the system doesn't hang on kernel panic.
> > +    (kernel-arguments (cons "panic=1"
> > +                            (operating-system-user-kernel-arguments os)))
> >      (services (cons (service marionette-service-type
> >                               (marionette-configuration
> >                                (requirements requirements)  
> 
> That’s a good idea.  Could you make it a separate commit?

Sure, okay.

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Ludovic Courtès]

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

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> I think it's better if, instead, we give the initrd (at least) a new
> parameter enter-repl-on-errors?, defaulting to #t,

‘call-with-error-handling’ already has #:on-debug, so I figured we could
simply expose that:


[-- Attachment #2: Type: text/x-patch, Size: 4772 bytes --]

diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index 4dd740174..997107a67 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -430,7 +430,8 @@ bailing out.~%root contents: ~s~%" (scandir "/"))
                       qemu-guest-networking?
                       volatile-root?
                       pre-mount
-                      (mounts '()))
+                      (mounts '())
+                      (on-error 'debug))
   "This procedure is meant to be called from an initrd.  Boot a system by
 first loading LINUX-MODULES (a list of module names) from
 LINUX-MODULE-DIRECTORY, then setting up QEMU guest networking if
@@ -444,7 +445,10 @@ if any.
 MOUNTS must be a list of <file-system> objects.
 
 When VOLATILE-ROOT? is true, the root file system is writable but any changes
-to it are lost."
+to it are lost.
+
+ON-ERROR is passed to 'call-with-error-handling'; it determines what happens
+upon error."
   (define (root-mount-point? fs)
     (string=? (file-system-mount-point fs) "/"))
 
@@ -517,6 +521,7 @@ to it are lost."
            (begin
              (display "no boot file passed via '--load'\n")
              (display "entering a warm and cozy REPL\n")
-             (start-repl)))))))
+             (start-repl)))))
+   #:on-error on-error))
 
 ;;; linux-initrd.scm ends here
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index b592defa4..0f7f4721d 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2016 Jan Nieuwenhuizen <janneke@gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
@@ -155,7 +155,8 @@ MODULES and taken from LINUX."
                       (mapped-devices '())
                       (helper-packages '())
                       qemu-networking?
-                      volatile-root?)
+                      volatile-root?
+                      (on-error 'debug))
   "Return a monadic derivation that builds a raw initrd, with kernel
 modules taken from LINUX.  FILE-SYSTEMS is a list of file-systems to be
 mounted by the initrd, possibly in addition to the root file system specified
@@ -167,8 +168,12 @@ e2fsck/static or other packages needed by the initrd to check root partition.
 
 When QEMU-NETWORKING? is true, set up networking with the standard QEMU
 parameters.
+
 When VOLATILE-ROOT? is true, the root file system is writable but any changes
-to it are lost."
+to it are lost.
+
+ON-ERROR is passed to 'call-with-error-handling'; it determines what happens
+upon error."
   (define device-mapping-commands
     ;; List of gexps to open the mapped devices.
     (map (lambda (md)
@@ -216,7 +221,8 @@ to it are lost."
                       #:linux-modules '#$linux-modules
                       #:linux-module-directory '#$kodir
                       #:qemu-guest-networking? #$qemu-networking?
-                      #:volatile-root? '#$volatile-root?)))
+                      #:volatile-root? '#$volatile-root?
+                      #:on-error '#$on-error)))
    #:name "raw-initrd"))
 
 (define* (file-system-packages file-systems #:key (volatile-root? #f))
@@ -243,7 +249,8 @@ FILE-SYSTEMS."
                       qemu-networking?
                       volatile-root?
                       (virtio? #t)
-                      (extra-modules '()))
+                      (extra-modules '())
+                      (on-error 'debug))
   "Return a monadic derivation that builds a generic initrd, with kernel
 modules taken from LINUX.  FILE-SYSTEMS is a list of file-systems to be
 mounted by the initrd, possibly in addition to the root file system specified
@@ -318,6 +325,7 @@ loaded at boot time in the order in which they appear."
               #:mapped-devices mapped-devices
               #:helper-packages helper-packages
               #:qemu-networking? qemu-networking?
-              #:volatile-root? volatile-root?))
+              #:volatile-root? volatile-root?
+              #:on-error on-error))
 
 ;;; linux-initrd.scm ends here

[-- Attachment #3: Type: text/plain, Size: 51 bytes --]


I’ll push it if it works for you.

Ludo’.

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Danny Milosavljevic]

Yes, it works!

[bug#29922] [PATCH v2] linux-boot: Improve error reporting.

[Ludovic Courtès]

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> Yes, it works!

Pushed as aeed74f3709446e94a87809ddd465517738921e0, thanks!

I guess you can push the remaining bits?

Ludo’.

[bug#29922] [PATCH 0/2] Make sure that marionette-operating-system fails on error.

[Danny Milosavljevic]

Danny Milosavljevic (2):
  system: Export operating-system-user-kernel-arguments.
  tests: marionette-operating-system: Add initrd parameter and
    kernel-arguments.

 gnu/system.scm | 1 +
 gnu/tests.scm  | 8 ++++++++
 2 files changed, 9 insertions(+)

[bug#29922] [PATCH 1/2] system: Export operating-system-user-kernel-arguments.

[Danny Milosavljevic]

* gnu/system.scm (operating-system-user-kernel-arguments): Export it.
---
 gnu/system.scm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gnu/system.scm b/gnu/system.scm
index df89ca06d..40e259f43 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -90,6 +90,7 @@
             operating-system-activation-script
             operating-system-user-accounts
             operating-system-shepherd-service-names
+            operating-system-user-kernel-arguments
 
             operating-system-derivation
             operating-system-profile

[bug#29922] [PATCH 2/2] tests: marionette-operating-system: Add initrd parameter and kernel-arguments.

[Danny Milosavljevic]

* gnu/tests.scm (marionette-operating-system): Use initrd parameter "on-error"
and kernel-arguments "panic".
---
 gnu/tests.scm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gnu/tests.scm b/gnu/tests.scm
index 0caa922fd..d3d15672b 100644
--- a/gnu/tests.scm
+++ b/gnu/tests.scm
@@ -172,6 +172,14 @@ marionette service in the guest is started after the Shepherd services listed
 in REQUIREMENTS."
   (operating-system
     (inherit os)
+    ;; Make sure the guest dies on failure.
+    (kernel-arguments (cons "panic=1"
+                            (operating-system-user-kernel-arguments os)))
+    ;; Make sure the guest doesn't hang in the REPL on failure.
+    (initrd (lambda (fs . rest)
+              (apply (operating-system-initrd os) fs
+                     #:on-error 'backtrace
+                     rest)))
     (services (cons (service marionette-service-type
                              (marionette-configuration
                               (requirements requirements)

[bug#29922] [PATCH 0/2] Make sure that marionette-operating-system fails on error.

[Ludovic Courtès]

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> Danny Milosavljevic (2):
>   system: Export operating-system-user-kernel-arguments.
>   tests: marionette-operating-system: Add initrd parameter and
>     kernel-arguments.

LGTM, thanks!

Ludo’.