* [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 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).