issue 384: NetworkSpec refactored - Adrian comments addressed

labs/virtualbox/src/main/java/org/jclouds/virtualbox/domain/CloneSpec.java

@@ -29,8 +29,8 @@ import static com.google.common.base.Preconditions.checkNotNull;
  */
 public class CloneSpec {
 
-   private VmSpec vmSpec;
+   private final VmSpec vmSpec;
-   private NetworkSpec networkSpec;
+   private final NetworkSpec networkSpec;
 
    public static Builder builder() {
       return new Builder();

labs/virtualbox/src/main/java/org/jclouds/virtualbox/domain/NetworkAdapter.java

@@ -19,16 +19,16 @@
 
 package org.jclouds.virtualbox.domain;
 
-import org.virtualbox_4_1.NetworkAttachmentType;
+import static com.google.common.base.Preconditions.checkNotNull;
-
-import com.google.common.base.Objects;
-import com.google.common.collect.Sets;
-import org.virtualbox_4_1.NATProtocol;
 
 import java.util.Collections;
 import java.util.Set;
 
-import static com.google.common.base.Preconditions.checkNotNull;
+import org.virtualbox_4_1.NATProtocol;
+import org.virtualbox_4_1.NetworkAttachmentType;
+
+import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableSet;
 
 /**
  * Represents a network adapter in VirtualBox.
@@ -39,14 +39,14 @@ public class NetworkAdapter {
 
 	private final NetworkAttachmentType networkAttachmentType;
 	private final String macAddress;
-	private final Set<RedirectRule> redirectRules = Sets.newLinkedHashSet();
+	private final Set<RedirectRule> redirectRules;
 
 	public NetworkAdapter(NetworkAttachmentType networkAttachmentType,
 			String macAddress, Set<RedirectRule> redirectRules) {
 		this.networkAttachmentType = checkNotNull(networkAttachmentType,
 				"networkAttachmentType");
 		this.macAddress = macAddress;
-		this.redirectRules.addAll(redirectRules);
+		this.redirectRules = ImmutableSet.<RedirectRule>copyOf(checkNotNull(redirectRules, "redirectRules"));
 	}
 
 	public static Builder builder() {
@@ -57,7 +57,7 @@ public class NetworkAdapter {
 
 		private NetworkAttachmentType networkAttachmentType;
 		private String macAddress;
-		private Set<RedirectRule> redirectRules = Sets.newLinkedHashSet();
+		private Set<RedirectRule> redirectRules;
 
 		/**
 		 * 

labs/virtualbox/src/main/java/org/jclouds/virtualbox/domain/NetworkInterfaceCard.java

@@ -18,6 +18,8 @@
  */
 package org.jclouds.virtualbox.domain;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.base.Objects;
 
 public class NetworkInterfaceCard {
@@ -28,9 +30,9 @@ public class NetworkInterfaceCard {
 
 	
 	public NetworkInterfaceCard(long slot, NetworkAdapter networkAdapter, String hostInterfaceName) {
-		this.slot = slot;
+		this.slot = checkNotNull(slot, "slot");
-		this.networkAdapter = networkAdapter;
+		this.networkAdapter = checkNotNull(networkAdapter, "networkAdapter");
-		this.hostInterfaceName = hostInterfaceName;
+		this.hostInterfaceName = checkNotNull(hostInterfaceName, "hostInterfaceName");
 	}
 
 	public static Builder builder() {

labs/virtualbox/src/main/java/org/jclouds/virtualbox/domain/NetworkSpec.java

@@ -18,12 +18,14 @@
  */
 package org.jclouds.virtualbox.domain;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import java.util.ArrayList;
 import java.util.List;
 
 import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableList;
 
 /**
  * Describes the network configuration for a VirtualBox machine.
@@ -33,7 +35,7 @@ public class NetworkSpec {
    private final List<NetworkInterfaceCard> networkInterfaceCards;
 
    public NetworkSpec(final List<NetworkInterfaceCard> networkInterfaceCards) {
-      this.networkInterfaceCards = checkNotNull(networkInterfaceCards, "networkInterfaceCards");
+      this.networkInterfaceCards = ImmutableList.copyOf(checkNotNull(networkInterfaceCards, "networkInterfaceCards"));
    }
 
    public static Builder builder() {
@@ -44,43 +46,26 @@ public class NetworkSpec {
 
       private List<NetworkInterfaceCard> networkInterfaceCards = new ArrayList<NetworkInterfaceCard>();
 
-      public Builder addNIC1(NetworkInterfaceCard networkInterfaceCard) {
+      public Builder addNIC(long slot, NetworkInterfaceCard networkInterfaceCard) {
-    	  NetworkInterfaceCard nic = NetworkInterfaceCard.builder().slot(0L).addNetworkAdapter(networkInterfaceCard.getNetworkAdapter()).build();
+         checkArgument(slot >= 0 && slot < 4, "must be 0, 1, 2, 3: %s", slot);
+    	  NetworkInterfaceCard nic = NetworkInterfaceCard.builder().slot(slot).addNetworkAdapter(networkInterfaceCard.getNetworkAdapter()).build();
          this.networkInterfaceCards.add(nic);
          return this;
       }
-      
-      public Builder addNIC2(NetworkInterfaceCard networkInterfaceCard) {
-    	  NetworkInterfaceCard nic = NetworkInterfaceCard.builder().slot(1L).addNetworkAdapter(networkInterfaceCard.getNetworkAdapter()).build();
-         this.networkInterfaceCards.add(nic);
-         return this;
-      }
-      
-      public Builder addNIC3(NetworkInterfaceCard networkInterfaceCard) {
-    	  NetworkInterfaceCard nic = NetworkInterfaceCard.builder().slot(2L).addNetworkAdapter(networkInterfaceCard.getNetworkAdapter()).build();
-         this.networkInterfaceCards.add(nic);
-         return this;
-      }
-      
-      public Builder addNIC4(NetworkInterfaceCard networkInterfaceCard) {
-    	  NetworkInterfaceCard nic = NetworkInterfaceCard.builder().slot(3L).addNetworkAdapter(networkInterfaceCard.getNetworkAdapter()).build();
-         this.networkInterfaceCards.add(nic);
-         return this;
-      }      
 
       public NetworkSpec build() {
          return new NetworkSpec(networkInterfaceCards);
       }
    }
 
-
    public List<NetworkInterfaceCard> getNetworkInterfaceCards() {
       return networkInterfaceCards;
    }
 
    @Override
    public boolean equals(Object o) {
-      if (this == o) return true;
+      if (this == o)
+         return true;
       if (o instanceof VmSpec) {
          NetworkSpec other = (NetworkSpec) o;
          return Objects.equal(networkInterfaceCards, other.networkInterfaceCards);
@@ -95,8 +80,6 @@ public class NetworkSpec {
 
    @Override
    public String toString() {
-      return "NetworkSpec{" +
+      return "NetworkSpec{" + "networkInterfaceCards= " + networkInterfaceCards + '}';
-              "networkInterfaceCards= " + networkInterfaceCards +
-              '}';
    }
 }

labs/virtualbox/src/main/java/org/jclouds/virtualbox/functions/AttachNicToMachine.java

@@ -18,6 +18,8 @@
  */
 package org.jclouds.virtualbox.functions;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import javax.annotation.Nullable;
 
 import org.jclouds.virtualbox.domain.NetworkInterfaceCard;
@@ -36,8 +38,8 @@ public class AttachNicToMachine implements Function<NetworkInterfaceCard, Void>
 
 
 	public AttachNicToMachine(String vmName, MachineUtils machineUtils) {
-		this.vmName = vmName;
+		this.vmName = checkNotNull(vmName, "vmName");
-		this.machineUtils = machineUtils;
+		this.machineUtils = checkNotNull(machineUtils, "machineUtils");
 	}
 
 	@Override

labs/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndInstallVm.java

@@ -139,8 +139,13 @@ public class CreateAndInstallVm implements Function<MasterSpec, IMachine> {
       }
    }
 
+   /**
+    * ensureMachineHasPowerDown needs to have this delay just to ensure that the machine is completely powered off
+    * 
+    * @param vmName
+    */
    private void ensureMachineHasPowerDown(String vmName) {
-	   while(manager.get().getVBox().findMachine(vmName).getState().equals(MachineState.RUNNING)) {
+	   while(!manager.get().getVBox().findMachine(vmName).getState().equals(MachineState.POWERED_OFF)) {
 		   machineUtils.lockSessionOnMachineAndApply(vmName, LockType.Shared, new Function<ISession, Void>() {
          @Override
          public Void apply(ISession session) {