Current: allow stdin to be redirected #37
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In case we have stdin redirected, we still have a terminal which can be obtained from stdout or stderr. This is what this commit does. This should allow something like this to work: > ctr run --rm --tty docker.io/library/hello-world:latest xxx < /dev/null NB: in case all three are redirected, but we still have a controlling tty, we can easily get it by opening /dev/tty, but then it should be closed, and it's not quite clear by whom and when, so this is left as a home exersize for the reader. Signed-off-by: Kir Kolyshkin <[email protected]>
|
tested with a scenario from commit message: [kir@kir-rhat ctr]$ sudo ctr run --rm --tty docker.io/library/hello-world:latest xxx < /dev/null
panic: provided file is not a console
goroutine 1 [running]:
panic(0x56323cdbc500, 0xc00036ccf0)
/usr/lib/golang/src/runtime/panic.go:1060 +0x424 fp=0xc0005af488 sp=0xc0005af3e0 pc=0x56323c4161d4
github.com/containerd/console.Current(...)
/usr/share/gocode/src/github.com/containerd/console/console.go:67
github.com/containerd/containerd/cmd/ctr/commands/run.glob..func1(0xc0000c98c0, 0x0, 0x0)
.....
# now with vendored containerd/console from this PR
[kir@kir-rhat ctr]$ sudo ./ctr run --rm --tty docker.io/library/hello-world:latest xxx < /dev/null
Hello from Docker!
This message shows that your installation appears to be working correctly.
... |
Signed-off-by: Kir Kolyshkin <[email protected]>
|
@crosbymichael @dmcgowan PTAL |
|
Note that many programs that work with a terminal ( |
|
I swear I tried really really hard, and yet I can't spell exercise 🤦 🤦 😞 |
kolyshkin
added a commit
to kolyshkin/runc
that referenced
this pull request
Jul 31, 2020
This fixes the following failure: > sudo runc run -b bundle ctr </dev/null > WARN[0000] exit status 2 > ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:459: container init caused: The "exit status 2" with no error message is most probably caused by the panic in console.Current(), and is addressed by [1]. Otherwise, the issue here is simple: the code assumes stdin is opened to a terminal, and fails to work otherwise. Some standard Linux tools (e.g. stty, top) do the same (modulo panic), while some others (reset, tput) use the trick of trying all the three std streams (starting with stderr as it is least likely to be redirected), and if all three fails, open /dev/tty. This commit does the same, except that /dev/tty is not tried. It also replaces the call to console.Current() which might panic by reusing the t.hostConsole. Fixes: opencontainers#2485 [1] containerd/console#37 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/runc
that referenced
this pull request
Jul 31, 2020
This fixes the following failure: > sudo runc run -b bundle ctr </dev/null > WARN[0000] exit status 2 > ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:459: container init caused: The "exit status 2" with no error message is caused by SIGHUP which is sent to init by the kernel when we are losing the controlling terminal. If we choose to ignore that, we'll get panic in console.Current(), which is addressed by [1]. Otherwise, the issue here is simple: the code assumes stdin is opened to a terminal, and fails to work otherwise. Some standard Linux tools (e.g. stty, top) do the same (modulo panic), while some others (reset, tput) use the trick of trying all the three std streams (starting with stderr as it is least likely to be redirected), and if all three fails, open /dev/tty. This commit does a similar thing (see initHostConsole). It also replaces the call to console.Current(), which may panic (see [1]), by reusing the t.hostConsole. Finally, a simple test case is added. Fixes: opencontainers#2485 [1] containerd/console#37 Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/runc
that referenced
this pull request
Aug 20, 2020
This fixes the following failure: > sudo runc run -b bundle ctr </dev/null > WARN[0000] exit status 2 > ERRO[0000] container_linux.go:367: starting container process caused: process_linux.go:459: container init caused: The "exit status 2" with no error message is caused by SIGHUP which is sent to init by the kernel when we are losing the controlling terminal. If we choose to ignore that, we'll get panic in console.Current(), which is addressed by [1]. Otherwise, the issue here is simple: the code assumes stdin is opened to a terminal, and fails to work otherwise. Some standard Linux tools (e.g. stty, top) do the same (modulo panic), while some others (reset, tput) use the trick of trying all the three std streams (starting with stderr as it is least likely to be redirected), and if all three fails, open /dev/tty. This commit does a similar thing (see initHostConsole). It also replaces the call to console.Current(), which may panic (see [1]), by reusing the t.hostConsole. Finally, a simple test case is added. Fixes: opencontainers#2485 [1] containerd/console#37 Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In case we have stdin redirected, we still have a terminal which
can be obtained from stdout or stderr. This is what this commit
does.
This should allow something like this to work:
(found while working on opencontainers/runc#2485)
NB: in case all three are redirected, but we still have a controlling
tty, we can easily get it by opening /dev/tty, but then it should be
closed, and it's not quite clear by whom and when, so this is left as
a home exersize for the reader.