unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [PATCH 1/2] gnu: Add btrfs-progs/static.
@ 2016-11-30 18:36 David Craven
  2016-11-30 18:36 ` [PATCH 2/2] system: Add btrfs file system support David Craven
  2016-12-03 15:15 ` [PATCH 1/2] gnu: Add btrfs-progs/static Ludovic Courtès
  0 siblings, 2 replies; 14+ messages in thread
From: David Craven @ 2016-11-30 18:36 UTC (permalink / raw)
  To: guix-devel

* gnu/packages/linux.scm (btrfs-progs/static): New variable.
---
 gnu/packages/linux.scm | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index a4639bd..8b6cce4 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -2714,6 +2714,36 @@ easy administration.")
     ;; GPL2: Everything else.
     (license (list license:gpl2 license:gpl2+))))
 
+(define-public btrfs-progs/static
+  (package
+    (name "btrfs-progs-static")
+    (version (package-version btrfs-progs))
+    (build-system trivial-build-system)
+    (source #f)
+    (arguments
+     `(#:modules ((guix build utils))
+       #:builder
+       (begin
+         (use-modules (guix build utils)
+                      (ice-9 ftw)
+                      (srfi srfi-26))
+
+         (let* ((btrfs  (assoc-ref %build-inputs "btrfs-progs:static"))
+                (out    (assoc-ref %outputs "out"))
+                (source (string-append btrfs "/bin/btrfs.static"))
+                (target (string-append out "/bin/btrfs")))
+           (mkdir-p (dirname target))
+           (copy-file source target)
+           (remove-store-references target)
+           (chmod target #o555)))))
+    (inputs `(("btrfs-progs:static" ,btrfs-progs "static")))
+    (synopsis "Statically-linked btrfs command from btrfsprogs")
+    (description
+     "This package provides statically-linked command of btrfs taken
+from the btrfsprogs package.  It is meant to be used in initrds.")
+    (home-page (package-home-page btrfs-progs))
+    (license (package-license btrfs-progs))))
+
 (define-public freefall
   (package
     (name "freefall")
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] system: Add btrfs file system support.
  2016-11-30 18:36 [PATCH 1/2] gnu: Add btrfs-progs/static David Craven
@ 2016-11-30 18:36 ` David Craven
  2016-12-01 19:18   ` Marius Bakke
  2016-12-03 15:31   ` Ludovic Courtès
  2016-12-03 15:15 ` [PATCH 1/2] gnu: Add btrfs-progs/static Ludovic Courtès
  1 sibling, 2 replies; 14+ messages in thread
From: David Craven @ 2016-11-30 18:36 UTC (permalink / raw)
  To: guix-devel

* gnu/system/linux-initrd.scm (linux-modules, helper-packages): Add
  btrfs modules when a btrfs file-system is used.
* gnu/build/file-systems.scm (check-file-system-irrecoverable-error,
  check-file-system-ext): New variables.
  (check-file-system): Support non ext file systems gracefully.
---
 gnu/build/file-systems.scm  | 30 ++++++++++++++++++++----------
 gnu/system/linux-initrd.scm | 10 +++++++---
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index 431b287..9f57ee5 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -410,12 +410,15 @@ the following:
     (else
      (error "unknown device title" title))))
 
-(define (check-file-system device type)
-  "Run a file system check of TYPE on DEVICE."
-  (define fsck
-    (string-append "fsck." type))
-
-  (let ((status (system* fsck "-v" "-p" "-C" "0" device)))
+(define (check-file-system-irrecoverable-error prog code device)
+  (format (current-error-port)
+          "'~a' exited with code ~a on ~a; spawning Bourne-like REPL~%"
+          prog code device)
+  (start-repl %bournish-language))
+
+(define (check-file-system-ext device type)
+  (let* ((fsck (string-append "fsck." type))
+         (status (system* fsck "-v" "-p" "-C" "0" device)))
     (match (status:exit-val status)
       (0
        #t)
@@ -428,10 +431,17 @@ the following:
        (sleep 3)
        (reboot))
       (code
-       (format (current-error-port) "'~a' exited with code ~a on ~a; \
-spawning Bourne-like REPL~%"
-               fsck code device)
-       (start-repl %bournish-language)))))
+       (check-file-system-irrecoverable-error fsck code device)))))
+
+(define (check-file-system device type)
+  "Run a file system check of TYPE on DEVICE."
+    (cond
+     ((string-prefix? "ext" type)
+      (check-file-system-ext device type))
+     ((string-prefix? "btrfs" type)
+      (zero? (system* "btrfs" "device" "scan")))
+     (#t (format (current-error-port)
+                 "Don't know how to check '~a' file systems; skipping~%" type))))
 
 (define (mount-flags->bit-mask flags)
   "Return the number suitable for the 'flags' argument of 'mount' that
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 174239a..de8b785 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -193,6 +193,9 @@ loaded at boot time in the order in which they appear."
       ,@(if (find (file-system-type-predicate "9p") file-systems)
             virtio-9p-modules
             '())
+      ,@(if (find (file-system-type-predicate "btrfs") file-systems)
+            '("btrfs")
+            '())
       ,@(if volatile-root?
             '("fuse")
             '())
@@ -200,11 +203,12 @@ loaded at boot time in the order in which they appear."
 
   (define helper-packages
     ;; Packages to be copied on the initrd.
-    `(,@(if (find (lambda (fs)
-                    (string-prefix? "ext" (file-system-type fs)))
-                  file-systems)
+    `(,@(if (find (file-system-type-predicate "ext4") file-systems)
             (list e2fsck/static)
             '())
+      ,@(if (find (file-system-type-predicate "btrfs") file-systems)
+            (list btrfs-progs/static)
+            '())
       ,@(if volatile-root?
             (list unionfs-fuse/static)
             '())))
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-11-30 18:36 ` [PATCH 2/2] system: Add btrfs file system support David Craven
@ 2016-12-01 19:18   ` Marius Bakke
  2016-12-02 10:50     ` David Craven
  2016-12-03 15:31   ` Ludovic Courtès
  1 sibling, 1 reply; 14+ messages in thread
From: Marius Bakke @ 2016-12-01 19:18 UTC (permalink / raw)
  To: David Craven, guix-devel


[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]

David Craven <david@craven.ch> writes:

> * gnu/system/linux-initrd.scm (linux-modules, helper-packages): Add
>   btrfs modules when a btrfs file-system is used.
> * gnu/build/file-systems.scm (check-file-system-irrecoverable-error,
>   check-file-system-ext): New variables.
>   (check-file-system): Support non ext file systems gracefully.

Hi! I submitted a similar patch for fat32 support a while back and Ludo
suggested refactoring the <file-system> object to contain a
'check-procedure'. I got stuck at some point and have been
procrastinating since..

Attached is what I have so far. The biggest problem is that some callers
of 'check-file-system' does not use a <file-system> object, but see also
5970e8e24 which shows how to convert a loose spec to a <file-system>.

I'll pick this back up, but testing and feedback welcome. Currently it
does not work at all :-)


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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-file-systems-Refactor-file-system-to-include-check-p.patch --]
[-- Type: text/x-patch, Size: 7193 bytes --]

From a222eb8781866e2b1dbb715f79acc91378e116c9 Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Tue, 8 Nov 2016 21:33:34 +0000
Subject: [PATCH] file-systems: Refactor <file-system> to include
 check-procedure.

* gnu/system/file-systems.scm (file-system-check-procedure): New
variable.  Extend file-system record to include it.  Export it.
* gnu/build/file-systems.scm (check-file-system): Use it.
(mount-file-system): Serialize spec before calling check-file-system.
* gnu/build/linux-boot.scm: Adjust check-file-system arguments.
* gnu/services/base.scm: Likewise.
* gnu/system/linux-initrd.scm (base-initrd): Remove e2fsck/static from
helper-packages.
---
 gnu/build/file-systems.scm  | 24 +++++++++++-------------
 gnu/build/linux-boot.scm    |  2 +-
 gnu/services/base.scm       |  8 +-------
 gnu/system/file-systems.scm | 17 ++++++++++++++++-
 gnu/system/linux-initrd.scm |  7 +------
 5 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index 0d55e91..e5053f5 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -410,27 +410,25 @@ the following:
     (else
      (error "unknown device title" title))))
 
-(define (check-file-system device type)
-  "Run a file system check of TYPE on DEVICE."
-  (define fsck
-    (string-append "fsck." type))
-
-  (let ((status (system* fsck "-v" "-p" "-C" "0" device)))
+(define (check-file-system file-system)
+  "Run a file system check on FILE-SYSTEM."
+  (let* ((fsck   (file-system-check-procedure file-system))
+         (status (fsck device)))
     (match (status:exit-val status)
       (0
        #t)
       (1
-       (format (current-error-port) "'~a' corrected errors on ~a; continuing~%"
-               fsck device))
+       (format (current-error-port) "'~a' corrected errors; continuing~%"
+               fsck))
       (2
-       (format (current-error-port) "'~a' corrected errors on ~a; rebooting~%"
-               fsck device)
+       (format (current-error-port) "'~a' corrected errors; rebooting~%"
+               fsck)
        (sleep 3)
        (reboot))
       (code
-       (format (current-error-port) "'~a' exited with code ~a on ~a; \
+       (format (current-error-port) "'~a' exited with code ~a; \
 spawning Bourne-like REPL~%"
-               fsck code device)
+               fsck code)
        (start-repl %bournish-language)))))
 
 (define (mount-flags->bit-mask flags)
@@ -470,7 +468,7 @@ run a file system check."
            (mount-point (string-append root "/" mount-point))
            (flags       (mount-flags->bit-mask flags)))
        (when check?
-         (check-file-system source type))
+         (check-file-system (spec->file-system spec)))
 
        ;; Create the mount point.  Most of the time this is a directory, but
        ;; in the case of a bind mount, a regular file may be needed.
diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index c34a3f7..903ce14 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -277,7 +277,7 @@ UNIONFS."
         ;; have to resort to 'pidof' here.
         (mark-as-not-killable (pidof unionfs)))
       (begin
-        (check-file-system root type)
+        (check-file-system root)
         (mount root "/root" type)))
 
   ;; Make sure /root/etc/mtab is a symlink to /proc/self/mounts.
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index afbecdb..2c18e0a 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -273,13 +273,7 @@ FILE-SYSTEM."
                                #~#t)
                          #$(if check?
                                #~(begin
-                                   ;; Make sure fsck.ext2 & co. can be found.
-                                   (setenv "PATH"
-                                           (string-append
-                                            #$e2fsprogs "/sbin:"
-                                            "/run/current-system/profile/sbin:"
-                                            (getenv "PATH")))
-                                   (check-file-system device #$type))
+                                   (check-file-system file-system))
                                #~#t)
 
                          (mount device #$target #$type flags
diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
index 4cc1221..58e7bad 100644
--- a/gnu/system/file-systems.scm
+++ b/gnu/system/file-systems.scm
@@ -18,8 +18,10 @@
 
 (define-module (gnu system file-systems)
   #:use-module (ice-9 match)
+  #:use-module (guix gexp)
   #:use-module (guix records)
   #:use-module (guix store)
+  #:use-module ((gnu packages linux) #:select (e2fsck/static))
   #:use-module ((gnu build file-systems)
                 #:select (string->uuid uuid->string))
   #:re-export (string->uuid
@@ -36,6 +38,7 @@
             file-system-options
             file-system-mount?
             file-system-check?
+            file-system-check-procedure
             file-system-create-mount-point?
             file-system-dependencies
 
@@ -90,6 +93,8 @@
                     (default #f))
   (check?           file-system-check?            ; Boolean
                     (default #t))
+  (check-procedure  file-system-check-procedure   ; Gexp or #f
+                    (default #f))
   (create-mount-point? file-system-create-mount-point? ; Boolean
                        (default #f))
   (dependencies     file-system-dependencies      ; list of <file-system>
@@ -105,7 +110,7 @@ file system."
   "Return a list corresponding to file-system FS that can be passed to the
 initrd code."
   (match fs
-    (($ <file-system> device title mount-point type flags options _ _ check?)
+    (($ <file-system> device title mount-point type flags options _ _ check? _)
      (list device title mount-point type flags options check?))))
 
 (define (spec->file-system sexp)
@@ -135,6 +140,16 @@ TARGET in the other system."
          (target spec)
          (writable? writable?)))))
 
+(define (file-system-check-procedure fs)
+  "Return an fsck command corresponding to file-system FS."
+  (let ((type   (file-system-type fs))
+        (device (file-system-device fs)))
+    (cond
+     ((string-prefix? "ext" type)
+      #~(system* #$(file-append e2fsck/static "/sbin/fsck." type)
+                 "-v" "-p" "-C" "0" device))
+     (else #~(system* (string-append "fsck." type) device)))))
+
 (define-syntax uuid
   (lambda (s)
     "Return the bytevector corresponding to the given UUID representation."
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 174239a..d4b8e45 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -200,12 +200,7 @@ loaded at boot time in the order in which they appear."
 
   (define helper-packages
     ;; Packages to be copied on the initrd.
-    `(,@(if (find (lambda (fs)
-                    (string-prefix? "ext" (file-system-type fs)))
-                  file-systems)
-            (list e2fsck/static)
-            '())
-      ,@(if volatile-root?
+    `(,@(if volatile-root?
             (list unionfs-fuse/static)
             '())))
 
-- 
2.10.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-01 19:18   ` Marius Bakke
@ 2016-12-02 10:50     ` David Craven
  2016-12-02 11:12       ` Chris Marusich
  2016-12-03 15:18       ` Ludovic Courtès
  0 siblings, 2 replies; 14+ messages in thread
From: David Craven @ 2016-12-02 10:50 UTC (permalink / raw)
  To: Marius Bakke; +Cc: guix-devel

I looked at your improvement suggestion. One issue I'm thinking about:

How to handle the return codes, since the fsck.vfat returns slightly
different error codes than fsck.ext. For example fsck.vfat error code
2 is a usage error, which would cause an infinite reboot cycle if the
passed device is misspelled or something. The message corrected
errors; rebooting could cause people quite some frustration I can
imagine, when the problem is something that is likely a trivial typo
or something.

Then there is the label and uuid detection logic for every file-system
that needs to be implemented. I'm wondering if it can be refactored, a
quick late night and untested implementation for btrfs suggests that
there is a lot of code repetition involved, I guess the same will be
true for vfat. I'll have to check if there are any tests for this
anywhere or write some before I mess with this "core piece" of guix.

Ludo: Is there any way we can speed up the installer system tests? :)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-02 10:50     ` David Craven
@ 2016-12-02 11:12       ` Chris Marusich
  2016-12-02 16:27         ` David Craven
  2016-12-03 15:21         ` Ludovic Courtès
  2016-12-03 15:18       ` Ludovic Courtès
  1 sibling, 2 replies; 14+ messages in thread
From: Chris Marusich @ 2016-12-02 11:12 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

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

David Craven <david@craven.ch> writes:

> Ludo: Is there any way we can speed up the installer system tests? :)

I've also wondered about this.  I'm not sure how long each test takes to
run, so an analysis of the tests to find their slow areas might help
guide us.  I suspect that we could probably speed up the tests by using
a shared test fixture of some kind (e.g., a common store, or a base qemu
disk image [1], which could be used by multiple tests), but first we'd
need to know which part(s) are slow.

Another possibility might be to parallelize the tests (I don't think the
system tests are done in parallel right now, even if you specify "make
-j", but someone correct me if I'm wrong), but that could be
counter-productive if it took up too many system resources.

For now, if you've got a specific test you want to exercise repeatedly,
you can run just that one test (e.g., "make check-system TESTS="basic
mcron").  That should speed things up if one test is all you need.

[1]  https://en.wikibooks.org/wiki/QEMU/Images#Copy_on_write

-- 
Chris

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-02 11:12       ` Chris Marusich
@ 2016-12-02 16:27         ` David Craven
  2016-12-03 15:21         ` Ludovic Courtès
  1 sibling, 0 replies; 14+ messages in thread
From: David Craven @ 2016-12-02 16:27 UTC (permalink / raw)
  To: Chris Marusich; +Cc: guix-devel

> How to handle the return codes.

This isn't a problem, the check-procedure can normalize the return codes. The
confusing thing is this part:

(else #~(system* (string-append "fsck." type) device)

The default behavior should be to emit a warning to stderr and do nothing. I
don't think this produces anything useful for any file system...

> I've also wondered about this.  I'm not sure how long each test takes to
> run, so an analysis of the tests to find their slow areas might help
> guide us.

I noticed that the system tests in general are fast. It's only the
installer tests that take so long.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gnu: Add btrfs-progs/static.
  2016-11-30 18:36 [PATCH 1/2] gnu: Add btrfs-progs/static David Craven
  2016-11-30 18:36 ` [PATCH 2/2] system: Add btrfs file system support David Craven
@ 2016-12-03 15:15 ` Ludovic Courtès
  2016-12-03 21:41   ` David Craven
  1 sibling, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-03 15:15 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

David Craven <david@craven.ch> skribis:

> * gnu/packages/linux.scm (btrfs-progs/static): New variable.

[...]

> +(define-public btrfs-progs/static
> +  (package
> +    (name "btrfs-progs-static")
> +    (version (package-version btrfs-progs))
> +    (build-system trivial-build-system)
> +    (source #f)
> +    (arguments
> +     `(#:modules ((guix build utils))

Add #:disallowed-references () to make sure you get the desired effect.

OK with this change, thanks!

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-02 10:50     ` David Craven
  2016-12-02 11:12       ` Chris Marusich
@ 2016-12-03 15:18       ` Ludovic Courtès
  1 sibling, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-03 15:18 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

David Craven <david@craven.ch> skribis:

> Then there is the label and uuid detection logic for every file-system
> that needs to be implemented. I'm wondering if it can be refactored, a
> quick late night and untested implementation for btrfs suggests that
> there is a lot of code repetition involved, I guess the same will be
> true for vfat. I'll have to check if there are any tests for this
> anywhere or write some before I mess with this "core piece" of guix.

This is covered by tests like “basic” and other lightweight tests: they
boot the system, so they get to exercise the file system label/UUID
lookup code (if the root partition is specified by a label/UUID at
least).

> Ludo: Is there any way we can speed up the installer system tests? :)

Most of the time is spent in building the latest Guix and running ‘guix
system init’.  I don’t think this can be sped up.

The QEMU images involved are copied around, and maybe that part can be
slightly optimized.  I did that in
130079ae27b47228516dc2934bcdecca5dbedf12.

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-02 11:12       ` Chris Marusich
  2016-12-02 16:27         ` David Craven
@ 2016-12-03 15:21         ` Ludovic Courtès
  1 sibling, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-03 15:21 UTC (permalink / raw)
  To: Chris Marusich; +Cc: guix-devel

Chris Marusich <cmmarusich@gmail.com> skribis:

> David Craven <david@craven.ch> writes:
>
>> Ludo: Is there any way we can speed up the installer system tests? :)
>
> I've also wondered about this.  I'm not sure how long each test takes to
> run, so an analysis of the tests to find their slow areas might help
> guide us.  I suspect that we could probably speed up the tests by using
> a shared test fixture of some kind (e.g., a common store, or a base qemu
> disk image [1], which could be used by multiple tests), but first we'd
> need to know which part(s) are slow.

Tests that use ‘system-qemu-image/shared-store-script’ (basic, mcron,
openssh, etc.) are relatively fast.

However, the installation tests have to use a real (full, standalone)
image for the installation disk, and also for the target disk.
Otherwise we wouldn’t be testing the same thing.

> Another possibility might be to parallelize the tests (I don't think the
> system tests are done in parallel right now, even if you specify "make
> -j", but someone correct me if I'm wrong), but that could be
> counter-productive if it took up too many system resources.

They are derivation builds, so they are performed in parallel according
to --max-jobs.

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-11-30 18:36 ` [PATCH 2/2] system: Add btrfs file system support David Craven
  2016-12-01 19:18   ` Marius Bakke
@ 2016-12-03 15:31   ` Ludovic Courtès
  2016-12-03 16:21     ` David Craven
  1 sibling, 1 reply; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-03 15:31 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

David Craven <david@craven.ch> skribis:

> * gnu/system/linux-initrd.scm (linux-modules, helper-packages): Add
>   btrfs modules when a btrfs file-system is used.
> * gnu/build/file-systems.scm (check-file-system-irrecoverable-error,
>   check-file-system-ext): New variables.
>   (check-file-system): Support non ext file systems gracefully.

[...]

> -(define (check-file-system device type)
> -  "Run a file system check of TYPE on DEVICE."
> -  (define fsck
> -    (string-append "fsck." type))
> -
> -  (let ((status (system* fsck "-v" "-p" "-C" "0" device)))
> +(define (check-file-system-irrecoverable-error prog code device)
> +  (format (current-error-port)
> +          "'~a' exited with code ~a on ~a; spawning Bourne-like REPL~%"
> +          prog code device)
> +  (start-repl %bournish-language))

This is confusing because this procedure doesn’t check anything, despite
the name.  :-)

> +(define (check-file-system-ext device type)
> +  (let* ((fsck (string-append "fsck." type))
> +         (status (system* fsck "-v" "-p" "-C" "0" device)))
>      (match (status:exit-val status)
>        (0
>         #t)

I think I made a different suggestion to Marius, so I may well be
contradicting myself now, but what comes to mind now is to define a list
of “checkers” corresponding to each file system type:

  (define-record-type <file-system-checker>
    (file-system-checker predicate checker)
    file-system-checker?
    (predicate  file-system-checker-predicate)
    (checker    file-system-checker-procedure))

Then we can have an alist:

  (define %ext2-checker
    (file-system-checker (cut string-prefix? "ext" <>)
                         (lambda (device type)
                           …)))

  (define %file-system-checkers
    (list %ext2-checker %vfat-checker %btrfs-checker))

Each checker procedure would return an enum (either using (rnrs enums)
or returning a symbol like 'pass, 'errors-corrected, or 'reboot-needed).

The code to start the REPL would be outside the checkers themselves, in
the generic ‘check-file-system’.

WDYT?

> --- a/gnu/system/linux-initrd.scm
> +++ b/gnu/system/linux-initrd.scm
> @@ -193,6 +193,9 @@ loaded at boot time in the order in which they appear."
>        ,@(if (find (file-system-type-predicate "9p") file-systems)
>              virtio-9p-modules
>              '())
> +      ,@(if (find (file-system-type-predicate "btrfs") file-systems)
> +            '("btrfs")
> +            '())
>        ,@(if volatile-root?
>              '("fuse")
>              '())
> @@ -200,11 +203,12 @@ loaded at boot time in the order in which they appear."
>  
>    (define helper-packages
>      ;; Packages to be copied on the initrd.
> -    `(,@(if (find (lambda (fs)
> -                    (string-prefix? "ext" (file-system-type fs)))
> -                  file-systems)
> +    `(,@(if (find (file-system-type-predicate "ext4") file-systems)
>              (list e2fsck/static)
>              '())
> +      ,@(if (find (file-system-type-predicate "btrfs") file-systems)
> +            (list btrfs-progs/static)
> +            '())
>        ,@(if volatile-root?
>              (list unionfs-fuse/static)
>              '())))

This part LGTM.

Thanks!

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-03 15:31   ` Ludovic Courtès
@ 2016-12-03 16:21     ` David Craven
  2016-12-05 20:44       ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: David Craven @ 2016-12-03 16:21 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

The nice thing about having the check procedure be part of the
<file-system> is that it can be overridden. I'm not sure what use
cases there are yet. One I can think of is that btrfs device scan is
only required when using a multi-device configuration like raid. So I
don't know if we want to run it by default as a %btrfs-checker or do
nothing by default, and let the user add a custom file system
pre-mount hook (file-system-checker) when needed.

A version of Marius'es patch that works would be here:
http://lists.gnu.org/archive/html/guix-devel/2016-12/msg00087.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gnu: Add btrfs-progs/static.
  2016-12-03 15:15 ` [PATCH 1/2] gnu: Add btrfs-progs/static Ludovic Courtès
@ 2016-12-03 21:41   ` David Craven
  2016-12-05 20:51     ` Ludovic Courtès
  0 siblings, 1 reply; 14+ messages in thread
From: David Craven @ 2016-12-03 21:41 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

> Add #:allowed-references () to make sure you get the desired effect.

Isn't this a key of the gnu-build-system?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] system: Add btrfs file system support.
  2016-12-03 16:21     ` David Craven
@ 2016-12-05 20:44       ` Ludovic Courtès
  0 siblings, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-05 20:44 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

David Craven <david@craven.ch> skribis:

> The nice thing about having the check procedure be part of the
> <file-system> is that it can be overridden. I'm not sure what use
> cases there are yet. One I can think of is that btrfs device scan is
> only required when using a multi-device configuration like raid. So I
> don't know if we want to run it by default as a %btrfs-checker or do
> nothing by default, and let the user add a custom file system
> pre-mount hook (file-system-checker) when needed.

I don’t know enough about btrfs.  Is it really useful in practice to
have a check procedure that is specific to the instance at hand?

For the other FS types, the check method really belongs in the FS type,
not in the FS instance.  That is, all ext4 file systems are checked in
the same way.  Thus, it doesn’t seem natural to store the fsck method in
the <file-system> object itself.

There’s also a practical issue with that: users would have to specify
the fsck method for every single <file-system> object:

  (list (file-system
          ;; …
          (type "ext4")
          (fsck %ext4-fsck))
       (file-system
          ;; …
          (type "ext4")
          (fsck %ext4-fsck))   ;again
       (file-system
          ;; …
          (type "ext4")
          (fsck %ext4-fsck)))  ;and again!  :-)

> A version of Marius'es patch that works would be here:
> http://lists.gnu.org/archive/html/guix-devel/2016-12/msg00087.html

OK, this patch doesn’t have the problem above, but it’s kinda cheating.
:-)

I’ll reply separately!

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] gnu: Add btrfs-progs/static.
  2016-12-03 21:41   ` David Craven
@ 2016-12-05 20:51     ` Ludovic Courtès
  0 siblings, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2016-12-05 20:51 UTC (permalink / raw)
  To: David Craven; +Cc: guix-devel

David Craven <david@craven.ch> skribis:

>> Add #:allowed-references () to make sure you get the desired effect.
>
> Isn't this a key of the gnu-build-system?

Oh indeed (and this is ridiculous).  Sorry for the confusion.

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-12-05 20:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-30 18:36 [PATCH 1/2] gnu: Add btrfs-progs/static David Craven
2016-11-30 18:36 ` [PATCH 2/2] system: Add btrfs file system support David Craven
2016-12-01 19:18   ` Marius Bakke
2016-12-02 10:50     ` David Craven
2016-12-02 11:12       ` Chris Marusich
2016-12-02 16:27         ` David Craven
2016-12-03 15:21         ` Ludovic Courtès
2016-12-03 15:18       ` Ludovic Courtès
2016-12-03 15:31   ` Ludovic Courtès
2016-12-03 16:21     ` David Craven
2016-12-05 20:44       ` Ludovic Courtès
2016-12-03 15:15 ` [PATCH 1/2] gnu: Add btrfs-progs/static Ludovic Courtès
2016-12-03 21:41   ` David Craven
2016-12-05 20:51     ` Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).