Check IsTerminal before SetRawTerminal

[coolljt0725]

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 call runc not via a terminal

Signed-off-by: Lei Jitang leijitang@huawei.com

[duglin]

Is there a testcase that shows the failure w/o the fix?

[crosbymichael]

But do we want the error if the user specified TTY and we are unable to create one?

[ashahab-altiscale]

Also, isn't the newTty create testing the same thing?

[coolljt0725]

@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.

[coolljt0725]

@duglin This is a simple test

package main
import (
        "bytes"
        "fmt"
        "os/exec"
)
func main() {
        stdout := bytes.NewBuffer(nil)
        stderr := bytes.NewBuffer(nil)
        stdin := bytes.NewReader(nil)
        cmd := exec.Command("runc", "start", "-b", "/home/lei/containers/busybox")
        cmd.Stdout = stdout
        cmd.Stderr = stderr
        cmd.Stdin = stdin
        err := cmd.Start()
        if err != nil {
                fmt.Printf("failed to start; %v\n", err)
        }
        err = cmd.Wait()
        if err != nil {
                fmt.Printf("failed to wait: %v\n, stdout is: %v, stderr is: %v\n", err, stdout, stderr)
        }
}

This will show the error

go run runc.go 
failed to wait: exit status 1
, stdout is: time="2016-02-01T20:32:18-05:00" level=fatal msg="Container start failed: failed to set the terminal from the stdin: inappropriate ioctl for device" 
, stderr is: 

[duglin]

can you add an integration test with that?

[duglin]

oops - forget that - sorry, I thought this was in docker/docker :-)

[coolljt0725]

@crosbymichael I think we should return the error if user specify tty but unable to create one

[crosbymichael]

@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.

[coolljt0725]

@crosbymichael I just a bit of confused :-)
From what I understand, if term.IsTerminal(os.Stdin.Fd()) is just to block
SetRawTerminal if the input is not a terminal, it's not related to create tty.
the tty is create by NewConsole(rootuid). correct me if I'm wrong :-)

[crosbymichael]

@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.

[crosbymichael]

Lets close this one for now as we rework the tty handling

[cyphar]

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.

[coolljt0725]

@cyphar 👍