all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [PATCH] scripts: environment: Properly handle SIGINT.
@ 2016-03-26 13:08 David Thompson
  2016-03-26 18:23 ` Ludovic Courtès
  0 siblings, 1 reply; 6+ messages in thread
From: David Thompson @ 2016-03-26 13:08 UTC (permalink / raw)
  To: guix-devel

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

Has anyone ever been really annoyed that C-c doesn't work in a bash
shell spawned by 'guix environment'?  Me too!  And I finally got around
to fixing it.  I would like to get this in before 0.10.0 is released.

Ludo, I removed one of the tests in guix-environment-container.sh
because it seems obsolete to me now.  Let me know if you think of
another test to replace it.

Thanks!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-scripts-environment-Properly-handle-SIGINT.patch --]
[-- Type: text/x-patch, Size: 2994 bytes --]

From ec7994eec73d322386abbcd901da1b1d2f6f7733 Mon Sep 17 00:00:00 2001
From: David Thompson <dthompson2@worcester.edu>
Date: Sat, 26 Mar 2016 08:45:08 -0400
Subject: [PATCH] scripts: environment: Properly handle SIGINT.

Switching to execlp means that the process spawned in a container is PID
1, which obsoleted one of the 'guix environment --container' tests
because the init process can't be killed in the usual manner.

* guix/scripts/environment.scm (launch-environment/fork): New procedure.
(launch-environment): Switch from system* to execlp.  Add handler for
SIGINT.
(guix-environment): Use launch-environment/fork.
* tests/guix-environment-container.sh: Remove obsolete test.
---
 guix/scripts/environment.scm        | 19 +++++++++++++++++--
 tests/guix-environment-container.sh |  7 -------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index ee8f6b1..d554ca2 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -358,8 +358,22 @@ and suitable for 'exit'."
   "Run COMMAND in a new environment containing INPUTS, using the native search
 paths defined by the list PATHS.  When PURE?, pre-existing environment
 variables are cleared before setting the new ones."
+  ;; Properly handle SIGINT, so pressing C-c in an interactive terminal
+  ;; application works.
+  (sigaction SIGINT SIG_DFL)
   (create-environment inputs paths pure?)
-  (apply system* command))
+  (match command
+    ((program . args)
+     (apply execlp program program args))))
+
+(define (launch-environment/fork command inputs paths pure?)
+  "Run COMMAND in a new process with an environment containing INPUTS, using
+the native search paths defined by the list PATHS.  When PURE?, pre-existing
+environment variables are cleared before setting the new ones."
+  (match (primitive-fork)
+    (0 (launch-environment command inputs paths pure?))
+    (pid (match (waitpid pid)
+           ((_ . status) status)))))
 
 (define* (launch-environment/container #:key command bash user-mappings
                                        profile paths network?)
@@ -582,4 +596,5 @@ message if any test fails."
                  (else
                   (return
                    (exit/status
-                    (launch-environment command profile paths pure?)))))))))))))
+                    (launch-environment/fork command profile
+                                             paths pure?)))))))))))))
diff --git a/tests/guix-environment-container.sh b/tests/guix-environment-container.sh
index aba34a3..da4c6fc 100644
--- a/tests/guix-environment-container.sh
+++ b/tests/guix-environment-container.sh
@@ -81,10 +81,3 @@ grep $(guix build guile-bootstrap) $tmpdir/mounts
 grep -e "$NIX_STORE_DIR/.*-bash" $tmpdir/mounts # bootstrap bash
 
 rm $tmpdir/mounts
-
-if guix environment --bootstrap --container \
-	--ad-hoc bootstrap-binaries -- kill -SEGV 2
-then false;
-else
-    test $? -gt 127
-fi
-- 
2.7.3


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


-- 
David Thompson

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

* Re: [PATCH] scripts: environment: Properly handle SIGINT.
  2016-03-26 13:08 [PATCH] scripts: environment: Properly handle SIGINT David Thompson
@ 2016-03-26 18:23 ` Ludovic Courtès
  2016-03-26 18:33   ` Thompson, David
  0 siblings, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2016-03-26 18:23 UTC (permalink / raw)
  To: David Thompson; +Cc: guix-devel

David Thompson <dthompson2@worcester.edu> skribis:

> Has anyone ever been really annoyed that C-c doesn't work in a bash
> shell spawned by 'guix environment'?  Me too!  And I finally got around
> to fixing it.  I would like to get this in before 0.10.0 is released.

Indeed, that’s annoyed me a few times.  :-)  (C-z does work though.)

> From ec7994eec73d322386abbcd901da1b1d2f6f7733 Mon Sep 17 00:00:00 2001
> From: David Thompson <dthompson2@worcester.edu>
> Date: Sat, 26 Mar 2016 08:45:08 -0400
> Subject: [PATCH] scripts: environment: Properly handle SIGINT.
>
> Switching to execlp means that the process spawned in a container is PID
> 1, which obsoleted one of the 'guix environment --container' tests
> because the init process can't be killed in the usual manner.
>
> * guix/scripts/environment.scm (launch-environment/fork): New procedure.
> (launch-environment): Switch from system* to execlp.  Add handler for
> SIGINT.
> (guix-environment): Use launch-environment/fork.

Isn’t it enough to add the ‘sigaction’ call to fix the C-c issue?

Now, it’s nice to be PID 1 instead of PID 2, but that seems to be a
separate issue, no?

> * tests/guix-environment-container.sh: Remove obsolete test.

[...]

> -if guix environment --bootstrap --container \
> -	--ad-hoc bootstrap-binaries -- kill -SEGV 2
> -then false;
> -else
> -    test $? -gt 127
> -fi

This test was added in light of <http://bugs.gnu.org/21958>.  We want to
make sure we don’t lose that property.

What happens exactly when a signal is sent to PID 1?  I would expect
that its parent process, which is outside the container in a waitpid
call, would simply get its exit value in the normal way, and thus,
changing “2” to “1” in this test should do the trick.  Am I naïve?  :-)

Thanks for looking into it!

Ludo’.

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

* Re: [PATCH] scripts: environment: Properly handle SIGINT.
  2016-03-26 18:23 ` Ludovic Courtès
@ 2016-03-26 18:33   ` Thompson, David
  2016-03-26 20:53     ` Thompson, David
  0 siblings, 1 reply; 6+ messages in thread
From: Thompson, David @ 2016-03-26 18:33 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

On Sat, Mar 26, 2016 at 2:23 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> David Thompson <dthompson2@worcester.edu> skribis:
>
>> Has anyone ever been really annoyed that C-c doesn't work in a bash
>> shell spawned by 'guix environment'?  Me too!  And I finally got around
>> to fixing it.  I would like to get this in before 0.10.0 is released.
>
> Indeed, that’s annoyed me a few times.  :-)  (C-z does work though.)
>
>> From ec7994eec73d322386abbcd901da1b1d2f6f7733 Mon Sep 17 00:00:00 2001
>> From: David Thompson <dthompson2@worcester.edu>
>> Date: Sat, 26 Mar 2016 08:45:08 -0400
>> Subject: [PATCH] scripts: environment: Properly handle SIGINT.
>>
>> Switching to execlp means that the process spawned in a container is PID
>> 1, which obsoleted one of the 'guix environment --container' tests
>> because the init process can't be killed in the usual manner.
>>
>> * guix/scripts/environment.scm (launch-environment/fork): New procedure.
>> (launch-environment): Switch from system* to execlp.  Add handler for
>> SIGINT.
>> (guix-environment): Use launch-environment/fork.
>
> Isn’t it enough to add the ‘sigaction’ call to fix the C-c issue?

No, because system(3) states that "SIGINT and SIGQUIT will be ignored."

> Now, it’s nice to be PID 1 instead of PID 2, but that seems to be a
> separate issue, no?

Ideally, it would be a separate issue, if it weren't for the
above-mentioned issue that makes 'system' not usable for our purposes.

>> * tests/guix-environment-container.sh: Remove obsolete test.
>
> [...]
>
>> -if guix environment --bootstrap --container \
>> -     --ad-hoc bootstrap-binaries -- kill -SEGV 2
>> -then false;
>> -else
>> -    test $? -gt 127
>> -fi
>
> This test was added in light of <http://bugs.gnu.org/21958>.  We want to
> make sure we don’t lose that property.
>
> What happens exactly when a signal is sent to PID 1?  I would expect
> that its parent process, which is outside the container in a waitpid
> call, would simply get its exit value in the normal way, and thus,
> changing “2” to “1” in this test should do the trick.  Am I naïve?  :-)

The problem is that a process within the container cannot just kill
PID 1 since its the init process and the kernel protects it, so
changing "2" to "1" doesn't work.  The exit status of the environment
command is 0 in that case because PID 1 never received the signal and
thus exits normally.

I'll try to come up with a replacement test case, thanks for giving me
the context in which it was added.  (I should've used 'git blame'
first.)

- Dave

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

* Re: [PATCH] scripts: environment: Properly handle SIGINT.
  2016-03-26 18:33   ` Thompson, David
@ 2016-03-26 20:53     ` Thompson, David
  2016-03-27 17:35       ` Ludovic Courtès
  0 siblings, 1 reply; 6+ messages in thread
From: Thompson, David @ 2016-03-26 20:53 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

On Sat, Mar 26, 2016 at 2:33 PM, Thompson, David
<dthompson2@worcester.edu> wrote:
> On Sat, Mar 26, 2016 at 2:23 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> David Thompson <dthompson2@worcester.edu> skribis:
>>
>>> * tests/guix-environment-container.sh: Remove obsolete test.
>>
>> [...]
>>
>>> -if guix environment --bootstrap --container \
>>> -     --ad-hoc bootstrap-binaries -- kill -SEGV 2
>>> -then false;
>>> -else
>>> -    test $? -gt 127
>>> -fi
>>
>> This test was added in light of <http://bugs.gnu.org/21958>.  We want to
>> make sure we don’t lose that property.
>>
>> What happens exactly when a signal is sent to PID 1?  I would expect
>> that its parent process, which is outside the container in a waitpid
>> call, would simply get its exit value in the normal way, and thus,
>> changing “2” to “1” in this test should do the trick.  Am I naïve?  :-)
>
> The problem is that a process within the container cannot just kill
> PID 1 since its the init process and the kernel protects it, so
> changing "2" to "1" doesn't work.  The exit status of the environment
> command is 0 in that case because PID 1 never received the signal and
> thus exits normally.
>
> I'll try to come up with a replacement test case, thanks for giving me
> the context in which it was added.  (I should've used 'git blame'
> first.)

Coming up with a replacement test has proved very difficult.  Since
PID 1 is unkillable, I'm having a hell of time coming up with a clever
way to kill a Guile process via a signal.

- Dave

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

* Re: [PATCH] scripts: environment: Properly handle SIGINT.
  2016-03-26 20:53     ` Thompson, David
@ 2016-03-27 17:35       ` Ludovic Courtès
  2016-03-28 16:54         ` Thompson, David
  0 siblings, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2016-03-27 17:35 UTC (permalink / raw)
  To: Thompson, David; +Cc: guix-devel

"Thompson, David" <dthompson2@worcester.edu> skribis:

> On Sat, Mar 26, 2016 at 2:33 PM, Thompson, David
> <dthompson2@worcester.edu> wrote:
>> On Sat, Mar 26, 2016 at 2:23 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>> David Thompson <dthompson2@worcester.edu> skribis:
>>>
>>>> * tests/guix-environment-container.sh: Remove obsolete test.
>>>
>>> [...]
>>>
>>>> -if guix environment --bootstrap --container \
>>>> -     --ad-hoc bootstrap-binaries -- kill -SEGV 2
>>>> -then false;
>>>> -else
>>>> -    test $? -gt 127
>>>> -fi
>>>
>>> This test was added in light of <http://bugs.gnu.org/21958>.  We want to
>>> make sure we don’t lose that property.
>>>
>>> What happens exactly when a signal is sent to PID 1?  I would expect
>>> that its parent process, which is outside the container in a waitpid
>>> call, would simply get its exit value in the normal way, and thus,
>>> changing “2” to “1” in this test should do the trick.  Am I naïve?  :-)
>>
>> The problem is that a process within the container cannot just kill
>> PID 1 since its the init process and the kernel protects it, so
>> changing "2" to "1" doesn't work.  The exit status of the environment
>> command is 0 in that case because PID 1 never received the signal and
>> thus exits normally.
>>
>> I'll try to come up with a replacement test case, thanks for giving me
>> the context in which it was added.  (I should've used 'git blame'
>> first.)
>
> Coming up with a replacement test has proved very difficult.  Since
> PID 1 is unkillable, I'm having a hell of time coming up with a clever
> way to kill a Guile process via a signal.

Would it help to change the test to:

  guix environment --bootstrap --container \
     --ad-hoc bootstrap-binaries -- sh -c 'exec kill -SEGV 2'

essentially mimicking previous behavior?

Ludo’.

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

* Re: [PATCH] scripts: environment: Properly handle SIGINT.
  2016-03-27 17:35       ` Ludovic Courtès
@ 2016-03-28 16:54         ` Thompson, David
  0 siblings, 0 replies; 6+ messages in thread
From: Thompson, David @ 2016-03-28 16:54 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: guix-devel

On Sun, Mar 27, 2016 at 1:35 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> "Thompson, David" <dthompson2@worcester.edu> skribis:
>
>> On Sat, Mar 26, 2016 at 2:33 PM, Thompson, David
>> <dthompson2@worcester.edu> wrote:
>>> On Sat, Mar 26, 2016 at 2:23 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>> David Thompson <dthompson2@worcester.edu> skribis:
>>>>
>>>>> * tests/guix-environment-container.sh: Remove obsolete test.
>>>>
>>>> [...]
>>>>
>>>>> -if guix environment --bootstrap --container \
>>>>> -     --ad-hoc bootstrap-binaries -- kill -SEGV 2
>>>>> -then false;
>>>>> -else
>>>>> -    test $? -gt 127
>>>>> -fi
>>>>
>>>> This test was added in light of <http://bugs.gnu.org/21958>.  We want to
>>>> make sure we don’t lose that property.
>>>>
>>>> What happens exactly when a signal is sent to PID 1?  I would expect
>>>> that its parent process, which is outside the container in a waitpid
>>>> call, would simply get its exit value in the normal way, and thus,
>>>> changing “2” to “1” in this test should do the trick.  Am I naïve?  :-)
>>>
>>> The problem is that a process within the container cannot just kill
>>> PID 1 since its the init process and the kernel protects it, so
>>> changing "2" to "1" doesn't work.  The exit status of the environment
>>> command is 0 in that case because PID 1 never received the signal and
>>> thus exits normally.
>>>
>>> I'll try to come up with a replacement test case, thanks for giving me
>>> the context in which it was added.  (I should've used 'git blame'
>>> first.)
>>
>> Coming up with a replacement test has proved very difficult.  Since
>> PID 1 is unkillable, I'm having a hell of time coming up with a clever
>> way to kill a Guile process via a signal.
>
> Would it help to change the test to:
>
>   guix environment --bootstrap --container \
>      --ad-hoc bootstrap-binaries -- sh -c 'exec kill -SEGV 2'
>
> essentially mimicking previous behavior?

This didn't work, but for the record Ludo and I figured out another
way on IRC that involves using the FFI to make Guile segfault.  Fun
times!

Pushed with that change.

- Dave

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

end of thread, other threads:[~2016-03-28 16:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-26 13:08 [PATCH] scripts: environment: Properly handle SIGINT David Thompson
2016-03-26 18:23 ` Ludovic Courtès
2016-03-26 18:33   ` Thompson, David
2016-03-26 20:53     ` Thompson, David
2016-03-27 17:35       ` Ludovic Courtès
2016-03-28 16:54         ` Thompson, David

Code repositories for project(s) associated with this external index

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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.