-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check IsTerminal before SetRawTerminal #527
Conversation
Do not set raw terminal if stdin is not a terminal Signed-off-by: Lei Jitang <leijitang@huawei.com>
Is there a testcase that shows the failure w/o the fix? |
But do we want the error if the user specified TTY and we are unable to create one? |
if err != nil { | ||
return nil, fmt.Errorf("failed to set the terminal from the stdin: %v", err) | ||
var state *term.State | ||
if term.IsTerminal(os.Stdin.Fd()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't the newTty create testing the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashahab-altiscale No, it's different, create
is whether to create a tty
to container or not. create
is true
, but the stdin
could not be a terminal.
@duglin This is a simple test
This will show the error
|
can you add an integration test with that? |
oops - forget that - sorry, I thought this was in docker/docker :-) |
@crosbymichael I think we should return the error if user specify tty but unable to create one |
@coolljt0725 that is that the ioctl error was doing. if you guard it with this if IsTerminal then the user will not get an error. |
@crosbymichael I just a bit of confused :-) |
@coolljt0725 that call is going to work but it's still going to fail when you copy from os.Stdin to the pty master because its not set as raw terminal and you are going to get weird output. |
Lets close this one for now as we rework the tty handling |
For those reading at home, currently the tty handling reworking is happening in this branch (it's still a WIP and I'll open a PR once the code stops segfaulting): https://github.com/cyphar/runc/tree/console-rewrite. |
@cyphar 👍 |
Add language for compliance requirements around platforms
So that the semantics of the tags are clear. The platform/protocol disconnect is unfortunate. "Protocol" was chosen in de3f1af (Remove language around Solaris being optional as it is covered in compliance language, 2016-08-17, opencontainers#527) because we may have compliance subsets that aren't linked to platforms [1]. I'd be open to renaming the JSON tag from platform:"..." -> protocol:"...", but that's probably more change than it's worth. The approach taken in this commit, on the other hand, renames "protocol" to "platform". I think that unnecessarily limits (or sets up confusing semantics for) the platform/protocol values you can use, but two maintainers both prefer "platform" [2,3]. [1]: opencontainers/runtime-spec#527 (comment) [2]: opencontainers/runtime-spec#570 (comment) [3]: opencontainers/runtime-spec#570 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Do not set raw terminal if stdin is not a terminal.
or else it will show this error
Container start failed: failed to set the terminal from the stdin: inappropriate ioctl for device
and failed to start if we callrunc
not via a terminalSigned-off-by: Lei Jitang leijitang@huawei.com