Merge pull request #10064 from hashicorp/refactor_step_run

builder/qemu: (tech-debt) Major refactor of step_run.

builder/qemu/builder.go

@@ -192,6 +192,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 
 	artifact.state["generated_data"] = state.Get("generated_data")
 	artifact.state["diskName"] = b.config.VMName
+
+	// placed in state in step_create_disk.go
 	diskpaths, ok := state.Get("qemu_disk_paths").([]string)
 	if ok {
 		artifact.state["diskPaths"] = diskpaths

builder/qemu/step_run.go

@@ -16,41 +16,45 @@ import (
 // stepRun runs the virtual machine
 type stepRun struct {
 	DiskImage bool
-}
 
-type qemuArgsTemplateData struct {
+	atLeastVersion2 bool
-	HTTPIP      string
+	ui              packer.Ui
-	HTTPPort    int
-	HTTPDir     string
-	OutputDir   string
-	Name        string
-	SSHHostPort int
 }
 
 func (s *stepRun) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
+	config := state.Get("config").(*Config)
 	driver := state.Get("driver").(Driver)
-	ui := state.Get("ui").(packer.Ui)
+	s.ui = state.Get("ui").(packer.Ui)
 
-	// Run command is different depending whether we're booting from an
+	// Figure out version of qemu; store on step for later use
-	// installation CD or a pre-baked image
+	rawVersion, err := driver.Version()
-	bootDrive := "once=d"
+	if err != nil {
-	message := "Starting VM, booting from CD-ROM"
+		err := fmt.Errorf("Error determining qemu version: %s", err)
-	if s.DiskImage {
+		s.ui.Error(err.Error())
-		bootDrive = "c"
+		return multistep.ActionHalt
-		message = "Starting VM, booting disk image"
 	}
-	ui.Say(message)
+	qemuVersion, err := version.NewVersion(rawVersion)
+	if err != nil {
+		err := fmt.Errorf("Error parsing qemu version: %s", err)
+		s.ui.Error(err.Error())
+		return multistep.ActionHalt
+	}
+	v2 := version.Must(version.NewVersion("2.0"))
 
-	command, err := getCommandArgs(bootDrive, state)
+	s.atLeastVersion2 = qemuVersion.GreaterThanOrEqual(v2)
+
+	// Generate the qemu command
+	command, err := s.getCommandArgs(config, state)
 	if err != nil {
 		err := fmt.Errorf("Error processing QemuArgs: %s", err)
-		ui.Error(err.Error())
+		s.ui.Error(err.Error())
 		return multistep.ActionHalt
 	}
 
+	// run the qemu command
 	if err := driver.Qemu(command...); err != nil {
 		err := fmt.Errorf("Error launching VM: %s", err)
-		ui.Error(err.Error())
+		s.ui.Error(err.Error())
 		return multistep.ActionHalt
 	}
 
@@ -66,140 +70,187 @@ func (s *stepRun) Cleanup(state multistep.StateBag) {
 	}
 }
 
-func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error) {
+func (s *stepRun) getDefaultArgs(config *Config, state multistep.StateBag) map[string]interface{} {
-	config := state.Get("config").(*Config)
-	isoPath := state.Get("iso_path").(string)
-	vncIP := config.VNCBindAddress
-	vncPort := state.Get("vnc_port").(int)
-	ui := state.Get("ui").(packer.Ui)
-	driver := state.Get("driver").(Driver)
-	vmName := config.VMName
-	imgPath := filepath.Join(config.OutputDir, vmName)
 
 	defaultArgs := make(map[string]interface{})
-	var deviceArgs []string
+
-	var driveArgs []string
+	// Configure "boot" arguement
-	var commHostPort int
+	// Run command is different depending whether we're booting from an
+	// installation CD or a pre-baked image
+	bootDrive := "once=d"
+	message := "Starting VM, booting from CD-ROM"
+	if s.DiskImage {
+		bootDrive = "c"
+		message = "Starting VM, booting disk image"
+	}
+	s.ui.Say(message)
+	defaultArgs["-boot"] = bootDrive
+
+	// configure "-qmp" arguments
+	if config.QMPEnable {
+		defaultArgs["-qmp"] = fmt.Sprintf("unix:%s,server,nowait", config.QMPSocketPath)
+	}
+
+	// configure "-name" arguments
+	defaultArgs["-name"] = config.VMName
+
+	// Configure "-machine" arguments
+	if config.Accelerator == "none" {
+		defaultArgs["-machine"] = fmt.Sprintf("type=%s", config.MachineType)
+		s.ui.Message("WARNING: The VM will be started with no hardware acceleration.\n" +
+			"The installation may take considerably longer to finish.\n")
+	} else {
+		defaultArgs["-machine"] = fmt.Sprintf("type=%s,accel=%s",
+			config.MachineType, config.Accelerator)
+	}
+
+	// Configure "-netdev" arguments
+	defaultArgs["-netdev"] = fmt.Sprintf("bridge,id=user.0,br=%s", config.NetBridge)
+	if config.NetBridge == "" {
+		defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0")
+		if config.CommConfig.Comm.Type != "none" {
+			commHostPort := state.Get("commHostPort").(int)
+			defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0,hostfwd=tcp::%v-:%d", commHostPort, config.CommConfig.Comm.Port())
+		}
+	}
+
+	// Configure "-vnc" arguments
+	// vncPort is always set in stepConfigureVNC, so we don't need to
+	// defensively assert
+	vncPort := state.Get("vnc_port").(int)
+	vncIP := config.VNCBindAddress
 
 	vncPort = vncPort - config.VNCPortMin
 	vnc := fmt.Sprintf("%s:%d", vncIP, vncPort)
 	if config.VNCUsePassword {
 		vnc = fmt.Sprintf("%s:%d,password", vncIP, vncPort)
 	}
+	defaultArgs["-vnc"] = vnc
 
-	if config.QMPEnable {
+	// Track the connection for the user
-		defaultArgs["-qmp"] = fmt.Sprintf("unix:%s,server,nowait", config.QMPSocketPath)
+	vncPass, _ := state.Get("vnc_password").(string)
+
+	message = getVncConnectionMessage(config.Headless, vnc, vncPass)
+	if message != "" {
+		s.ui.Message(message)
 	}
 
-	defaultArgs["-name"] = vmName
+	// Configure "-m" memory argument
-	defaultArgs["-machine"] = fmt.Sprintf("type=%s", config.MachineType)
+	defaultArgs["-m"] = fmt.Sprintf("%dM", config.MemorySize)
 
-	if config.NetBridge == "" {
+	// Configure "-smp" processor hardware arguments
-		if config.CommConfig.Comm.Type != "none" {
+	if config.CpuCount > 1 {
-			commHostPort = state.Get("commHostPort").(int)
+		defaultArgs["-smp"] = fmt.Sprintf("cpus=%d,sockets=%d", config.CpuCount, config.CpuCount)
-			defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0,hostfwd=tcp::%v-:%d", commHostPort, config.CommConfig.Comm.Port())
+	}
-		} else {
+
-			defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0")
+	// Configure "-fda" floppy disk attachment
-		}
+	if floppyPathRaw, ok := state.GetOk("floppy_path"); ok {
+		defaultArgs["-fda"] = floppyPathRaw.(string)
 	} else {
-		defaultArgs["-netdev"] = fmt.Sprintf("bridge,id=user.0,br=%s", config.NetBridge)
+		log.Println("Qemu Builder has no floppy files, not attaching a floppy.")
 	}
 
-	rawVersion, err := driver.Version()
+	// Configure GUI display
-	if err != nil {
+	if !config.Headless {
-		return nil, err
+		if s.atLeastVersion2 {
-	}
+			// FIXME: "none" is a valid display option in qemu but we have
-	qemuVersion, err := version.NewVersion(rawVersion)
+			// departed from the qemu usage here to instaed mean "let qemu
-	if err != nil {
+			// set a reasonable default". We need to deprecate this behavior
-		return nil, err
+			// and let users just set "UseDefaultDisplay" if they want to let
-	}
+			// qemu do its thing.
-	v2 := version.Must(version.NewVersion("2.0"))
+			if len(config.Display) > 0 && config.Display != "none" {
-
+				defaultArgs["-display"] = config.Display
-	if qemuVersion.GreaterThanOrEqual(v2) {
-		if config.DiskInterface == "virtio-scsi" {
-			if config.DiskImage {
-				deviceArgs = append(deviceArgs, "virtio-scsi-pci,id=scsi0", "scsi-hd,bus=scsi0.0,drive=drive0")
-				driveArgumentString := fmt.Sprintf("if=none,file=%s,id=drive0,cache=%s,discard=%s,format=%s", imgPath, config.DiskCache, config.DiskDiscard, config.Format)
-				if config.DetectZeroes != "off" {
-					driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes)
-				}
-				driveArgs = append(driveArgs, driveArgumentString)
-			} else {
-				deviceArgs = append(deviceArgs, "virtio-scsi-pci,id=scsi0")
-				diskFullPaths := state.Get("qemu_disk_paths").([]string)
-				for i, diskFullPath := range diskFullPaths {
-					deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi0.0,drive=drive%d", i))
-					driveArgumentString := fmt.Sprintf("if=none,file=%s,id=drive%d,cache=%s,discard=%s,format=%s", diskFullPath, i, config.DiskCache, config.DiskDiscard, config.Format)
-					if config.DetectZeroes != "off" {
-						driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes)
-					}
-					driveArgs = append(driveArgs, driveArgumentString)
-				}
-			}
-		} else {
-			if config.DiskImage {
-				driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format)
-				if config.DetectZeroes != "off" {
-					driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes)
-				}
-				driveArgs = append(driveArgs, driveArgumentString)
-			} else {
-				diskFullPaths := state.Get("qemu_disk_paths").([]string)
-				for _, diskFullPath := range diskFullPaths {
-					driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", diskFullPath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format)
-					if config.DetectZeroes != "off" {
-						driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes)
-					}
-					driveArgs = append(driveArgs, driveArgumentString)
-				}
-			}
-		}
-	} else {
-		driveArgs = append(driveArgs, fmt.Sprintf("file=%s,if=%s,cache=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.Format))
-	}
-	deviceArgs = append(deviceArgs, fmt.Sprintf("%s,netdev=user.0", config.NetDevice))
-
-	if config.Headless == true {
-		vncPortRaw, vncPortOk := state.GetOk("vnc_port")
-		vncPass := state.Get("vnc_password")
-
-		if vncPortOk && vncPass != nil && len(vncPass.(string)) > 0 {
-			vncPort := vncPortRaw.(int)
-
-			ui.Message(fmt.Sprintf(
-				"The VM will be run headless, without a GUI. If you want to\n"+
-					"view the screen of the VM, connect via VNC to vnc://%s:%d\n"+
-					"with the password: %s", vncIP, vncPort, vncPass))
-		} else if vncPortOk {
-			vncPort := vncPortRaw.(int)
-
-			ui.Message(fmt.Sprintf(
-				"The VM will be run headless, without a GUI. If you want to\n"+
-					"view the screen of the VM, connect via VNC without a password to\n"+
-					"vnc://%s:%d", vncIP, vncPort))
-		} else {
-			ui.Message("The VM will be run headless, without a GUI, as configured.\n" +
-				"If the run isn't succeeding as you expect, please enable the GUI\n" +
-				"to inspect the progress of the build.")
-		}
-	} else {
-		if qemuVersion.GreaterThanOrEqual(v2) {
-			if len(config.Display) > 0 {
-				if config.Display != "none" {
-					defaultArgs["-display"] = config.Display
-				}
 			} else if !config.UseDefaultDisplay {
 				defaultArgs["-display"] = "gtk"
 			}
 		} else {
-			ui.Message("WARNING: The version of qemu  on your host doesn't support display mode.\n" +
+			s.ui.Message("WARNING: The version of qemu  on your host doesn't support display mode.\n" +
 				"The display parameter will be ignored.")
 		}
 	}
 
+	deviceArgs, driveArgs := s.getDeviceAndDriveArgs(config, state)
+	defaultArgs["-device"] = deviceArgs
+	defaultArgs["-drive"] = driveArgs
+
+	return defaultArgs
+}
+
+func getVncConnectionMessage(headless bool, vnc string, vncPass string) string {
+	// Configure GUI display
+	if headless {
+		if vnc == "" {
+			return "The VM will be run headless, without a GUI, as configured.\n" +
+				"If the run isn't succeeding as you expect, please enable the GUI\n" +
+				"to inspect the progress of the build."
+		}
+
+		if vncPass != "" {
+			return fmt.Sprintf(
+				"The VM will be run headless, without a GUI. If you want to\n"+
+					"view the screen of the VM, connect via VNC to vnc://%s\n"+
+					"with the password: %s", vnc, vncPass)
+		}
+
+		return fmt.Sprintf(
+			"The VM will be run headless, without a GUI. If you want to\n"+
+				"view the screen of the VM, connect via VNC without a password to\n"+
+				"vnc://%s", vnc)
+	}
+	return ""
+}
+
+func (s *stepRun) getDeviceAndDriveArgs(config *Config, state multistep.StateBag) ([]string, []string) {
+	var deviceArgs []string
+	var driveArgs []string
+
+	vmName := config.VMName
+	imgPath := filepath.Join(config.OutputDir, vmName)
+
+	// Configure virtual hard drives
+	if s.atLeastVersion2 {
+		// We have different things to attach based on whether we are booting
+		// from an iso or a boot image.
+		drivesToAttach := []string{}
+		if config.DiskImage {
+			drivesToAttach = append(drivesToAttach, imgPath)
+		}
+
+		diskFullPaths := state.Get("qemu_disk_paths").([]string)
+		drivesToAttach = append(drivesToAttach, diskFullPaths...)
+
+		for i, drivePath := range drivesToAttach {
+			driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", drivePath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format)
+			if config.DiskInterface == "virtio-scsi" {
+				// TODO: Megan: Remove this conditional. This, and the code
+				// under the TODO below, reproduce the old behavior. While it
+				// may be broken, the goal of this commit is to refactor in a way
+				// that creates a result that is testably the same as the old
+				// code. A pr will follow fixing this broken behavior.
+				if i == 0 {
+					deviceArgs = append(deviceArgs, fmt.Sprintf("virtio-scsi-pci,id=scsi%d", i))
+				}
+				// TODO: Megan: When you remove above conditional,
+				// set deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi%d.0,drive=drive%d", i, i))
+				deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi0.0,drive=drive%d", i))
+				driveArgumentString = fmt.Sprintf("if=none,file=%s,id=drive%d,cache=%s,discard=%s,format=%s", drivePath, i, config.DiskCache, config.DiskDiscard, config.Format)
+			}
+			if config.DetectZeroes != "off" {
+				driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes)
+			}
+			driveArgs = append(driveArgs, driveArgumentString)
+		}
+	} else {
+		driveArgs = append(driveArgs, fmt.Sprintf("file=%s,if=%s,cache=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.Format))
+	}
+
+	deviceArgs = append(deviceArgs, fmt.Sprintf("%s,netdev=user.0", config.NetDevice))
+
+	// Configure virtual CDs
 	cdPaths := []string{}
 	// Add the installation CD to the run command
 	if !config.DiskImage {
+		isoPath := state.Get("iso_path").(string)
 		cdPaths = append(cdPaths, isoPath)
 	}
 	// Add our custom CD created from cd_files, if it exists
@@ -220,64 +271,48 @@ func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error
 		}
 	}
 
-	defaultArgs["-device"] = deviceArgs
+	return deviceArgs, driveArgs
-	defaultArgs["-drive"] = driveArgs
+}
 
-	defaultArgs["-boot"] = bootDrive
+func (s *stepRun) applyUserOverrides(defaultArgs map[string]interface{}, config *Config, state multistep.StateBag) ([]string, error) {
-	defaultArgs["-m"] = fmt.Sprintf("%dM", config.MemorySize)
+	// Done setting up defaults; time to process user args and defaults together
-	if config.CpuCount > 1 {
+	// and generate output args
-		defaultArgs["-smp"] = fmt.Sprintf("cpus=%d,sockets=%d", config.CpuCount, config.CpuCount)
-	}
-	defaultArgs["-vnc"] = vnc
-
-	// Append the accelerator to the machine type if it is specified
-	if config.Accelerator != "none" {
-		defaultArgs["-machine"] = fmt.Sprintf("%s,accel=%s", defaultArgs["-machine"], config.Accelerator)
-	} else {
-		ui.Message("WARNING: The VM will be started with no hardware acceleration.\n" +
-			"The installation may take considerably longer to finish.\n")
-	}
-
-	// Determine if we have a floppy disk to attach
-	if floppyPathRaw, ok := state.GetOk("floppy_path"); ok {
-		defaultArgs["-fda"] = floppyPathRaw.(string)
-	} else {
-		log.Println("Qemu Builder has no floppy files, not attaching a floppy.")
-	}
 
 	inArgs := make(map[string][]string)
 	if len(config.QemuArgs) > 0 {
-		ui.Say("Overriding default Qemu arguments with QemuArgs...")
+		s.ui.Say("Overriding default Qemu arguments with qemuargs template option...")
 
+		commHostPort := state.Get("commHostPort").(int)
 		httpIp := state.Get("http_ip").(string)
 		httpPort := state.Get("http_port").(int)
-		ictx := config.ctx
+
-		if config.CommConfig.Comm.Type != "none" {
+		type qemuArgsTemplateData struct {
-			ictx.Data = qemuArgsTemplateData{
+			HTTPIP      string
-				httpIp,
+			HTTPPort    int
-				httpPort,
+			HTTPDir     string
-				config.HTTPDir,
+			OutputDir   string
-				config.OutputDir,
+			Name        string
-				config.VMName,
+			SSHHostPort int
-				commHostPort,
-			}
-		} else {
-			ictx.Data = qemuArgsTemplateData{
-				HTTPIP:    httpIp,
-				HTTPPort:  httpPort,
-				HTTPDir:   config.HTTPDir,
-				OutputDir: config.OutputDir,
-				Name:      config.VMName,
-			}
 		}
+
+		ictx := config.ctx
+		ictx.Data = qemuArgsTemplateData{
+			HTTPIP:      httpIp,
+			HTTPPort:    httpPort,
+			HTTPDir:     config.HTTPDir,
+			OutputDir:   config.OutputDir,
+			Name:        config.VMName,
+			SSHHostPort: commHostPort,
+		}
+
+		// Interpolate each string in qemuargs
 		newQemuArgs, err := processArgs(config.QemuArgs, &ictx)
 		if err != nil {
 			return nil, err
 		}
 
-		// because qemu supports multiple appearances of the same
+		// Qemu supports multiple appearances of the same switch. This means
-		// switch, just different values, each key in the args hash
+		// each key in the args hash will have an array of string values
-		// will have an array of string values
 		for _, qemuArgs := range newQemuArgs {
 			key := qemuArgs[0]
 			val := strings.Join(qemuArgs[1:], "")
@@ -326,6 +361,12 @@ func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error
 	return outArgs, nil
 }
 
+func (s *stepRun) getCommandArgs(config *Config, state multistep.StateBag) ([]string, error) {
+	defaultArgs := s.getDefaultArgs(config, state)
+
+	return s.applyUserOverrides(defaultArgs, config, state)
+}
+
 func processArgs(args [][]string, ctx *interpolate.Context) ([][]string, error) {
 	var err error
 

builder/qemu/step_run_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/hashicorp/packer/common"
+	"github.com/hashicorp/packer/helper/communicator"
 	"github.com/hashicorp/packer/helper/multistep"
 	"github.com/hashicorp/packer/packer"
 	"github.com/stretchr/testify/assert"
@@ -11,8 +13,6 @@ import (
 
 func runTestState(t *testing.T, config *Config) multistep.StateBag {
 	state := new(multistep.BasicStateBag)
-
-	state.Put("ui", packer.TestUi(t))
 	state.Put("config", config)
 
 	d := new(DriverMock)
@@ -31,86 +31,396 @@ func runTestState(t *testing.T, config *Config) multistep.StateBag {
 	return state
 }
 
-func Test_getCommandArgs(t *testing.T) {
+func Test_UserOverrides(t *testing.T) {
-	state := runTestState(t, &Config{})
+	type testCase struct {
-
+		Config   *Config
-	args, err := getCommandArgs("", state)
+		Expected []string
-	if err != nil {
+		Reason   string
-		t.Fatalf("should not have an error getting args. Error: %s", err)
+	}
+	testcases := []testCase{
+		{
+			&Config{
+				HTTPConfig: common.HTTPConfig{
+					HTTPDir: "http/directory",
+				},
+				OutputDir: "output/directory",
+				VMName:    "myvm",
+				QemuArgs: [][]string{
+					{"-randomflag1", "{{.HTTPIP}}-{{.HTTPPort}}-{{.HTTPDir}}"},
+					{"-randomflag2", "{{.OutputDir}}-{{.Name}}"},
+				},
+			},
+			[]string{
+				"-display", "gtk",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-randomflag1", "127.0.0.1-1234-http/directory",
+				"-randomflag2", "output/directory-myvm",
+				"-device", ",netdev=user.0",
+			},
+			"Test that interpolation overrides work.",
+		},
+		{
+			&Config{
+				VMName:   "myvm",
+				QemuArgs: [][]string{{"-display", "partydisplay"}},
+			},
+			[]string{
+				"-display", "partydisplay",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-device", ",netdev=user.0",
+			},
+			"User input overrides default, rest is populated as normal",
+		},
+		{
+			&Config{
+				VMName:    "myvm",
+				NetDevice: "mynetdevice",
+				QemuArgs:  [][]string{{"-device", "somerandomdevice"}},
+			},
+			[]string{
+				"-display", "gtk",
+				"-device", "somerandomdevice",
+				"-device", "mynetdevice,netdev=user.0",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"Net device gets added",
+		},
 	}
 
-	expected := []string{
+	for _, tc := range testcases {
-		"-display", "gtk",
+		state := runTestState(t, tc.Config)
-		"-m", "0M",
+
-		"-boot", "",
+		step := &stepRun{
-		"-fda", "fake_floppy_path",
+			atLeastVersion2: true,
-		"-name", "",
+			ui:              packer.TestUi(t),
-		"-netdev", "user,id=user.0,hostfwd=tcp::5000-:0",
+		}
-		"-vnc", ":5905",
+		args, err := step.getCommandArgs(tc.Config, state)
-		"-machine", "type=,accel=",
+		if err != nil {
-		"-device", ",netdev=user.0",
+			t.Fatalf("should not have an error getting args. Error: %s", err)
-		"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+		}
+
+		expected := append([]string{
+			"-m", "0M",
+			"-boot", "once=d",
+			"-fda", "fake_floppy_path",
+			"-name", "myvm",
+			"-netdev", "user,id=user.0,hostfwd=tcp::5000-:0",
+			"-vnc", ":5905",
+			"-machine", "type=,accel="},
+			tc.Expected...)
+
+		assert.ElementsMatch(t, args, expected,
+			fmt.Sprintf("%s, \nRecieved: %#v", tc.Reason, args))
 	}
 
-	assert.ElementsMatch(t, args, expected, "unexpected generated args")
 }
 
-func Test_CDFilesPath(t *testing.T) {
+func Test_DriveAndDeviceArgs(t *testing.T) {
-	// cd_path is set and DiskImage is false
+	type testCase struct {
-	state := runTestState(t, &Config{})
+		Config     *Config
-	state.Put("cd_path", "fake_cd_path.iso")
+		ExtraState map[string]interface{}
-
+		Step       *stepRun
-	args, err := getCommandArgs("", state)
+		Expected   []string
-	if err != nil {
+		Reason     string
-		t.Fatalf("should not have an error getting args. Error: %s", err)
 	}
 
-	expected := []string{
+	testcases := []testCase{
-		"-display", "gtk",
+		{
-		"-m", "0M",
+			&Config{},
-		"-boot", "",
+			map[string]interface{}{},
-		"-fda", "fake_floppy_path",
+			&stepRun{
-		"-name", "",
+				atLeastVersion2: true,
-		"-netdev", "user,id=user.0,hostfwd=tcp::5000-:0",
+				ui:              packer.TestUi(t),
-		"-vnc", ":5905",
+			},
-		"-machine", "type=,accel=",
+			[]string{
-		"-device", ",netdev=user.0",
+				"-display", "gtk",
-		"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-boot", "once=d",
-		"-drive", "file=fake_cd_path.iso,index=1,media=cdrom",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"Boot value should default to once=d when diskImage isn't set",
+		},
+		{
+			&Config{
+				DiskImage:     true,
+				DiskInterface: "virtio-scsi",
+
+				OutputDir:    "path_to_output",
+				DiskCache:    "writeback",
+				Format:       "qcow2",
+				DetectZeroes: "off",
+			},
+			map[string]interface{}{
+				"cd_path": "fake_cd_path.iso",
+			},
+			&stepRun{
+				DiskImage:       true,
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "c",
+				"-device", "virtio-scsi-pci,id=scsi0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
+				"-drive", "if=none,file=path_to_output,id=drive0,cache=writeback,discard=,format=qcow2",
+				"-drive", "file=fake_cd_path.iso,index=0,media=cdrom",
+			},
+			"virtio-scsi interface, DiskImage true, extra cdrom, detectZeroes off",
+		},
+		{
+			&Config{
+				DiskImage:     true,
+				DiskInterface: "virtio-scsi",
+
+				OutputDir:    "path_to_output",
+				DiskCache:    "writeback",
+				Format:       "qcow2",
+				DetectZeroes: "on",
+			},
+			map[string]interface{}{
+				"cd_path": "fake_cd_path.iso",
+			},
+			&stepRun{
+				DiskImage:       true,
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "c",
+				"-device", "virtio-scsi-pci,id=scsi0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
+				"-drive", "if=none,file=path_to_output,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=on",
+				"-drive", "file=fake_cd_path.iso,index=0,media=cdrom",
+			},
+			"virtio-scsi interface, DiskImage true, extra cdrom, detectZeroes on",
+		},
+		{
+			&Config{
+				DiskInterface: "virtio-scsi",
+
+				OutputDir:    "path_to_output",
+				DiskCache:    "writeback",
+				Format:       "qcow2",
+				DetectZeroes: "off",
+			},
+			map[string]interface{}{
+				"cd_path": "fake_cd_path.iso",
+				// when disk image is false, we will always have at least one
+				// disk path: the one we create to be the main disk.
+				"qemu_disk_paths": []string{"qemupath1", "qemupath2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-device", "virtio-scsi-pci,id=scsi0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive1",
+				"-drive", "if=none,file=qemupath1,id=drive0,cache=writeback,discard=,format=qcow2",
+				"-drive", "if=none,file=qemupath2,id=drive1,cache=writeback,discard=,format=qcow2",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-drive", "file=fake_cd_path.iso,index=1,media=cdrom",
+			},
+			"virtio-scsi interface, bootable iso, cdrom",
+		},
+		{
+			&Config{
+				DiskInterface: "virtio-scsi",
+
+				OutputDir:    "path_to_output",
+				DiskCache:    "writeback",
+				Format:       "qcow2",
+				DetectZeroes: "on",
+			},
+			map[string]interface{}{
+				"cd_path": "fake_cd_path.iso",
+				// when disk image is false, we will always have at least one
+				// disk path: the one we create to be the main disk.
+				"qemu_disk_paths": []string{"qemupath1", "qemupath2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-device", "virtio-scsi-pci,id=scsi0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive1",
+				"-drive", "if=none,file=qemupath1,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=on",
+				"-drive", "if=none,file=qemupath2,id=drive1,cache=writeback,discard=,format=qcow2,detect-zeroes=on",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-drive", "file=fake_cd_path.iso,index=1,media=cdrom",
+			},
+			"virtio-scsi interface, DiskImage false, extra cdrom, detect zeroes on",
+		},
+		{
+			&Config{
+				DiskInterface: "virtio-scsi",
+
+				OutputDir: "path_to_output",
+				DiskCache: "writeback",
+				Format:    "qcow2",
+			},
+			map[string]interface{}{
+				// when disk image is false, we will always have at least one
+				// disk path: the one we create to be the main disk.
+				"qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-device", "virtio-scsi-pci,id=scsi0",
+				"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
+				"-drive", "if=none,file=output/dir/path/mydisk.qcow2,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"virtio-scsi interface, DiskImage false, no extra disks or cds",
+		},
+		{
+			&Config{},
+			map[string]interface{}{
+				"cd_path":         "fake_cd_path.iso",
+				"qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-drive", "file=output/dir/path/mydisk.qcow2,if=,cache=,discard=,format=,detect-zeroes=",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+				"-drive", "file=fake_cd_path.iso,index=1,media=cdrom",
+			},
+			"cd_path is set and DiskImage is false",
+		},
+		{
+			&Config{},
+			map[string]interface{}{
+				// when disk image is false, we will always have at least one
+				// disk path: the one we create to be the main disk.
+				"qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-drive", "file=output/dir/path/mydisk.qcow2,if=,cache=,discard=,format=,detect-zeroes=",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"empty config",
+		},
+		{
+			&Config{
+				OutputDir:     "path_to_output",
+				DiskInterface: "virtio",
+				DiskCache:     "writeback",
+				Format:        "qcow2",
+			},
+			map[string]interface{}{
+				// when disk image is false, we will always have at least one
+				// disk path: the one we create to be the main disk.
+				"qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"},
+			},
+			&stepRun{
+				atLeastVersion2: false,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-boot", "once=d",
+				"-drive", "file=path_to_output,if=virtio,cache=writeback,format=qcow2",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"version less than 2",
+		},
+		{
+			&Config{
+				OutputDir:     "path_to_output",
+				DiskInterface: "virtio",
+				DiskCache:     "writeback",
+				Format:        "qcow2",
+			},
+			map[string]interface{}{
+				"cd_path":         "fake_cd_path.iso",
+				"qemu_disk_paths": []string{"qemupath1", "qemupath2"},
+			},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "once=d",
+				"-drive", "file=qemupath1,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=",
+				"-drive", "file=qemupath2,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=",
+				"-drive", "file=fake_cd_path.iso,index=1,media=cdrom",
+				"-drive", "file=/path/to/test.iso,index=0,media=cdrom",
+			},
+			"virtio interface with extra disks",
+		},
+		{
+			&Config{
+				DiskImage:     true,
+				OutputDir:     "path_to_output",
+				DiskInterface: "virtio",
+				DiskCache:     "writeback",
+				Format:        "qcow2",
+			},
+			map[string]interface{}{
+				"cd_path": "fake_cd_path.iso",
+			},
+			&stepRun{
+				DiskImage:       true,
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{
+				"-display", "gtk",
+				"-boot", "c",
+				"-drive", "file=path_to_output,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=",
+				"-drive", "file=fake_cd_path.iso,index=0,media=cdrom",
+			},
+			"virtio interface with disk image",
+		},
 	}
+	for _, tc := range testcases {
+		state := runTestState(t, &Config{})
+		for k, v := range tc.ExtraState {
+			state.Put(k, v)
+		}
 
-	assert.ElementsMatch(t, args, expected, fmt.Sprintf("unexpected generated args: %#v", args))
+		args, err := tc.Step.getCommandArgs(tc.Config, state)
+		if err != nil {
+			t.Fatalf("should not have an error getting args. Error: %s", err)
+		}
 
-	// cd_path is set and DiskImage is true
+		expected := append([]string{
-	config := &Config{
+			"-m", "0M",
-		DiskImage:     true,
+			"-fda", "fake_floppy_path",
-		DiskInterface: "virtio-scsi",
+			"-name", "",
+			"-netdev", "user,id=user.0,hostfwd=tcp::5000-:0",
+			"-vnc", ":5905",
+			"-machine", "type=,accel=",
+			"-device", ",netdev=user.0"},
+			tc.Expected...)
+
+		assert.ElementsMatch(t, args, expected,
+			fmt.Sprintf("%s, \nRecieved: %#v", tc.Reason, args))
 	}
-	state = runTestState(t, config)
-	state.Put("cd_path", "fake_cd_path.iso")
-
-	args, err = getCommandArgs("c", state)
-	if err != nil {
-		t.Fatalf("should not have an error getting args. Error: %s", err)
-	}
-
-	expected = []string{
-		"-display", "gtk",
-		"-m", "0M",
-		"-boot", "c",
-		"-fda", "fake_floppy_path",
-		"-name", "",
-		"-netdev", "user,id=user.0,hostfwd=tcp::5000-:0",
-		"-vnc", ":5905",
-		"-machine", "type=,accel=",
-		"-device", ",netdev=user.0",
-		"-device", "virtio-scsi-pci,id=scsi0",
-		"-device", "scsi-hd,bus=scsi0.0,drive=drive0",
-		"-drive", "if=none,file=,id=drive0,cache=,discard=,format=,detect-zeroes=",
-		"-drive", "file=fake_cd_path.iso,index=0,media=cdrom",
-	}
-
-	assert.ElementsMatch(t, args, expected, fmt.Sprintf("unexpected generated args: %#v", args))
 }
 
 func Test_OptionalConfigOptionsGetSet(t *testing.T) {
@@ -124,8 +434,11 @@ func Test_OptionalConfigOptionsGetSet(t *testing.T) {
 	}
 
 	state := runTestState(t, c)
-
+	step := &stepRun{
-	args, err := getCommandArgs("once=d", state)
+		atLeastVersion2: true,
+		ui:              packer.TestUi(t),
+	}
+	args, err := step.getCommandArgs(c, state)
 	if err != nil {
 		t.Fatalf("should not have an error getting args. Error: %s", err)
 	}
@@ -144,5 +457,251 @@ func Test_OptionalConfigOptionsGetSet(t *testing.T) {
 		"-qmp", "unix:qmp_path,server,nowait",
 	}
 
-	assert.ElementsMatch(t, args, expected, "password flag should be set, and d drive should be set.")
+	assert.ElementsMatch(t, args, expected, "password flag should be set, and d drive should be set: %s", args)
+}
+
+// Tests for presence of Packer-generated arguments. Doesn't test that
+// arguments which shouldn't be there are absent.
+func Test_Defaults(t *testing.T) {
+	type testCase struct {
+		Config     *Config
+		ExtraState map[string]interface{}
+		Step       *stepRun
+		Expected   []string
+		Reason     string
+	}
+
+	testcases := []testCase{
+		{
+			&Config{},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-boot", "once=d"},
+			"Boot value should default to once=d",
+		},
+		{
+			&Config{},
+			map[string]interface{}{},
+			&stepRun{
+				DiskImage: true,
+				ui:        packer.TestUi(t),
+			},
+			[]string{"-boot", "c"},
+			"Boot value should be set to c when DiskImage is set on step",
+		},
+		{
+			&Config{
+				QMPEnable:     true,
+				QMPSocketPath: "/path/to/socket",
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-qmp", "unix:/path/to/socket,server,nowait"},
+			"Args should contain -qmp when qmp_enable is set",
+		},
+		{
+			&Config{
+				QMPEnable: true,
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-qmp", "unix:,server,nowait"},
+			"Args contain -qmp even when socket path isn't set, if qmp enabled",
+		},
+		{
+			&Config{
+				VMName: "partyname",
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-name", "partyname"},
+			"Name is set from config",
+		},
+		{
+			&Config{},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-name", ""},
+			"Name is set from config, even when name is blank (which won't " +
+				"happen for real thanks to defaulting in build prepare)",
+		},
+		{
+			&Config{
+				Accelerator: "none",
+				MachineType: "fancymachine",
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-machine", "type=fancymachine"},
+			"Don't add accelerator tag when no accelerator is set.",
+		},
+		{
+			&Config{
+				Accelerator: "kvm",
+				MachineType: "fancymachine",
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-machine", "type=fancymachine,accel=kvm"},
+			"Add accelerator tag when accelerator is set.",
+		},
+		{
+			&Config{
+				NetBridge: "fakebridge",
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-netdev", "bridge,id=user.0,br=fakebridge"},
+			"Add netbridge tag when netbridge is set.",
+		},
+		{
+			&Config{
+				CommConfig: CommConfig{
+					Comm: communicator.Config{
+						Type: "none",
+					},
+				},
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-netdev", "user,id=user.0"},
+			"No host forwarding when no net bridge and no communicator",
+		},
+		{
+			&Config{
+				CommConfig: CommConfig{
+					Comm: communicator.Config{
+						Type: "ssh",
+						SSH: communicator.SSH{
+							SSHPort: 4567,
+						},
+					},
+				},
+			},
+			map[string]interface{}{
+				"commHostPort": 1111,
+			},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-netdev", "user,id=user.0,hostfwd=tcp::1111-:4567"},
+			"Host forwarding when a communicator is configured",
+		},
+		{
+			&Config{
+				VNCBindAddress: "1.1.1.1",
+			},
+			map[string]interface{}{
+				"vnc_port": 5959,
+			},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-vnc", "1.1.1.1:5959"},
+			"no VNC password should be set",
+		},
+		{
+			&Config{
+				VNCBindAddress: "1.1.1.1",
+				VNCUsePassword: true,
+			},
+			map[string]interface{}{
+				"vnc_port": 5959,
+			},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-vnc", "1.1.1.1:5959,password"},
+			"VNC password should be set",
+		},
+		{
+			&Config{
+				MemorySize: 2345,
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-m", "2345M"},
+			"Memory is set, with unit M",
+		},
+		{
+			&Config{
+				CpuCount: 2,
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-smp", "cpus=2,sockets=2"},
+			"both cpus and sockets are set to config's CpuCount",
+		},
+		{
+			&Config{
+				CpuCount: 2,
+			},
+			map[string]interface{}{},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-smp", "cpus=2,sockets=2"},
+			"both cpus and sockets are set to config's CpuCount",
+		},
+		{
+			&Config{
+				CpuCount: 2,
+			},
+			map[string]interface{}{
+				"floppy_path": "/path/to/floppy",
+			},
+			&stepRun{ui: packer.TestUi(t)},
+			[]string{"-fda", "/path/to/floppy"},
+			"floppy path should be set under fda flag, when it exists",
+		},
+		{
+			&Config{
+				Headless:          false,
+				Display:           "fakedisplay",
+				UseDefaultDisplay: false,
+			},
+			map[string]interface{}{},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{"-display", "fakedisplay"},
+			"Display option should value config display",
+		},
+		{
+			&Config{
+				Headless: false,
+			},
+			map[string]interface{}{},
+			&stepRun{
+				atLeastVersion2: true,
+				ui:              packer.TestUi(t),
+			},
+			[]string{"-display", "gtk"},
+			"Display option should default to gtk",
+		},
+	}
+
+	for _, tc := range testcases {
+		state := runTestState(t, &Config{})
+		for k, v := range tc.ExtraState {
+			state.Put(k, v)
+		}
+
+		args, err := tc.Step.getCommandArgs(tc.Config, state)
+		if err != nil {
+			t.Fatalf("should not have an error getting args. Error: %s", err)
+		}
+		if !matchArgument(args, tc.Expected) {
+			t.Fatalf("Couldn't find %#v in result. Got: %#v, Reason: %s",
+				tc.Expected, args, tc.Reason)
+		}
+	}
+}
+
+// This test makes sure that arguments don't end up in the final boot command
+// if they aren't configured in the config.
+// func TestDefaultsAbsentValues(t *testing.T) {}
+func matchArgument(actual []string, expected []string) bool {
+	key := expected[0]
+	for i, k := range actual {
+		if key == k {
+			if expected[1] == actual[i+1] {
+				return true
+			}
+		}
+	}
+	return false
 }