clean up remote.Cmd api

Combine the ExitStatus and Err values from remote.Cmd into an error
returned by Wait, better matching the behavior of the os/exec package.

Non-zero exit codes are returned from Wait as a remote.ExitError.
Communicator related errors are returned directly.

Clean up all the error handling in the provisioners using a
communicator. Also remove the extra copyOutput synchronization that was
copied from package to package.

builtin/provisioners/chef/resource_provisioner.go

@@ -680,17 +680,10 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 
 	outR, outW := io.Pipe()
 	errR, errW := io.Pipe()
-	outDoneCh := make(chan struct{})
+	go p.copyOutput(o, outR)
-	errDoneCh := make(chan struct{})
+	go p.copyOutput(o, errR)
-	go p.copyOutput(o, outR, outDoneCh)
+	defer outW.Close()
-	go p.copyOutput(o, errR, errDoneCh)
+	defer errW.Close()
-	go func() {
-		// Wait for output to clean up
-		outW.Close()
-		errW.Close()
-		<-outDoneCh
-		<-errDoneCh
-	}()
 
 	cmd := &remote.Cmd{
 		Command: command,
@@ -703,20 +696,17 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 		return fmt.Errorf("Error executing command %q: %v", cmd.Command, err)
 	}
 
-	cmd.Wait()
+	if err := cmd.Wait(); err != nil {
-	if cmd.Err() != nil {
+		if rc, ok := err.(remote.ExitError); ok {
-		return cmd.Err()
+			return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc)
-	}
+		}
-
+		return err
-	if cmd.ExitStatus() != 0 {
-		return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus())
 	}
 
 	return nil
 }
 
-func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) {
+func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader) {
-	defer close(doneCh)
 	lr := linereader.New(r)
 	for line := range lr.Ch {
 		o.Output(line)

builtin/provisioners/habitat/resource_provisioner.go

@@ -729,8 +729,7 @@ func (p *provisioner) uploadUserTOML(o terraform.UIOutput, comm communicator.Com
 
 }
 
-func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) {
+func (p *provisioner) copyOutput(o terraform.UIOutput, r io.Reader) {
-	defer close(doneCh)
 	lr := linereader.New(r)
 	for line := range lr.Ch {
 		o.Output(line)
@@ -741,16 +740,10 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 	outR, outW := io.Pipe()
 	errR, errW := io.Pipe()
 
-	outDoneCh := make(chan struct{})
+	go p.copyOutput(o, outR)
-	errDoneCh := make(chan struct{})
+	go p.copyOutput(o, errR)
-	go p.copyOutput(o, outR, outDoneCh)
+	defer outW.Close()
-	go p.copyOutput(o, errR, errDoneCh)
+	defer errW.Close()
-	defer func() {
-		outW.Close()
-		errW.Close()
-		<-outDoneCh
-		<-errDoneCh
-	}()
 
 	cmd := &remote.Cmd{
 		Command: command,
@@ -762,13 +755,11 @@ func (p *provisioner) runCommand(o terraform.UIOutput, comm communicator.Communi
 		return fmt.Errorf("Error executing command %q: %v", cmd.Command, err)
 	}
 
-	cmd.Wait()
+	if err := cmd.Wait(); err != nil {
-	if cmd.Err() != nil {
+		if rc, ok := err.(remote.ExitError); ok {
-		return cmd.Err()
+			return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, rc)
-	}
+		}
-
+		return err
-	if cmd.ExitStatus() != 0 {
-		return fmt.Errorf("Command %q exited with non-zero exit status: %d", cmd.Command, cmd.ExitStatus())
 	}
 
 	return nil

builtin/provisioners/remote-exec/resource_provisioner.go

@@ -196,16 +196,14 @@ func runScripts(
 		if err := comm.Start(cmd); err != nil {
 			return fmt.Errorf("Error starting script: %v", err)
 		}
-		cmd.Wait()
 
-		if err := cmd.Err(); err != nil {
+		if err := cmd.Wait(); err != nil {
+			if rc, ok := err.(remote.ExitError); ok {
+				return fmt.Errorf("Script exited with non-zero exit status: %d", rc)
+			}
 			return fmt.Errorf("Remote command exited with error: %s", err)
 		}
 
-		if cmd.ExitStatus() != 0 {
-			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus())
-		}
-
 		// Upload a blank follow up file in the same path to prevent residual
 		// script contents from remaining on remote machine
 		empty := bytes.NewReader([]byte(""))

builtin/provisioners/salt-masterless/resource_provisioner.go

@@ -114,8 +114,6 @@ func Provisioner() terraform.ResourceProvisioner {
 // Apply executes the file provisioner
 func applyFn(ctx context.Context) error {
 	// Decode the raw config for this provisioner
-	var err error
-
 	o := ctx.Value(schema.ProvOutputKey).(terraform.UIOutput)
 	d := ctx.Value(schema.ProvConfigDataKey).(*schema.ResourceData)
 	connState := ctx.Value(schema.ProvRawStateKey).(*terraform.InstanceState)
@@ -162,21 +160,20 @@ func applyFn(ctx context.Context) error {
 			err = fmt.Errorf("Unable to download Salt: %s", err)
 		}
 
-		if err == nil {
+		if err := cmd.Wait(); err != nil {
-			cmd.Wait()
+			if rc, ok := err.(remote.ExitError); ok {
-			if cmd.Err() != nil {
+				return fmt.Errorf("Curl exited with non-zero exit status: %d", rc)
-				err = cmd.Err()
-			} else if cmd.ExitStatus() != 0 {
-				err = fmt.Errorf("Curl exited with non-zero exit status: %d", cmd.ExitStatus())
 			}
+			return err
 		}
 
 		outR, outW := io.Pipe()
 		errR, errW := io.Pipe()
-		outDoneCh := make(chan struct{})
+		go copyOutput(o, outR)
-		errDoneCh := make(chan struct{})
+		go copyOutput(o, errR)
-		go copyOutput(o, outR, outDoneCh)
+		defer outW.Close()
-		go copyOutput(o, errR, errDoneCh)
+		defer errW.Close()
+
 		cmd = &remote.Cmd{
 			Command: fmt.Sprintf("%s /tmp/install_salt.sh %s", p.sudo("sh"), p.BootstrapArgs),
 			Stdout:  outW,
@@ -184,24 +181,14 @@ func applyFn(ctx context.Context) error {
 		}
 
 		o.Output(fmt.Sprintf("Installing Salt with command %s", cmd.Command))
-		if err = comm.Start(cmd); err != nil {
+		if err := comm.Start(cmd); err != nil {
-			err = fmt.Errorf("Unable to install Salt: %s", err)
+			return fmt.Errorf("Unable to install Salt: %s", err)
 		}
 
-		if err == nil {
+		if err := cmd.Wait(); err != nil {
-			cmd.Wait()
+			if rc, ok := err.(remote.ExitError); ok {
-			if cmd.Err() != nil {
+				return fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", rc)
-				err = cmd.Err()
-			} else if cmd.ExitStatus() != 0 {
-				err = fmt.Errorf("install_salt.sh exited with non-zero exit status: %d", cmd.ExitStatus())
 			}
-		}
-		// Wait for output to clean up
-		outW.Close()
-		errW.Close()
-		<-outDoneCh
-		<-errDoneCh
-		if err != nil {
 			return err
 		}
 	}
@@ -270,11 +257,11 @@ func applyFn(ctx context.Context) error {
 
 	outR, outW := io.Pipe()
 	errR, errW := io.Pipe()
-	outDoneCh := make(chan struct{})
+	go copyOutput(o, outR)
-	errDoneCh := make(chan struct{})
+	go copyOutput(o, errR)
+	defer outW.Close()
+	defer errW.Close()
 
-	go copyOutput(o, outR, outDoneCh)
-	go copyOutput(o, errR, errDoneCh)
 	o.Output(fmt.Sprintf("Running: salt-call --local %s", p.CmdArgs))
 	cmd := &remote.Cmd{
 		Command: p.sudo(fmt.Sprintf("salt-call --local %s", p.CmdArgs)),
@@ -285,21 +272,13 @@ func applyFn(ctx context.Context) error {
 		err = fmt.Errorf("Error executing salt-call: %s", err)
 	}
 
-	if err == nil {
+	if err := cmd.Wait(); err != nil {
-		cmd.Wait()
+		if rc, ok := err.(remote.ExitError); ok {
-		if cmd.Err() != nil {
+			return fmt.Errorf("Script exited with non-zero exit status: %d", rc)
-			err = cmd.Err()
-		} else if cmd.ExitStatus() != 0 {
-			err = fmt.Errorf("Script exited with non-zero exit status: %d", cmd.ExitStatus())
 		}
+		return err
 	}
-	// Wait for output to clean up
+	return nil
-	outW.Close()
-	errW.Close()
-	<-outDoneCh
-	<-errDoneCh
-
-	return err
 }
 
 // Prepends sudo to supplied command if config says to
@@ -360,12 +339,13 @@ func (p *provisioner) moveFile(o terraform.UIOutput, comm communicator.Communica
 	if err := comm.Start(cmd); err != nil {
 		return fmt.Errorf("Unable to move %s to %s: %s", src, dst, err)
 	}
-	cmd.Wait()
+	if err := cmd.Wait(); err != nil {
-	if cmd.ExitStatus() != 0 {
+		if rc, ok := err.(remote.ExitError); ok {
-		return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, cmd.ExitStatus())
+			return fmt.Errorf("Unable to move %s to %s: exit status: %d", src, dst, rc)
+		}
+		return err
 	}
-
+	return nil
-	return cmd.Err()
 }
 
 func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error {
@@ -377,11 +357,13 @@ func (p *provisioner) createDir(o terraform.UIOutput, comm communicator.Communic
 		return err
 	}
 
-	cmd.Wait()
+	if err := cmd.Wait(); err != nil {
-	if cmd.ExitStatus() != 0 {
+		if _, ok := err.(remote.ExitError); ok {
-		return fmt.Errorf("Non-zero exit status.")
+			return fmt.Errorf("Non-zero exit status.")
+		}
+		return err
 	}
-	return cmd.Err()
+	return nil
 }
 
 func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communicator, dir string) error {
@@ -392,11 +374,13 @@ func (p *provisioner) removeDir(o terraform.UIOutput, comm communicator.Communic
 	if err := comm.Start(cmd); err != nil {
 		return err
 	}
-	cmd.Wait()
+	if err := cmd.Wait(); err != nil {
-	if cmd.ExitStatus() != 0 {
+		if _, ok := err.(remote.ExitError); ok {
-		return fmt.Errorf("Non-zero exit status.")
+			return fmt.Errorf("Non-zero exit status.")
+		}
+		return err
 	}
-	return cmd.Err()
+	return nil
 }
 
 func (p *provisioner) uploadDir(o terraform.UIOutput, comm communicator.Communicator, dst, src string, ignore []string) error {
@@ -549,8 +533,7 @@ func decodeConfig(d *schema.ResourceData) (*provisioner, error) {
 }
 
 func copyOutput(
-	o terraform.UIOutput, r io.Reader, doneCh chan<- struct{}) {
+	o terraform.UIOutput, r io.Reader) {
-	defer close(doneCh)
 	lr := linereader.New(r)
 	for line := range lr.Ch {
 		o.Output(line)

communicator/remote/command.go

@@ -1,6 +1,7 @@
 package remote
 
 import (
+	"fmt"
 	"io"
 	"sync"
 )
@@ -59,23 +60,28 @@ func (c *Cmd) SetExitStatus(status int, err error) {
 	close(c.exitCh)
 }
 
-// Err returns any communicator related error.
-func (c *Cmd) Err() error {
-	c.Lock()
-	defer c.Unlock()
-
-	return c.err
-}
-
-// ExitStatus returns the exit status of the remote command
-func (c *Cmd) ExitStatus() int {
-	c.Lock()
-	defer c.Unlock()
-
-	return c.exitStatus
-}
-
 // Wait waits for the remote command to complete.
-func (c *Cmd) Wait() {
+// Wait may return an error from the communicator, or an ExitError if the
+// process exits with a non-zero exit status.
+func (c *Cmd) Wait() error {
 	<-c.exitCh
+
+	c.Lock()
+	defer c.Unlock()
+
+	if c.err != nil {
+		return c.err
+	}
+
+	if c.exitStatus != 0 {
+		return ExitError(c.exitStatus)
+	}
+
+	return nil
+}
+
+type ExitError int
+
+func (e ExitError) Error() string {
+	return fmt.Sprintf("exit status: %d", e)
 }

communicator/ssh/communicator.go

@@ -359,11 +359,11 @@ func (c *Communicator) UploadScript(path string, input io.Reader) error {
 			"Error chmodding script file to 0777 in remote "+
 				"machine: %s", err)
 	}
-	cmd.Wait()
+
-	if cmd.ExitStatus() != 0 {
+	if err := cmd.Wait(); err != nil {
 		return fmt.Errorf(
 			"Error chmodding script file to 0777 in remote "+
-				"machine %d: %s %s", cmd.ExitStatus(), stdout.String(), stderr.String())
+				"machine %v: %s %s", err, stdout.String(), stderr.String())
 	}
 
 	return nil

communicator/ssh/communicator_test.go

@@ -219,13 +219,10 @@ func TestLostConnection(t *testing.T) {
 		c.Disconnect()
 	}()
 
-	cmd.Wait()
+	err = cmd.Wait()
-	if cmd.Err() == nil {
+	if err == nil {
 		t.Fatal("expected communicator error")
 	}
-	if cmd.ExitStatus() != 0 {
-		t.Fatal("command should not have returned an exit status")
-	}
 }
 
 func TestHostKey(t *testing.T) {