veyron/lib/{modules,exec}: tidy up env var handling.
Tidy up the handling of environment variables in particular to
make working with and implementing the node manager and associated
tests easier.
The exec package now unsets the VEYRON_EXEC_VERSION env var when
it no longer needs to avoid having it be accidentally passed on
to other process that are are not necessarily started by this
package.
The modules shell type now has a 'CommandEnvelope' method that can
be used by the node manager when constructing envelopes for use
with the app manager.
Change-Id: I86a896557bdf6b87f39523fb7399453cf7d2a0fe
diff --git a/lib/exec/child.go b/lib/exec/child.go
index 8beb45a..105d0d5 100644
--- a/lib/exec/child.go
+++ b/lib/exec/child.go
@@ -9,8 +9,8 @@
)
var (
- ErrNoVersion = errors.New(versionVariable + " environment variable missing")
- ErrUnsupportedVersion = errors.New("Unsupported version of veyron/lib/exec request by " + versionVariable + " environment variable")
+ ErrNoVersion = errors.New(VersionVariable + " environment variable missing")
+ ErrUnsupportedVersion = errors.New("Unsupported version of veyron/lib/exec request by " + VersionVariable + " environment variable")
)
type ChildHandle struct {
@@ -77,10 +77,11 @@
}
func createChildHandle() (*ChildHandle, error) {
- switch os.Getenv(versionVariable) {
+ switch os.Getenv(VersionVariable) {
case "":
return nil, ErrNoVersion
case version1:
+ os.Setenv(VersionVariable, "")
// TODO(cnicolaou): need to use major.minor.build format for
// version #s.
default:
diff --git a/lib/exec/exec_test.go b/lib/exec/exec_test.go
index 1e7662e..cc1b292 100644
--- a/lib/exec/exec_test.go
+++ b/lib/exec/exec_test.go
@@ -394,6 +394,13 @@
clean(t, ph)
}
+func verifyNoExecVariable() {
+ version := os.Getenv(vexec.VersionVariable)
+ if len(version) != 0 {
+ log.Fatal(os.Stderr, "Version variable %q has a value: %s", vexec.VersionVariable, version)
+ }
+}
+
// TestHelperProcess isn't a real test; it's used as a helper process
// for the other tests.
func TestHelperProcess(*testing.T) {
@@ -404,6 +411,11 @@
}
defer os.Exit(0)
+ version := os.Getenv(vexec.VersionVariable)
+ if len(version) == 0 {
+ log.Fatal(os.Stderr, "Version variable %q has no value", vexec.VersionVariable)
+ }
+
// Write errors to stderr or using log. since the parent
// process is reading stderr.
args := os.Args
@@ -427,12 +439,14 @@
if err != nil {
log.Fatal(os.Stderr, "%s\n", err)
}
+ verifyNoExecVariable()
fmt.Fprintf(os.Stderr, "never ready")
case "testTooSlowToReady":
ch, err := vexec.GetChildHandle()
if err != nil {
log.Fatal(os.Stderr, "%s\n", err)
}
+ verifyNoExecVariable()
// Wait for the parent to tell us when it's ok to proceed.
controlPipe := ch.NewExtraFile(0, "control_rd")
for {
@@ -458,6 +472,7 @@
if err != nil {
log.Fatal(os.Stderr, "%s", err)
}
+ verifyNoExecVariable()
if err := ch.SetFailed(fmt.Errorf("%s", strings.Join(args, " "))); err != nil {
fmt.Fprintf(os.Stderr, "%s\n", err)
@@ -467,6 +482,7 @@
if err != nil {
log.Fatal(os.Stderr, "%s", err)
}
+ verifyNoExecVariable()
ch.SetReady()
fmt.Fprintf(os.Stderr, ".")
case "testReadySlow":
@@ -474,6 +490,7 @@
if err != nil {
log.Fatal(os.Stderr, "%s", err)
}
+ verifyNoExecVariable()
// Wait for the parent to tell us when it's ok to proceed.
controlPipe := ch.NewExtraFile(0, "control_rd")
for {
@@ -493,6 +510,7 @@
if err != nil {
log.Fatal(os.Stderr, "%s\n", err)
}
+ verifyNoExecVariable()
ch.SetReady()
rc := make(chan int)
go func() {
@@ -511,6 +529,7 @@
if err != nil {
log.Fatalf("%v", err)
} else {
+ verifyNoExecVariable()
serialized, err := ch.Config.Serialize()
if err != nil {
log.Fatalf("%v", err)
@@ -522,6 +541,7 @@
if err != nil {
log.Fatalf("%s", err)
} else {
+ verifyNoExecVariable()
fmt.Fprintf(os.Stderr, "%s", ch.Secret)
}
case "testExtraFiles":
@@ -529,6 +549,7 @@
if err != nil {
log.Fatal("error.... %s\n", err)
}
+ verifyNoExecVariable()
err = ch.SetReady()
rd := ch.NewExtraFile(0, "read")
buf := make([]byte, 1024)
diff --git a/lib/exec/parent.go b/lib/exec/parent.go
index bfcba80..a3aebec 100644
--- a/lib/exec/parent.go
+++ b/lib/exec/parent.go
@@ -66,7 +66,7 @@
// NewParentHandle creates a ParentHandle for the child process represented by
// an instance of exec.Cmd.
func NewParentHandle(c *exec.Cmd, opts ...ParentHandleOpt) *ParentHandle {
- c.Env = append(c.Env, versionVariable+"="+version1)
+
cfg, secret := NewConfig(), ""
tk := timekeeper.RealTime()
for _, opt := range opts {
@@ -92,6 +92,7 @@
// Start starts the child process, sharing a secret with it and
// setting up a communication channel over which to read its status.
func (p *ParentHandle) Start() error {
+ p.c.Env = append(p.c.Env, VersionVariable+"="+version1)
// Create anonymous pipe for communicating data between the child
// and the parent.
dataRead, dataWrite, err := os.Pipe()
diff --git a/lib/exec/shared.go b/lib/exec/shared.go
index 08c54ad..5b4058f 100644
--- a/lib/exec/shared.go
+++ b/lib/exec/shared.go
@@ -1,9 +1,18 @@
package exec
const (
- version1 = "1.0.0"
- readyStatus = "ready::"
- failedStatus = "failed::"
- initStatus = "init"
- versionVariable = "VEYRON_EXEC_VERSION"
+ version1 = "1.0.0"
+ readyStatus = "ready::"
+ failedStatus = "failed::"
+ initStatus = "init"
+
+ // The exec package uses this environment variable to communicate
+ // the version of the protocol being used between the parent and child.
+ // It takes care to clear this variable from the child process'
+ // environment as soon as it can, however, there may still be some
+ // situations where an application may need to test for its presence
+ // or ensure that it doesn't appear in a set of environment variables;
+ // exposing the name of this variable is intended to support such
+ // situations.
+ VersionVariable = "VEYRON_EXEC_VERSION"
)
diff --git a/lib/modules/exec.go b/lib/modules/exec.go
index ca5a4ff..44ef680 100644
--- a/lib/modules/exec.go
+++ b/lib/modules/exec.go
@@ -91,18 +91,6 @@
eh.mu.Unlock()
}
-// mergeOSEnv returns a slice contained the merged set of environment
-// variables from the OS environment and those in this Shell, preferring
-// values in the Shell environment over those found in the OS environment.
-func (sh *Shell) mergeOSEnvSlice() []string {
- merged := sh.mergeOSEnv()
- env := []string{}
- for k, v := range merged {
- env = append(env, k+"="+v)
- }
- return env
-}
-
func osEnvironMap() map[string]string {
m := make(map[string]string)
for _, osv := range os.Environ() {
@@ -129,6 +117,13 @@
return merged
}
+func (eh *execHandle) envelope(sh *Shell, args ...string) ([]string, []string) {
+ newargs := []string{os.Args[0]}
+ newargs = append(newargs, testFlags()...)
+ newargs = append(newargs, args...)
+ return newargs, append(sh.MergedEnv(), eh.entryPoint)
+}
+
func (eh *execHandle) start(sh *Shell, args ...string) (Handle, error) {
eh.mu.Lock()
defer eh.mu.Unlock()
@@ -138,7 +133,7 @@
// the flag package.
newargs := append(testFlags(), args[1:]...)
cmd := exec.Command(os.Args[0], newargs...)
- cmd.Env = append(sh.mergeOSEnvSlice(), eh.entryPoint)
+ cmd.Env = append(sh.MergedEnv(), eh.entryPoint)
fname := strings.TrimPrefix(eh.entryPoint, ShellEntryPoint+"=")
stderr, err := newLogfile(strings.TrimLeft(fname, "-\n\t "))
if err != nil {
@@ -176,7 +171,7 @@
defer eh.mu.Unlock()
eh.stdin.Close()
logFile := eh.stderr.Name()
- defer eh.sh.forget(eh)
+ defer eh.sh.Forget(eh)
defer func() {
os.Remove(logFile)
@@ -194,13 +189,15 @@
// Stderr is buffered to a file, so we can safely read it after we
// wait for the process.
eh.stderr.Close()
- stderrFile, err := os.Open(logFile)
- if err != nil {
- fmt.Fprintf(os.Stderr, "failed to open %q: %s\n", logFile, err)
- return procErr
+ if stderr != nil {
+ stderrFile, err := os.Open(logFile)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "failed to open %q: %s\n", logFile, err)
+ return procErr
+ }
+ readTo(stderrFile, stderr)
+ stderrFile.Close()
}
- readTo(stderrFile, stderr)
- stderrFile.Close()
return procErr
}
diff --git a/lib/modules/func.go b/lib/modules/func.go
index 69c290b..68d103b 100644
--- a/lib/modules/func.go
+++ b/lib/modules/func.go
@@ -48,6 +48,10 @@
fh.mu.Unlock()
}
+func (fh *functionHandle) envelope(sh *Shell, args ...string) ([]string, []string) {
+ return args, sh.MergedEnv()
+}
+
func (fh *functionHandle) start(sh *Shell, args ...string) (Handle, error) {
fh.mu.Lock()
defer fh.mu.Unlock()
@@ -119,7 +123,7 @@
fh.mu.Lock()
fh.stdout.r.Close()
- fh.sh.forget(fh)
+ fh.sh.Forget(fh)
fh.mu.Unlock()
return funcErr
}
diff --git a/lib/modules/modules_test.go b/lib/modules/modules_test.go
index e4357ef..3c96b07 100644
--- a/lib/modules/modules_test.go
+++ b/lib/modules/modules_test.go
@@ -6,14 +6,17 @@
"fmt"
"io"
"os"
+ "strings"
"testing"
"time"
+ "veyron.io/veyron/veyron/lib/exec"
"veyron.io/veyron/veyron/lib/modules"
)
func init() {
- modules.RegisterChild("envtest", "envtest: <variables to print>...", PrintEnv)
+ modules.RegisterChild("envtest", "envtest: <variables to print>...", PrintFromEnv)
+ modules.RegisterChild("printenv", "printenv", PrintEnv)
modules.RegisterChild("echo", "[args]*", Echo)
modules.RegisterChild("errortest", "", ErrorMain)
}
@@ -26,8 +29,8 @@
return nil
}
-func Print(stdin io.Reader, stdout, stderr io.Writer, env map[string]string, args ...string) error {
- for _, a := range args {
+func PrintFromEnv(stdin io.Reader, stdout, stderr io.Writer, env map[string]string, args ...string) error {
+ for _, a := range args[1:] {
if v := env[a]; len(v) > 0 {
fmt.Fprintf(stdout, a+"="+v+"\n")
} else {
@@ -39,16 +42,15 @@
return nil
}
+const printEnvArgPrefix = "PRINTENV_ARG="
+
func PrintEnv(stdin io.Reader, stdout, stderr io.Writer, env map[string]string, args ...string) error {
for _, a := range args {
- if v := env[a]; len(v) > 0 {
- fmt.Fprintf(stdout, a+"="+v+"\n")
- } else {
- fmt.Fprintf(stderr, "missing %s\n", a)
- }
+ fmt.Fprintf(stdout, "%s%s\n", printEnvArgPrefix, a)
}
- modules.WaitForEOF(stdin)
- fmt.Fprintf(stdout, "done\n")
+ for k, v := range env {
+ fmt.Fprintf(stdout, k+"="+v+"\n")
+ }
return nil
}
@@ -131,7 +133,7 @@
defer sh.Cleanup(nil, nil)
key, val := "simpleVar", "foo & bar & baz"
sh.SetVar(key, val)
- sh.AddFunction("envtestf", PrintEnv, "envtest: <variables to print>...")
+ sh.AddFunction("envtestf", PrintFromEnv, "envtest: <variables to print>...")
testCommand(t, sh, "envtestf", key, val)
}
@@ -167,7 +169,6 @@
if got, want := stderrBuf.String(), stderrOutput; got != want {
t.Errorf("got %q want %q", got, want)
}
-
}
func TestShutdownSubprocess(t *testing.T) {
@@ -194,9 +195,88 @@
}
}
+func find(want string, in []string) bool {
+ for _, a := range in {
+ if a == want {
+ return true
+ }
+ }
+ return false
+}
+
+func TestEnvelope(t *testing.T) {
+ sh := modules.NewShell("printenv")
+ defer sh.Cleanup(nil, nil)
+ sh.SetVar("a", "1")
+ sh.SetVar("b", "2")
+ args := []string{"oh", "ah"}
+ h, err := sh.Start("printenv", args...)
+ if err != nil {
+ t.Fatalf("unexpected error: %s", err)
+ }
+ scanner := bufio.NewScanner(h.Stdout())
+ childArgs, childEnv := []string{}, []string{}
+ for scanner.Scan() {
+ o := scanner.Text()
+ if strings.HasPrefix(o, printEnvArgPrefix) {
+ childArgs = append(childArgs, strings.TrimPrefix(o, printEnvArgPrefix))
+ } else {
+ childEnv = append(childEnv, o)
+ }
+ }
+ shArgs, shEnv := sh.CommandEnvelope("printenv", args...)
+ for _, want := range args {
+ if !find(want, childArgs) {
+ t.Errorf("failed to find %q in %s", want, childArgs)
+ }
+ if !find(want, shArgs) {
+ t.Errorf("failed to find %q in %s", want, shArgs)
+ }
+ }
+
+ for _, want := range shEnv {
+ if !find(want, childEnv) {
+ t.Errorf("failed to find %q in %s", want, shEnv)
+ }
+ }
+ for _, want := range childEnv {
+ if want == exec.VersionVariable+"=" {
+ continue
+ }
+ if !find(want, shEnv) {
+ t.Errorf("failed to find %q in %s", want, childEnv)
+ }
+ }
+}
+
+func TestEnvMerge(t *testing.T) {
+ sh := modules.NewShell("printenv")
+ defer sh.Cleanup(nil, nil)
+ sh.SetVar("a", "1")
+ os.Setenv("a", "wrong, should be 1")
+ h, err := sh.Start("printenv")
+ if err != nil {
+ t.Fatalf("unexpected error: %s", err)
+ }
+ scanner := bufio.NewScanner(h.Stdout())
+ for scanner.Scan() {
+ o := scanner.Text()
+ if strings.HasPrefix(o, "a=") {
+ if got, want := o, "a=1"; got != want {
+ t.Errorf("got: %s, want %s", got, want)
+ }
+ }
+ }
+}
+
func TestHelperProcess(t *testing.T) {
modules.DispatchInTest()
}
// TODO(cnicolaou): more complete tests for environment variables,
// OS being overridden by Shell for example.
+//
+// TODO(cnicolaou): test for one or either of the io.Writers being nil
+// on calls to Shutdown
+//
+// TODO(cnicolaou): test for error return from cleanup
diff --git a/lib/modules/shell.go b/lib/modules/shell.go
index baa2f6b..69e633a 100644
--- a/lib/modules/shell.go
+++ b/lib/modules/shell.go
@@ -179,13 +179,7 @@
// the Cleanup method. If the non-registered subprocess command does not
// exist then the Start command will return an error.
func (sh *Shell) Start(name string, args ...string) (Handle, error) {
- sh.mu.Lock()
- cmd := sh.cmds[name]
- sh.mu.Unlock()
- if cmd == nil {
- entryPoint := ShellEntryPoint + "=" + name
- cmd = &commandDesc{func() command { return newExecHandle(entryPoint) }, ""}
- }
+ cmd := sh.getCommand(name)
expanded := append([]string{name}, sh.expand(args...)...)
h, err := cmd.factory().start(sh, expanded...)
if err != nil {
@@ -197,8 +191,28 @@
return h, nil
}
-// forget tells the Shell to stop tracking the supplied Handle.
-func (sh *Shell) forget(h Handle) {
+func (sh *Shell) getCommand(name string) *commandDesc {
+ sh.mu.Lock()
+ cmd := sh.cmds[name]
+ sh.mu.Unlock()
+ if cmd == nil {
+ entryPoint := ShellEntryPoint + "=" + name
+ cmd = &commandDesc{func() command { return newExecHandle(entryPoint) }, ""}
+ }
+ return cmd
+}
+
+// CommandEnvelope returns the command line and environment that would be
+// used for running the subprocess or function if it were started with the
+// specifed arguments.
+func (sh *Shell) CommandEnvelope(name string, args ...string) ([]string, []string) {
+ return sh.getCommand(name).factory().envelope(sh, args...)
+}
+
+// Forget tells the Shell to stop tracking the supplied Handle. This is
+// generally used when the application wants to control the order that
+// commands are shutdown in.
+func (sh *Shell) Forget(h Handle) {
sh.mu.Lock()
delete(sh.handles, h)
sh.mu.Unlock()
@@ -266,6 +280,18 @@
}
}
+// MergedEnv returns a slice that contains the merged set of environment
+// variables from the OS environment and those in this Shell, preferring
+// values in the Shell environment over those found in the OS environment.
+func (sh *Shell) MergedEnv() []string {
+ merged := sh.mergeOSEnv()
+ env := []string{}
+ for k, v := range merged {
+ env = append(env, k+"="+v)
+ }
+ return env
+}
+
// Handle represents a running command.
type Handle interface {
// Stdout returns a reader to the running command's stdout stream.
@@ -299,5 +325,6 @@
// command is used to abstract the implementations of inprocess and subprocess
// commands.
type command interface {
+ envelope(sh *Shell, args ...string) ([]string, []string)
start(sh *Shell, args ...string) (Handle, error)
}
