Skip to content
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

Fix condition to allow creating console on stdio #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hotgake
Copy link

@hotgake hotgake commented Dec 3, 2021

When I used stdio interactive console, two errors occurs.

  1. With Serial Console, checkSerials() raise wrong error: "If StdioInteractiveConsole is set, stdio must be a TTY"

To connect cmd.Stdin with os.Stdin and interact with VM, os.Stdout should be terminal.
But if os.Stdout is terminal, checkSerials() raise error that say that "must be a terminal."
When I checked the previous code, it was originally !isTerminal(). I think, it is a typo.

  1. Error that occurs when cmd.StdoutPipe() is called while cmd.Stdout is already set: "Stdout already set"

If use stdio console, cmd.Stdout could not be nil. But cmd.StdoutPipe() requires that cmd.Stdout must be nil.
Thus, I could not use stdio console.

filename := h.findStdioTTY()
if filename != "" {
	if isTerminal(os.Stdout) {
		cmd.Stdin = os.Stdin
		cmd.Stdout = os.Stdout // With ConsoleStdio, cmd.Stdout is set to os.Stdout.
		cmd.Stderr = os.Stderr
	} else {
		...
	}
}
if log != nil {
	log.Debugf("hyperkit: Redirecting stdout/stderr to logger")
	stdout, err := cmd.StdoutPipe() // <- This line raise error!
	...
}
func (c *Cmd) StdoutPipe() (io.ReadCloser, error) {
	if c.Stdout != nil {
		return nil, errors.New("exec: Stdout already set") // If use ConsoleStdio, c.Stdout could not be nil and return error.
	}
	...
}

I used Google translate to write this PR. If any of my expressions were rude, it was not intentional, so please understand.

Signed-off-by: HyeonCheol Yun <hotgake@gmail.com>
@hotgake
Copy link
Author

hotgake commented Dec 3, 2021

The CI pipeline failed because of failure to update repository for opam which irrelevant to this PR.

Is there any way I can fix this?

@thaJeztah
Copy link
Member

I used Google translate to write this PR. If any of my expressions were rude, it was not intentional, so please understand.

It looks perfect, thanks! (also, don't worry, most contributors in open source are not native english speakers, and we are not easily offended 🤗)

The CI pipeline failed because of failure to update repository for opam which irrelevant to this PR.

I tried to restart CI, but it failed again. I'm not very familiar with this repository though, but perhaps @djs55 could have a quick look?

@thaJeztah thaJeztah requested a review from djs55 December 3, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants