-
Notifications
You must be signed in to change notification settings - Fork 10
Add command line flag to run enclave application. #59
Conversation
This commit adds a new command line flag that instructs nitriding to run the given enclave application. Nitriding then blocks until the command finishes (if ever) and prints both the command's stdout and stderr. This feature has the advantage that one does not need a shell script to start both nitriding and the enclave application, meaning that it's now easier to build very slim Docker images that only contain a statically-compiled nitriding and enclave application. This fixes #58.
cmd/main.go
Outdated
// printOutput continuously reads from the given Reader until an EOF occurs. | ||
// Each newly read line is passed to the given function f. | ||
func printOutput(readCloser io.ReadCloser, f func(string), output string) { |
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.
This could be forwardOutput
to reflect that it's just forwarding to a callback, which may not actually print anything.
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.
Separately, does taking an io.ReadCloser
here do anything? I don't see an explicit close
call, and presumedly we want to keep the underlying pipe open?
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.
The documentation of StderrPipe
(and StdoutPipe
) says:
Wait will close the pipe after seeing the command exit, so most callers need not close the pipe themselves.
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.
This could be
forwardOutput
to reflect that it's just forwarding to a callback, which may not actually print anything.
Addressed in caf63c5.
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.
I see. Thanks for explaining.
cmd/main.go
Outdated
if errors.Is(err, io.EOF) { | ||
l.Printf("Encountered EOF in %s. Returning.", output) | ||
return |
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.
I think this drops a final, unterminated line. Should still forward f(b)
in this case?
ReadBytes reads until the first occurrence of delim in the input, returning a slice containing the data up to and including the delimiter. If ReadBytes encounters an error before finding a delimiter, it returns the data read before the error and the error itself (often io.EOF). ReadBytes returns err != nil if and only if the returned data does not end in delim.
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.
The same docs also suggest Scanner for line breaking.
scanner := bufio.NewScanner(readCloser)
for scanner.Scan() {
f(scanner.Text()) // f() needs to use Println since the scanner absorbs the delimeter.
}
if err := scanner.Err(); err != nil {
l.Printf("Error reading from enclave application:", err)
}
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.
I like the Scanner-based solution better. Addressed in e40c62f.
} | ||
dummy := func(string) {} | ||
|
||
runAppCommand("seq 1 3", f, dummy) |
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.
I would have said seq 3
but I guess this is clear.
...because it's more descriptive of what the function is actually doing.
This makes the code more elegant and easier to understand.
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.
lgtm
// starts nitriding). In this case, we simply block forever. | ||
if appCmd != "" { | ||
f := func(s string) { | ||
l.Printf("Application says: %s", s) |
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.
Oh, does this format string need to have a terminating \n
since Scanner
absorbs them?
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.
Good catch but no: Go's Logger package (which we are using here) is automatically appending a newline to log messages:
A newline is appended if the last character of s is not already a newline.
This commit adds a new command line flag that instructs nitriding to run the given enclave application. Nitriding then blocks until the command finishes (if ever) and prints both the command's stdout and stderr.
This feature has the advantage that one does not need a shell script to start both nitriding and the enclave application, meaning that it's now easier to build very slim Docker images that only contain a statically-compiled nitriding and enclave application.
This fixes #58.