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

go: improve serial console #292

Merged
merged 4 commits into from
Aug 24, 2020
Merged

go: improve serial console #292

merged 4 commits into from
Aug 24, 2020

Conversation

djs55
Copy link
Collaborator

@djs55 djs55 commented Jul 23, 2020

The current Go API for setting up the serial console has limitations, including:

  • it supports at most one serial port
  • it requires either stdio or a tty, and will trigger an assert if you don't provide one of them
  • it combines the request for a tty with the request to log the output to a ring-buffer or to Apple System Log.

This PR adds a new Go API while keeping (but deprecating) the old one. It's now possible to have multiple serial ports, each of which is connected to one of:

  • /dev/null
  • a TTY
  • stdio

and of which can be separately logged to any combination of:

  • /dev/null
  • a ring buffer
  • Apple System Log

This PR also adds a go.mod and some unit tests for the console-related command-line building functions.

This allows a native `go build` and `go test`.

Signed-off-by: David Scott <[email protected]>
The previous API didn't allow us to disable the PTY but keep the
ring buffer, for example. These should be orthogonal, so add a new
API and deprecate the old one.

Signed-off-by: David Scott <[email protected]>
@djs55
Copy link
Collaborator Author

djs55 commented Jul 24, 2020

User observed a problem here: docker/roadmap#12 (comment) -- it looks like I've forgotten to modify the execute() function.
Unit tests are currently failing:

=== RUN   TestLegacyConsole
--- FAIL: TestLegacyConsole (0.00s)
    hyperkit_test.go:12: 
        	Error Trace:	hyperkit_test.go:12
        	Error:      	Expected nil, but got: &errors.errorString{s:"Could not find hyperkit executable hyperkit: exec: \"hyperkit\": executable file not found in $PATH"}
        	Test:       	TestLegacyConsole
=== RUN   TestNewSerial
--- FAIL: TestNewSerial (0.00s)
    hyperkit_test.go:21: 
        	Error Trace:	hyperkit_test.go:21
        	Error:      	Expected nil, but got: &errors.errorString{s:"Could not find hyperkit executable hyperkit: exec: \"hyperkit\": executable file not found in $PATH"}
        	Test:       	TestNewSerial
=== RUN   TestNullSerial
--- FAIL: TestNullSerial (0.00s)
    hyperkit_test.go:35: 
        	Error Trace:	hyperkit_test.go:35
        	Error:      	Expected nil, but got: &errors.errorString{s:"Could not find hyperkit executable hyperkit: exec: \"hyperkit\": executable file not found in $PATH"}
        	Test:       	TestNullSerial
FAIL
FAIL	github.com/moby/hyperkit/go	0.148s
FAIL
make: *** [test] Error 1

It's useful to be able to log the serial console output to a ring
buffer (or even ASL) without necessarily having a real host console
attached to it (i.e. no TTY or `stdio`).

The code requires an open file descriptor so add a new type `null`
which opens `/dev/null`.

Signed-off-by: David Scott <[email protected]>
@djs55 djs55 changed the title WIP: Improve serial console Improve serial console Aug 4, 2020
@djs55 djs55 changed the title Improve serial console go: improve serial console Aug 4, 2020
Copy link
Contributor

@mat007 mat007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment on lines +274 to +275
} else {
if err := h.checkSerials(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: } else if { works as well

@djs55
Copy link
Collaborator Author

djs55 commented Aug 24, 2020

Thanks!

@djs55 djs55 merged commit b54460a into moby:master Aug 24, 2020
@djs55 djs55 deleted the improve-serial-console branch August 24, 2020 10:23
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