[PATCH 0/4] Isolated inferiors.

OpenSubmitted by Christopher Baines.
Details
2 participants
  • Ludovic Courtès
  • Christopher Baines
Owner
unassigned
Severity
normal
C
C
Christopher Baines wrote on 24 Feb 2019 17:12
(address . guix-patches@gnu.org)
875zt9go87.fsf@cbaines.net
These patches form a prototype for Guix inferiors, that are
isolated. Access to the inferior Guix is done through running a REPL as
a separate process. These patches provide a way of launching that REPL
in an isolated environment through Linux namespaces, providing some
isolation from the wider system.

These patches should work, at least enough to get the derivations for
packages within the inferior Guix, as well as doing 'guix pull' within
the inferior Guix.

They're not ready to be merged just yet though. I think some of the
approaches are a little odd (e.g. using (ice-9 popen) internals) and
I've got no idea if the isolation is actually working properly.


Christopher Baines (4):
utils: Add #:base-directory to call-with-temporary-directory.
linux-container: Add 'start-child-in-container'.
inferior: Add a shared-directory field to <inferior>
inferior: Add 'open-inferior/container'.

gnu/build/linux-container.scm | 82 +++++++++++++++++++++++++++++++
guix/inferior.scm | 90 ++++++++++++++++++++++++++++++-----
guix/utils.scm | 4 +-
3 files changed, 163 insertions(+), 13 deletions(-)
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlxywlhfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9XfDjg//W/v3E1k3KSBgq/cbEB3eDzShPBdOfQjXsm+8NtdZUtmifuzomzQPbSRb
MdtFZzv1u91baWGc7OcArUujtDb7BJJFOdPLbaU+X5ypEsvdNHdndJTuPSefQS4V
rAmbJFLi7vdgqguO8kp5UqT/mHoLDqNPcvCMYQFrlpF6hu1nIHGxtKyTX7TmGVOD
amLBFkHr1IF47Fy8+gahkz50jvW5bvc2kyUwXGFUU4xfB/shgKHuq7tZQDHbZwrz
IwHEBT2Db4g6bJYb5XP0MpqBLaN9CSCCjaNUTo7Y4rxDxCiiOfkwLZtdugt1ghH7
RnYApd9SMNf90VSjVJithX8Y/FtTCOsh+DVqPwEW0fVIDM0XZJ1a7V7JE3WKAy9+
sNMX0AF4o9VxSSJXupAYb/vqXD3DhctY17VszZDVimkuyAvb3IAipdMiRe5rbQ0O
8SSFXxmvQ+eQsSQ5YF5oq462DZmJ9yhkEXApIS3bwhWXqZw9gzxX8IPUWfAobVtc
CpSYhsNDwyz5h0Iult+9rovwDBWu4DtsmRs7L1tykbvSNWOWhgFAqpTf+lx2V6J/
F/XQe6dFuy26c3vH0xVbjSZsWRiqhBXBsRyHtiijI8ctZ5w2fbkzKH3F04st3yRv
ARqN+r66Zi207Y5HiSVGxZiYReFoS9i+RL9IJWcxdcMsPJDm0Dk=
=PrMq
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 24 Feb 2019 17:18
[PATCH 1/4] utils: Add #:base-directory to call-with-temporary-directory.
(address . 34638@debbugs.gnu.org)
20190224161855.2632-1-mail@cbaines.net
This allows more easily creating temporary directories within a specific
directory. This is motivated by using this in inferior-eval-with-store.

* guix/utils.scm (call-with-temporary-directory): Add optional keyword
argument, base-directory.
---
guix/utils.scm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Toggle diff (19 lines)
diff --git a/guix/utils.scm b/guix/utils.scm
index ed1a418cca..abeb156f40 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -620,10 +620,10 @@ call."
         (false-if-exception (close out))
         (false-if-exception (delete-file template))))))
 
-(define (call-with-temporary-directory proc)
+(define* (call-with-temporary-directory proc #:key base-directory)
   "Call PROC with a name of a temporary directory; close the directory and
 delete it when leaving the dynamic extent of this call."
-  (let* ((directory (or (getenv "TMPDIR") "/tmp"))
+  (let* ((directory (or base-directory (getenv "TMPDIR") "/tmp"))
          (template  (string-append directory "/guix-directory.XXXXXX"))
          (tmp-dir   (mkdtemp! template)))
     (dynamic-wind
-- 
2.20.1
C
C
Christopher Baines wrote on 24 Feb 2019 17:18
[PATCH 3/4] inferior: Add a shared-directory field to <inferior>
(address . 34638@debbugs.gnu.org)
20190224161855.2632-3-mail@cbaines.net
---
guix/inferior.scm | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)

Toggle diff (67 lines)
diff --git a/guix/inferior.scm b/guix/inferior.scm
index 027418a98d..cf72454426 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -97,14 +97,15 @@
 
 ;; Inferior Guix process.
 (define-record-type <inferior>
-  (inferior pid socket close version packages table)
+  (inferior pid socket close shared-directory version packages table)
   inferior?
-  (pid      inferior-pid)
-  (socket   inferior-socket)
-  (close    inferior-close-socket)               ;procedure
-  (version  inferior-version)                    ;REPL protocol version
-  (packages inferior-package-promise)            ;promise of inferior packages
-  (table    inferior-package-table))             ;promise of vhash
+  (pid              inferior-pid)
+  (socket           inferior-socket)
+  (close            inferior-close-socket)       ;procedure
+  (shared-directory inferior-shared-directory)
+  (version          inferior-version)            ;REPL protocol version
+  (packages         inferior-package-promise)    ;promise of inferior packages
+  (table            inferior-package-table))     ;promise of vhash
 
 (define (inferior-pipe directory command)
   "Return an input/output pipe on the Guix instance in DIRECTORY.  This runs
@@ -136,7 +137,7 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
-(define* (port->inferior pipe #:optional (close close-port))
+(define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
 inferior."
@@ -144,7 +145,8 @@ inferior."
 
   (match (read pipe)
     (('repl-version 0 rest ...)
-     (letrec ((result (inferior 'pipe pipe close (cons 0 rest)
+     (letrec ((result (inferior 'pipe pipe close shared-directory
+                                (cons 0 rest)
                                 (delay (%inferior-packages result))
                                 (delay (%inferior-package-table result)))))
        (inferior-eval '(use-modules (guix)) result)
@@ -162,7 +164,7 @@ equivalent.  Return #f if the inferior could not be launched."
   (define pipe
     (inferior-pipe directory command))
 
-  (port->inferior pipe close-pipe))
+  (port->inferior pipe #f close-pipe))
 
 (define (close-inferior inferior)
   "Close INFERIOR."
@@ -479,7 +481,8 @@ thus be the code of a one-argument procedure that accepts a store."
          ((client . address)
           (proxy client (store-connection-socket store))))
        (close-port socket)
-       (read-inferior-response inferior)))))
+       (read-inferior-response inferior)))
+   #:base-directory (inferior-shared-directory inferior)))
 
 (define* (inferior-package-derivation store package
                                       #:optional
-- 
2.20.1
C
C
Christopher Baines wrote on 24 Feb 2019 17:18
[PATCH 4/4] inferior: Add 'open-inferior/container'.
(address . 34638@debbugs.gnu.org)
20190224161855.2632-4-mail@cbaines.net
---
guix/inferior.scm | 65 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)

Toggle diff (92 lines)
diff --git a/guix/inferior.scm b/guix/inferior.scm
index cf72454426..a5f773c147 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -40,6 +40,9 @@
   #:use-module (guix store)
   #:use-module (guix derivations)
   #:use-module (guix base32)
+  #:use-module (gnu system file-systems)
+  #:use-module (gnu build linux-container)
+  #:use-module (guix build syscalls)
   #:use-module (gcrypt hash)
   #:autoload   (guix cache) (maybe-remove-expired-cache-entries)
   #:autoload   (guix ui) (show-what-to-build*)
@@ -54,6 +57,7 @@
   #:use-module ((rnrs bytevectors) #:select (string->utf8))
   #:export (inferior?
             open-inferior
+            open-inferior/container
             port->inferior
             close-inferior
             inferior-eval
@@ -137,6 +141,67 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
+(define* (open-inferior/container store guix-store-item
+                                  #:key
+                                  (command "bin/guix")
+                                  (share-host-network? #f)
+                                  (extra-shared-directories '())
+                                  (extra-environment-variables '()))
+  (define requisite-store-items
+    (requisites store (list guix-store-item)))
+
+  (define shared-directory
+    (mkdtemp! (string-append (or (getenv "TMPDIR") "/tmp")
+                             "/guix-inferior.XXXXXX")))
+
+  (define mappings
+    (append
+     (map (lambda (dir)
+            (file-system-mapping
+             (source dir)
+             (target dir)
+             (writable? #f)))
+          `(;; Share a directory, used in inferior-eval-with-store
+            ,shared-directory
+            ,@requisite-store-items
+            ,@extra-shared-directories))
+     (if share-host-network?
+         %network-file-mappings
+         '())))
+
+  (define mounts
+    (append %container-file-systems
+            (map file-system-mapping->bind-mount
+                 mappings)))
+
+  (define (inferior-pipe/container store
+                                   guix-store-item
+                                   shared-directory
+                                   command)
+    (start-child-in-container
+     (list (string-append guix-store-item "/bin/guix")
+           ;; TODO I'm not sure why "repl" is duplicated in the following
+           ;; command
+           "repl" "repl" "-t" "machine")
+     #:read? #t
+     #:write? #t
+     #:mounts mounts
+     #:namespaces (if share-host-network?
+                      (delq 'net %namespaces)
+                      %namespaces)
+     #:extra-environment-variables
+     `(;; Set HOME, so that the (guix profiles) module can be loaded, without it
+       ;; trying to read from /etc/passwd
+       "HOME=/tmp"
+       ,@extra-environment-variables)))
+
+  (port->inferior (inferior-pipe/container store
+                                           guix-store-item
+                                           shared-directory
+                                           command)
+                  shared-directory
+                  close-pipe))
+
 (define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
-- 
2.20.1
C
C
Christopher Baines wrote on 24 Feb 2019 17:18
[PATCH 2/4] linux-container: Add 'start-child-in-container'.
(address . 34638@debbugs.gnu.org)
20190224161855.2632-2-mail@cbaines.net
This new procedure is similar to open-pipe* in (ice-9 popen), but using
run-container from (gnu build linux-container).

* gnu/build/linux-container.scm (start-child-in-container): New procedure.
---
gnu/build/linux-container.scm | 82 +++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)

Toggle diff (102 lines)
diff --git a/gnu/build/linux-container.scm b/gnu/build/linux-container.scm
index 65e1325577..63c83902e4 100644
--- a/gnu/build/linux-container.scm
+++ b/gnu/build/linux-container.scm
@@ -32,6 +32,7 @@
             setgroups-supported?
             %namespaces
             run-container
+            start-child-in-container
             call-with-container
             container-excursion
             container-excursion*))
@@ -210,6 +211,87 @@ corresponds to the symbols in NAMESPACES."
                ('net  CLONE_NEWNET))
               namespaces)))
 
+(define* (start-child-in-container command
+                                   #:key read? write?
+                                   (root 'temporary)
+                                   (mounts '())
+                                   (namespaces %namespaces)
+                                   (host-uids 1)
+                                   (extra-environment-variables '()))
+  (define (with-root-directory f)
+    (if (eq? root 'temporary)
+        (call-with-temporary-directory f)
+        (f root)))
+
+  ;; (ice-9 popen) internals
+  (define make-rw-port (@@ (ice-9 popen) make-rw-port))
+  (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
+  (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))
+
+  ;; car is the inport port, cdr is the output port. You write to the output
+  ;; port, and read from the input port.
+  (define child-to-parent-pipe
+    (if read?
+        (pipe)
+        #f))
+
+  (define parent-to-child-pipe
+    (if write?
+        (pipe)
+        #f))
+
+  (define (run-program)
+    (when read?
+      (match child-to-parent-pipe
+        ((input-port . output-port)
+         ;; close the output part of the child-to-parent-pipe, as this is used
+         ;; by the parent process
+         (close-port input-port)
+
+         ;; Make the input part of the child-to-parent-pipe the standard
+         ;; output of this process
+         (dup2 (fileno output-port) 1))))
+
+    (when write?
+      (match parent-to-child-pipe
+        ((input-port . output-port)
+         ;; close the input part of the parent-to-child-pipe, as this is used
+         ;; by the parent processs
+         (close-port output-port)
+
+         ;; Make the output part of the parent-to-child-pipe the standard
+         ;; input of this process
+         (dup2 (fileno input-port) 0))))
+
+    ;; TODO Maybe close all file descriptors, as start_child in Guile does?
+
+    (for-each putenv extra-environment-variables)
+
+    (apply execlp command))
+
+  (with-root-directory
+   (lambda (root)
+     (let ((pid (run-container root mounts namespaces host-uids run-program)))
+       ;; Catch SIGINT and kill the container process.
+       (sigaction SIGINT
+         (lambda (signum)
+           (false-if-exception
+            (kill pid SIGKILL))))
+
+       (let* ((read-port (and=> child-to-parent-pipe car))
+              (write-port (and=> parent-to-child-pipe cdr))
+
+              (port (or (and read-port write-port
+                             (make-rw-port read-port write-port))
+                        read-port
+                        write-port))
+              (pipe-info (make-pipe-info pid)))
+
+         (pipe-guardian pipe-info)
+         (%set-port-property! port 'popen-pipe-info pipe-info)
+
+         port)))))
+
 (define (run-container root mounts namespaces host-uids thunk)
   "Run THUNK in a new container process and return its PID.  ROOT specifies
 the root directory for the container.  MOUNTS is a list of <file-system>
-- 
2.20.1
L
L
Ludovic Courtès wrote on 14 Mar 2019 20:35
Re: [bug#34638] [PATCH 0/4] Isolated inferiors.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
878sxhi6zq.fsf@gnu.org
Hello!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (10 lines)
> These patches form a prototype for Guix inferiors, that are
> isolated. Access to the inferior Guix is done through running a REPL as
> a separate process. These patches provide a way of launching that REPL
> in an isolated environment through Linux namespaces, providing some
> isolation from the wider system.
>
> These patches should work, at least enough to get the derivations for
> packages within the inferior Guix, as well as doing 'guix pull' within
> the inferior Guix.

This is really cool.

When we do this kind of thing (like also the “Compute Guix derivation”
trampoline used by ‘guix pull’), it reminds me of what the Nix people
call “recursive Nix”—the ability for a derivation’s build process to
compute other derivation. If we had that, then basically what you’re
doing might just as well be a derivation.

BTW, thinking about it, for the Guix Data Service, would
‘gexp->derivation-in-inferior’ be of any use? This is used, for
example, to compute the package cache when running ‘guix pull’. I
think it’s good enough if all you want is to extract basic file
meta-data, but it’s no good if you also want to extract package
derivations and the likes.

Or we could have a new store back-end that computes derivations in
memory and eventually spits a Nar…

I’m just thinking out loud!

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 14 Mar 2019 19:17
Re: [bug#34638] [PATCH 2/4] linux-container: Add 'start-child-in-container'.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
87lg1hiam0.fsf@gnu.org
Hello!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (5 lines)
> This new procedure is similar to open-pipe* in (ice-9 popen), but using
> run-container from (gnu build linux-container).
>
> * gnu/build/linux-container.scm (start-child-in-container): New procedure.

[...]

+(define* (start-child-in-container command
Toggle quote (7 lines)
> + #:key read? write?
> + (root 'temporary)
> + (mounts '())
> + (namespaces %namespaces)
> + (host-uids 1)
> + (extra-environment-variables '()))

We could even call that ‘open-pipe/container’, for clarity.

Toggle quote (10 lines)
> + (define (with-root-directory f)
> + (if (eq? root 'temporary)
> + (call-with-temporary-directory f)
> + (f root)))
> +
> + ;; (ice-9 popen) internals
> + (define make-rw-port (@@ (ice-9 popen) make-rw-port))
> + (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
> + (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))

So this is the funky part. ;-)

What if we did something like:

(call-with-container mounts
(lambda ()
;; Somehow act as a proxy between the output process
;; and the one spawned by ‘open-pipe*’.
(open-pipe* …)))

? Would that work?

That’s create an extra process, but if it works, it’s probably safer and
a lesser maintenance burden.

Now, I think that Guile should expose some of the popen internals
somehow so we can do things like you did, but that’s another story.

Ludo’.
C
C
Christopher Baines wrote on 19 Apr 2019 16:04
[PATCH v2 3/4] inferior: Add a shared-directory field to <inferior>
(address . 34638@debbugs.gnu.org)
20190419140427.15183-3-mail@cbaines.net
---
guix/inferior.scm | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)

Toggle diff (67 lines)
diff --git a/guix/inferior.scm b/guix/inferior.scm
index 63c95141d7..6d18ab90e9 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -97,14 +97,15 @@
 
 ;; Inferior Guix process.
 (define-record-type <inferior>
-  (inferior pid socket close version packages table)
+  (inferior pid socket close shared-directory version packages table)
   inferior?
-  (pid      inferior-pid)
-  (socket   inferior-socket)
-  (close    inferior-close-socket)               ;procedure
-  (version  inferior-version)                    ;REPL protocol version
-  (packages inferior-package-promise)            ;promise of inferior packages
-  (table    inferior-package-table))             ;promise of vhash
+  (pid              inferior-pid)
+  (socket           inferior-socket)
+  (close            inferior-close-socket)       ;procedure
+  (shared-directory inferior-shared-directory)
+  (version          inferior-version)            ;REPL protocol version
+  (packages         inferior-package-promise)    ;promise of inferior packages
+  (table            inferior-package-table))     ;promise of vhash
 
 (define (inferior-pipe directory command)
   "Return an input/output pipe on the Guix instance in DIRECTORY.  This runs
@@ -136,7 +137,7 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
-(define* (port->inferior pipe #:optional (close close-port))
+(define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
 inferior."
@@ -144,7 +145,8 @@ inferior."
 
   (match (read pipe)
     (('repl-version 0 rest ...)
-     (letrec ((result (inferior 'pipe pipe close (cons 0 rest)
+     (letrec ((result (inferior 'pipe pipe close shared-directory
+                                (cons 0 rest)
                                 (delay (%inferior-packages result))
                                 (delay (%inferior-package-table result)))))
        (inferior-eval '(use-modules (guix)) result)
@@ -162,7 +164,7 @@ equivalent.  Return #f if the inferior could not be launched."
   (define pipe
     (inferior-pipe directory command))
 
-  (port->inferior pipe close-pipe))
+  (port->inferior pipe #f close-pipe))
 
 (define (close-inferior inferior)
   "Close INFERIOR."
@@ -479,7 +481,8 @@ thus be the code of a one-argument procedure that accepts a store."
          ((client . address)
           (proxy client (store-connection-socket store))))
        (close-port socket)
-       (read-inferior-response inferior)))))
+       (read-inferior-response inferior)))
+   #:base-directory (inferior-shared-directory inferior)))
 
 (define* (inferior-package-derivation store package
                                       #:optional
-- 
2.21.0
C
C
Christopher Baines wrote on 19 Apr 2019 16:04
[PATCH v2 2/4] linux-container: Add 'start-child-in-container'.
(address . 34638@debbugs.gnu.org)
20190419140427.15183-2-mail@cbaines.net
This new procedure is similar to open-pipe* in (ice-9 popen), but using
run-container from (gnu build linux-container).

* gnu/build/linux-container.scm (start-child-in-container): New procedure.
---
gnu/build/linux-container.scm | 83 +++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)

Toggle diff (103 lines)
diff --git a/gnu/build/linux-container.scm b/gnu/build/linux-container.scm
index 3d7b52f098..88b00e00f6 100644
--- a/gnu/build/linux-container.scm
+++ b/gnu/build/linux-container.scm
@@ -32,6 +32,7 @@
             setgroups-supported?
             %namespaces
             run-container
+            start-child-in-container
             call-with-container
             container-excursion
             container-excursion*))
@@ -213,6 +214,88 @@ corresponds to the symbols in NAMESPACES."
                ('net  CLONE_NEWNET))
               namespaces)))
 
+(define* (start-child-in-container command
+                                   #:key read? write?
+                                   (root 'temporary)
+                                   (mounts '())
+                                   (namespaces %namespaces)
+                                   (host-uids 1)
+                                   (extra-environment-variables '()))
+  (define (with-root-directory f)
+    (if (eq? root 'temporary)
+        (call-with-temporary-directory f)
+        (f root)))
+
+  (define (make-rw-port read-port write-port)
+    (make-soft-port
+     (vector
+      (lambda (c) (write-char c write-port))
+      (lambda (s) (display s write-port))
+      (lambda () (force-output write-port))
+      (lambda () (read-char read-port))
+      (lambda () (close-port read-port) (close-port write-port)))
+     "r+"))
+
+  ;; car is the inport port, cdr is the output port. You write to the output
+  ;; port, and read from the input port.
+  (define child-to-parent-pipe
+    (if read?
+        (pipe)
+        #f))
+
+  (define parent-to-child-pipe
+    (if write?
+        (pipe)
+        #f))
+
+  (define (run-program)
+    (when read?
+      (match child-to-parent-pipe
+        ((input-port . output-port)
+         ;; close the output part of the child-to-parent-pipe, as this is used
+         ;; by the parent process
+         (close-port input-port)
+
+         ;; Make the input part of the child-to-parent-pipe the standard
+         ;; output of this process
+         (dup2 (fileno output-port) 1))))
+
+    (when write?
+      (match parent-to-child-pipe
+        ((input-port . output-port)
+         ;; close the input part of the parent-to-child-pipe, as this is used
+         ;; by the parent processs
+         (close-port output-port)
+
+         ;; Make the output part of the parent-to-child-pipe the standard
+         ;; input of this process
+         (dup2 (fileno input-port) 0))))
+
+    ;; TODO Maybe close all file descriptors, as start_child in Guile does?
+
+    (for-each putenv extra-environment-variables)
+
+    (apply execlp command))
+
+  (with-root-directory
+   (lambda (root)
+     (let ((pid (run-container root mounts namespaces host-uids run-program)))
+       ;; Catch SIGINT and kill the container process.
+       (sigaction SIGINT
+         (lambda (signum)
+           (false-if-exception
+            (kill pid SIGKILL))))
+
+       (let* ((read-port (and=> child-to-parent-pipe car))
+              (write-port (and=> parent-to-child-pipe cdr))
+
+              (port (or (and read-port write-port
+                             (make-rw-port read-port write-port))
+                        read-port
+                        write-port)))
+
+         (values port pid))))))
+
 (define* (run-container root mounts namespaces host-uids thunk
                         #:key (guest-uid 0) (guest-gid 0))
   "Run THUNK in a new container process and return its PID.  ROOT specifies
-- 
2.21.0
C
C
Christopher Baines wrote on 19 Apr 2019 16:04
[PATCH v2 1/4] utils: Add #:base-directory to call-with-temporary-directory.
(address . 34638@debbugs.gnu.org)
20190419140427.15183-1-mail@cbaines.net
This allows more easily creating temporary directories within a specific
directory. This is motivated by using this in inferior-eval-with-store.

* guix/utils.scm (call-with-temporary-directory): Add optional keyword
argument, base-directory.
---
guix/utils.scm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Toggle diff (19 lines)
diff --git a/guix/utils.scm b/guix/utils.scm
index ed1a418cca..abeb156f40 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -620,10 +620,10 @@ call."
         (false-if-exception (close out))
         (false-if-exception (delete-file template))))))
 
-(define (call-with-temporary-directory proc)
+(define* (call-with-temporary-directory proc #:key base-directory)
   "Call PROC with a name of a temporary directory; close the directory and
 delete it when leaving the dynamic extent of this call."
-  (let* ((directory (or (getenv "TMPDIR") "/tmp"))
+  (let* ((directory (or base-directory (getenv "TMPDIR") "/tmp"))
          (template  (string-append directory "/guix-directory.XXXXXX"))
          (tmp-dir   (mkdtemp! template)))
     (dynamic-wind
-- 
2.21.0
C
C
Christopher Baines wrote on 19 Apr 2019 16:04
[PATCH v2 4/4] inferior: Add 'open-inferior/container'.
(address . 34638@debbugs.gnu.org)
20190419140427.15183-4-mail@cbaines.net
---
guix/inferior.scm | 76 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)

Toggle diff (110 lines)
diff --git a/guix/inferior.scm b/guix/inferior.scm
index 6d18ab90e9..8238c7fb38 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -19,6 +19,7 @@
 (define-module (guix inferior)
   #:use-module (srfi srfi-9)
   #:use-module (srfi srfi-9 gnu)
+  #:use-module (srfi srfi-11)
   #:use-module ((guix utils)
                 #:select (%current-system
                           source-properties->location
@@ -40,6 +41,9 @@
   #:use-module (guix store)
   #:use-module (guix derivations)
   #:use-module (guix base32)
+  #:use-module (gnu system file-systems)
+  #:use-module (gnu build linux-container)
+  #:use-module (guix build syscalls)
   #:use-module (gcrypt hash)
   #:autoload   (guix cache) (maybe-remove-expired-cache-entries)
   #:autoload   (guix ui) (show-what-to-build*)
@@ -54,6 +58,7 @@
   #:use-module ((rnrs bytevectors) #:select (string->utf8))
   #:export (inferior?
             open-inferior
+            open-inferior/container
             port->inferior
             close-inferior
             inferior-eval
@@ -137,6 +142,77 @@ it's an old Guix."
                           ((@ (guix scripts repl) machine-repl))))))
         pipe)))
 
+(define* (open-inferior/container store guix-store-item
+                                  #:key
+                                  (command "bin/guix")
+                                  (share-host-network? #f)
+                                  (extra-shared-directories '())
+                                  (extra-environment-variables '()))
+  (define requisite-store-items
+    (requisites store (list guix-store-item)))
+
+  (define shared-directory
+    (mkdtemp! (string-append (or (getenv "TMPDIR") "/tmp")
+                             "/guix-inferior.XXXXXX")))
+
+  (define mappings
+    (append
+     (map (lambda (dir)
+            (file-system-mapping
+             (source dir)
+             (target dir)
+             (writable? #f)))
+          `(;; Share a directory, used in inferior-eval-with-store
+            ,shared-directory
+            ,@requisite-store-items
+            ,@extra-shared-directories))
+     (if share-host-network?
+         %network-file-mappings
+         '())))
+
+  (define mounts
+    (append %container-file-systems
+            (map file-system-mapping->bind-mount
+                 mappings)))
+
+  (define (inferior-pipe/container store
+                                   guix-store-item
+                                   shared-directory
+                                   command)
+    (start-child-in-container
+     (list (string-append guix-store-item "/bin/guix")
+           ;; TODO I'm not sure why "repl" is duplicated in the following
+           ;; command
+           "repl" "repl" "-t" "machine")
+     #:read? #t
+     #:write? #t
+     #:mounts mounts
+     #:namespaces (if share-host-network?
+                      (delq 'net %namespaces)
+                      %namespaces)
+     #:extra-environment-variables
+     `(;; Set HOME, so that the (guix profiles) module can be loaded, without it
+       ;; trying to read from /etc/passwd
+       "HOME=/tmp"
+       ,@extra-environment-variables)))
+
+  (let*-values
+      (((pipe pid)
+        (inferior-pipe/container store
+                                 guix-store-item
+                                 shared-directory
+                                 command))
+       ((close-inferior-pipe)
+        (lambda (pipe*)
+          (unless (eq? pipe pipe*)
+            (error "wrong pipe being closed"))
+          (close-port pipe)
+          (cdr (waitpid pid)))))
+
+      (port->inferior pipe
+                      shared-directory
+                      close-inferior-pipe)))
+
 (define* (port->inferior pipe shared-directory #:optional (close close-port))
   "Given PIPE, an input/output port, return an inferior that talks over PIPE.
 PIPE is closed with CLOSE when 'close-inferior' is called on the returned
-- 
2.21.0
C
C
Christopher Baines wrote on 19 Apr 2019 16:16
Re: [bug#34638] [PATCH 2/4] linux-container: Add 'start-child-in-container'.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 34638@debbugs.gnu.org)
87pnpit707.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (21 lines)
> Hello!
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>> run-container from (gnu build linux-container).
>>
>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>
> [...]
>
> +(define* (start-child-in-container command
>> + #:key read? write?
>> + (root 'temporary)
>> + (mounts '())
>> + (namespaces %namespaces)
>> + (host-uids 1)
>> + (extra-environment-variables '()))
>
> We could even call that ‘open-pipe/container’, for clarity.

I've made some changes (see below) that move this a little further away
from open-pipe in terms of behaviour now.

Toggle quote (28 lines)
>> + (define (with-root-directory f)
>> + (if (eq? root 'temporary)
>> + (call-with-temporary-directory f)
>> + (f root)))
>> +
>> + ;; (ice-9 popen) internals
>> + (define make-rw-port (@@ (ice-9 popen) make-rw-port))
>> + (define pipe-guardian (@@ (ice-9 popen) pipe-guardian))
>> + (define make-pipe-info (@@ (ice-9 popen) make-pipe-info))
>
> So this is the funky part. ;-)
>
> What if we did something like:
>
> (call-with-container mounts
> (lambda ()
> ;; Somehow act as a proxy between the output process
> ;; and the one spawned by ‘open-pipe*’.
> (open-pipe* …)))
>
> ? Would that work?
>
> That’s create an extra process, but if it works, it’s probably safer and
> a lesser maintenance burden.
>
> Now, I think that Guile should expose some of the popen internals
> somehow so we can do things like you did, but that’s another story.

I'm hesitant to try that, as the additional process in the middle seems
a bit awkward to me.

I've made another pass over the code, removed all the uses of (ice-9
popen) internals, and sent another set of patches. For the make-rw-port
function, I just copied that over. The pipe-guardian isn't being used
now, and instead of returning a <pipe-info> record, the port and pid are
returned instead. This works with the inferior use case, as the close
function provided to port->inferior does the right thing, closing the
port and then waiting for the child process to exit, just like popen.

I'm still more interested in getting something working than it being
perfect in any particular way, but let me know what you think.

Thanks,

Chris
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAly52EhfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9XfZTw/6A8MgyDllG42QeJT5f/qXG9mee6q8edCLQHBTytq9Mzv3RRr20uJ4jIoz
xc8AYRsTO8niCgU6CquAIbz1eR/koz4xo753qlo1grQYj/M71VpI5lQkukyPhcX6
ZcCZPDXWxrz/wiYlbhVB813B6rP8DLo7wxK9StB3ybQk4kkI8+lR9b69nteyXa/B
YoV3L8CG+jiuvyRokTcNytpj3GJjv+Xiy9hOVPe9B1Oken76kIpqmc14kV2341Ih
8o+T29vuHS+0dlTz5ovVtvGp5QIK5tKBR0jZingiN60tSCqgrY7AouEGMAM+V1S4
Z+QLGqk+boLKhFIZgseT3Tn66OIiO12J7SPPyLhyBLU8dUa3r8oYFVlmQFWVsiK7
RDLTot0C2+MDOooCtA+Kam33IAppD1DZY3AImbMut7ZzXVwTOMdaCgzRLP8acn3X
lZlURvieJkzAiQFDIKYnicbqnDFdXoH8tm997tb98giu8Ruf7YiaT1aHM5u2iGsT
Hh1uFEWnlaW+Y09fLFdsPhJBH4ovoESvT1TuxZqbYF3FtRTdhlQ4daUYx49Zf04+
5fNdzX79mFPUXygc0PZ+v8+mQ4vwOUQS02giBS63igBWseb6ObzR/gA/mjLwRJ+w
qYb7A9gUURe9aXC47AoL92lxFL+xfmTAPDFyi4LAMS1WQFtaEuk=
=0lHn
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 26 Mar 2020 10:22
Re: [bug#34638] [PATCH v2 1/4] utils: Add #:base-directory to call-with-temporary-directory.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
87lfnneah8.fsf@gnu.org
Hello,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (6 lines)
> This allows more easily creating temporary directories within a specific
> directory. This is motivated by using this in inferior-eval-with-store.
>
> * guix/utils.scm (call-with-temporary-directory): Add optional keyword
> argument, base-directory.

LGTM.
L
L
Ludovic Courtès wrote on 26 Mar 2020 10:28
Re: [bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
87ftdvea8d.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (5 lines)
> This new procedure is similar to open-pipe* in (ice-9 popen), but using
> run-container from (gnu build linux-container).
>
> * gnu/build/linux-container.scm (start-child-in-container): New procedure.

[...]

Toggle quote (8 lines)
> +(define* (start-child-in-container command
> + #:key read? write?
> + (root 'temporary)
> + (mounts '())
> + (namespaces %namespaces)
> + (host-uids 1)
> + (extra-environment-variables '()))

Please add a docstring. :-)

I’d change (extra-environment-variables '()) to:

(environment-variables (environ))

I always find it too hard to reason about “extra” thing; it’s just more
convenient as an interface to specify the whole thing rather than a list
of “extras”.

Toggle quote (2 lines)
> + (apply execlp command))

To provide a correct argv[0] by default, you should probably change it
to:

(match command
((program arguments ...)
(execlp program program arguments)))

(That’ll also address a comment of yours in one of the subsequent
patches.)

Could you add a test to ‘tests/containers.scm’?

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 26 Mar 2020 10:30
Re: [bug#34638] [PATCH v2 3/4] inferior: Add a shared-directory field to <inferior>
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
87blojea3y.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (4 lines)
> ---
> guix/inferior.scm | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)

Commit log please. :-)

Toggle quote (5 lines)
> + (pid inferior-pid)
> + (socket inferior-socket)
> + (close inferior-close-socket) ;procedure
> + (shared-directory inferior-shared-directory)

Please add a margin comment like “#f | directory”.

Toggle quote (6 lines)
> -(define* (port->inferior pipe #:optional (close close-port))
> +(define* (port->inferior pipe shared-directory #:optional (close close-port))
> "Given PIPE, an input/output port, return an inferior that talks over PIPE.
> PIPE is closed with CLOSE when 'close-inferior' is called on the returned
> inferior."

Make ‘shared-directory’ a keyword argument?

(Otherwise there’s a user in (guix ssh) that needs to be updated.)

Toggle quote (7 lines)
> ((client . address)
> (proxy client (store-connection-socket store))))
> (close-port socket)
> - (read-inferior-response inferior)))))
> + (read-inferior-response inferior)))
> + #:base-directory (inferior-shared-directory inferior)))

What if ‘inferior-shared-directory’ returns #f?

Otherwise LGTM.

Ludo’.
L
L
Ludovic Courtès wrote on 26 Mar 2020 10:32
Re: [bug#34638] [PATCH v2 4/4] inferior: Add 'open-inferior/container'.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
877dz7ea1g.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (4 lines)
> ---
> guix/inferior.scm | 76 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)

[...]

Toggle quote (7 lines)
> +(define* (open-inferior/container store guix-store-item
> + #:key
> + (command "bin/guix")
> + (share-host-network? #f)
> + (extra-shared-directories '())
> + (extra-environment-variables '()))

Please add a docstring. Same comment as before regarding “extras”. :-)

Toggle quote (6 lines)
> + (start-child-in-container
> + (list (string-append guix-store-item "/bin/guix")
> + ;; TODO I'm not sure why "repl" is duplicated in the following
> + ;; command
> + "repl" "repl" "-t" "machine")

This is the argv[0] issue mentioned earlier.

I think it’s not really feasible to write a test for this one, or at
least I don’t see how.

Otherwise LGTM, thanks!

Ludo’.
C
C
Christopher Baines wrote on 28 Mar 2020 12:26
Re: [bug#34638] [PATCH v2 2/4] linux-container: Add 'start-child-in-container'.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 34638@debbugs.gnu.org)
87mu80sosv.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (27 lines)
> Christopher Baines <mail@cbaines.net> skribis:
>
>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>> run-container from (gnu build linux-container).
>>
>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>
> [...]
>
>> +(define* (start-child-in-container command
>> + #:key read? write?
>> + (root 'temporary)
>> + (mounts '())
>> + (namespaces %namespaces)
>> + (host-uids 1)
>> + (extra-environment-variables '()))
>
> Please add a docstring. :-)
>
> I’d change (extra-environment-variables '()) to:
>
> (environment-variables (environ))
>
> I always find it too hard to reason about “extra” thing; it’s just more
> convenient as an interface to specify the whole thing rather than a list
> of “extras”.

I had a go at this, but I think trying to copy the environment variables
from the host Guix to the inferior one caused problems, at least this
backtrace appears when calling open-inferior/container and I'm guessing
it comes from the inferior guix.

I think calling it environment-variables and having it be '() is OK, the
only change I can see being made elsewhere is that
open-inferior/container adds HOME=/tmp, and that's just to avoid issues
with (guix profiles).

Does that make sense?


Backtrace:
6 (apply-smob/1 #<catch-closure 7f77e0a889a0>)
In ice-9/boot-9.scm:
705:2 5 (call-with-prompt ("prompt") #<procedure 7f77e0a9f560 at ice-9/eval.scm:330:13 ()> #<procedure default-prompt-handler (k proc)>)
In ice-9/eval.scm:
619:8 4 (_ #(#(#<directory (guile-user) 7f77e0717140>)))
293:34 3 (_ #(#(#<directory (guile-user) 7f77e0717140>) ("/gnu/store/ain1rvg7vrrcr85v0fgpyjm8k2sflxpz-guix-1.0.1-15.0984481/bin/.guix-real" "repl" "-t" "machi?")))
159:9 2 (_ #(#(#<directory (guile-user) 7f77e0717140>) ("/gnu/store/ain1rvg7vrrcr85v0fgpyjm8k2sflxpz-guix-1.0.1-15.0984481/bin/.guix-real" "repl" "-t" "machi?")))
In ice-9/boot-9.scm:
2803:6 1 (resolve-interface _ #:select _ #:hide _ #:prefix _ #:renamer _ #:version _)
In unknown file:
0 (scm-error misc-error #f "~A ~S" ("no code for module" (guix ui)) #f)
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAl5/NHBfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9XfXhxAAp4e9MN7waIdKAiqb+wTQZrwvNzzPutFMWJiao99q+TMI7EEoIwPwBJBi
V/+cG769CtrTSvRpYi2/BjCaeFVNwGbXcXFzSMsFbFzZ9WFDkPQXI5IzU4UxbQ8/
JssHQqEr+OiSVsgjcLQxYleXT8UOB9ejoLRvIiQ86QJoDDMOj8ZminlLzu9yeMR8
sHojdMM79D2NvSbaiKEpzgOaWsoPKvP0B8oqm7qWyGXYsSql8Iz5/8XbCuWlwyVi
lftYl/8FCAfS5Emtm25+L4iRN1oTphfaD/QIa3JrPDoc/eewYS+NKsyuIGBUELwO
wnIyxbbW+hEoYz35v0mT6K4YJTJiPdTCmK/N+9MAco9cQyOYkL0zBB/ZtNoNgyXJ
OE1r4w3dkEuN0fFEjiRH9MtPcgw5ZdsWKaqzznW9qV7fTIuo5NiRY+2wf7Zcc1Ws
Gg6ORRnmOq89wy73fEOQUVygcxgT8T3pD6TuQ0wkOo2zfFs/QSlEZzz/QB/8iiL8
D8YPhpvFwi437pGkl403BOKn8/ssCVLUWjFCV0RmWNLOVtLjaIOAU+c0DfCkPGD8
mdSLOwqa7eEcZAHVO705ZIp7Lm6gYPEJ/Jk7iEK/bMBpNjAexLW7NYqWEdnalUtv
lE5W/Qphf48ARQswn6qivbDsiVw0yfIZqc3fbb1XpA2wtLobovE=
=ThB4
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 28 Mar 2020 13:20
(name . Christopher Baines)(address . mail@cbaines.net)(address . 34638@debbugs.gnu.org)
877dz44qo7.fsf@gnu.org
Hi!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (41 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Christopher Baines <mail@cbaines.net> skribis:
>>
>>> This new procedure is similar to open-pipe* in (ice-9 popen), but using
>>> run-container from (gnu build linux-container).
>>>
>>> * gnu/build/linux-container.scm (start-child-in-container): New procedure.
>>
>> [...]
>>
>>> +(define* (start-child-in-container command
>>> + #:key read? write?
>>> + (root 'temporary)
>>> + (mounts '())
>>> + (namespaces %namespaces)
>>> + (host-uids 1)
>>> + (extra-environment-variables '()))
>>
>> Please add a docstring. :-)
>>
>> I’d change (extra-environment-variables '()) to:
>>
>> (environment-variables (environ))
>>
>> I always find it too hard to reason about “extra” thing; it’s just more
>> convenient as an interface to specify the whole thing rather than a list
>> of “extras”.
>
> I had a go at this, but I think trying to copy the environment variables
> from the host Guix to the inferior one caused problems, at least this
> backtrace appears when calling open-inferior/container and I'm guessing
> it comes from the inferior guix.
>
> I think calling it environment-variables and having it be '() is OK, the
> only change I can see being made elsewhere is that
> open-inferior/container adds HOME=/tmp, and that's just to avoid issues
> with (guix profiles).
>
> Does that make sense?

Ah yes, defaulting to the empty list is even better.

Thanks,
Ludo’.
?