[vbox] more refactoring

Keep <Boot> for now, booting from the HDD should be forced in run-virt.
Also note that virtualbox only allows controller named Floppy, SATA,
IDE, SCSI etc. multiple entries with the same name are ignored (the
first one takes precedence). Controller named differently are just
ignored. Upon shutdown, virtualbox cleans it up from the xml.

So assuming the uploader actually started and shut the machine down, we
can be safe to assume that controller nodes are unique. Nevertheless we
try to at least log these cases, to debug those corner cases should they
occur. Thanks for reading that far :P

2 files changed, 107 insertions, 67 deletions

diff --git a/src/main/java/org/openslx/util/vm/VboxConfig.java b/src/main/java/org/openslx/util/vm/VboxConfig.java
index 30bcc7f..1e3ba10 100644
--- a/src/main/java/org/openslx/util/vm/VboxConfig.java
+++ b/src/main/java/org/openslx/util/vm/VboxConfig.java
@@ -44,7 +44,7 @@ public class VboxConfig
 	private ArrayList<HardDisk> hddsArray = new ArrayList<HardDisk>();
 
 	// XPath and DOM parsing related members
-	private final XPath xPath = XPathFactory.newInstance().newXPath();
+	private static final XPath xPath = XPathFactory.newInstance().newXPath();
 	private Document doc = null;
 	private static DocumentBuilder dBuilder;
 	static {
@@ -65,7 +65,6 @@ public class VboxConfig
 			"/VirtualBox/Machine/Hardware/HID",
 			"/VirtualBox/Machine/Hardware/USB",
 			"/VirtualBox/Machine/Hardware/LPT",
-			"/VirtualBox/Machine/Hardware/Boot",
 			"/VirtualBox/Machine/Hardware/SharedFolders",
 			"/VirtualBox/Machine/Hardware/Network/Adapter[@enabled='true']/*",
 			"/VirtualBox/Machine/ExtraData",
@@ -97,7 +96,8 @@ public class VboxConfig
 	 * 
 	 * @param file the input XML file
 	 * @throws IOException if an error occurs while reading the file
-	 * @throws UnsupportedVirtualizerFormatException if the given file is not a valid VirtualBox configuration file. 
+	 * @throws UnsupportedVirtualizerFormatException if the given file is not a valid VirtualBox
+	 *            configuration file.
 	 */
 	public VboxConfig( File file ) throws IOException, UnsupportedVirtualizerFormatException
 	{
@@ -125,11 +125,12 @@ public class VboxConfig
 	}
 
 	/**
-	 * Creates an vbox configuration by constructing a DOM from the XML content given as a byte array.
+	 * Creates an vbox configuration by constructing a DOM from the XML content given as a byte
+	 * array.
 	 * 
 	 * @param machineDescription content of the XML file saved as a byte array.
 	 * @param length of the machine description byte array.
-	 * @throws IOException if an 
+	 * @throws IOException if an
 	 */
 	public VboxConfig( byte[] machineDescription, int length )
 	{
@@ -137,7 +138,7 @@ public class VboxConfig
 			InputSource is = new InputSource( new StringReader( new String( machineDescription ) ) );
 			doc = dBuilder.parse( is );
 		} catch ( SAXException | IOException e ) {
-			LOGGER.error( "Failed to create a DOM from " + machineDescription + "\nTrace: ", e );
+			LOGGER.error( "Failed to create a DOM from give machine description: ", e );
 		}
 	}
 
@@ -293,6 +294,11 @@ public class VboxConfig
 		}
 	}
 
+	/**
+	 * Getter for the parsed display name
+	 *
+	 * @return the display name of this VM
+	 */
 	public String getDisplayName()
 	{
 		return displayName;
@@ -439,24 +445,23 @@ public class VboxConfig
 
 	/**
 	 * Function used to change the value of an attribute of given element.
-	 * 
-	 * NOTE: If more than one element is found, this function will update them all!
-	 * TODO: Do we want this?
-	 * 
-	 * @param element
-	 * @param attribute
-	 * @param value
+	 * The given xpath to the element needs to find a single node, or this function will return
+	 * false. If only one element was found, it will return the result of calling addAttributeToNode.
+	 * Note that due to the way setAttribute() works, this function to create the attribute if it
+	 * doesn't exists.
+	 *
+	 * @param elementXPath given as an xpath expression
+	 * @param attribute attribute to change
+	 * @param value to set the attribute to
 	 */
-	public void changeAttribute( String element, String attribute, String value )
+	public boolean changeAttribute( String elementXPath, String attribute, String value )
 	{
-		NodeList nodes = findNodes( element );
-		LOGGER.debug( "Found: " + nodes.getLength() + " for : " + element );
-		for ( int i = 0; i < nodes.getLength(); i++ ) {
-			Element current = (Element)nodes.item( i );
-			if ( current == null )
-				return;
-			current.setAttribute( attribute, value );
+		NodeList nodes = findNodes( elementXPath );
+		if ( nodes == null || nodes.getLength() != 1 ) {
+			LOGGER.error( "No unique node could be found for: " + elementXPath );
+			return false;
 		}
+		return addAttributeToNode( nodes.item( 0 ), attribute, value );
 	}
 
 	/**
@@ -470,8 +475,8 @@ public class VboxConfig
 	 */
 	public boolean addAttributeToNode( Node node, String attribute, String value )
 	{
-		if ( node == null ) {
-			LOGGER.warn( "Node is null; stopped!" );
+		if ( node == null || node.getNodeType() != Node.ELEMENT_NODE ) {
+			LOGGER.error( "Trying to change attribute of a non element node!" );
 			return false;
 		}
 		try {
@@ -490,27 +495,34 @@ public class VboxConfig
 	 * @param nameOfnewNode name of the node to be added
 	 * @return the newly added Node
 	 */
-	public Node addNewNode( String parentXPath, String nameOfnewNode )
+	public Node addNewNode( String parentXPath, String childName )
 	{
 		NodeList possibleParents = findNodes( parentXPath );
-		if ( possibleParents == null || possibleParents.getLength() == 0 ) {
-			LOGGER.error( "Could not find any parent node to add new node to: " + parentXPath );
+		if ( possibleParents == null || possibleParents.getLength() != 1 ) {
+			LOGGER.error( "Could not find unique parent node to add new node to: " + parentXPath );
 			return null;
 		}
-		LOGGER.debug( "Possible parents: " + possibleParents.getLength() );
-		// warn if we have more than two, which we should never have
-		if ( possibleParents.getLength() > 2 ) {
-			LOGGER.error( "SKIPPING: Multiple nodes found named: '" + parentXPath );
+		return addNewNode( possibleParents.item( 0 ), childName );
+	}
+
+	/**
+	 * Creates a new element to the given parent node.
+	 *
+	 * @param parent to add the new element to
+	 * @param childName name of the new element to create
+	 * @return the newly created node
+	 */
+	public Node addNewNode( Node parent, String childName )
+	{
+		if ( parent == null || parent.getNodeType() != Node.ELEMENT_NODE ) {
 			return null;
 		}
-		Node parent = possibleParents.item( 0 );
-		Element newNode;
+		Node newNode = null;
 		try {
-			newNode = doc.createElement( nameOfnewNode );
+			newNode = doc.createElement( childName );
 			parent.appendChild( newNode );
 		} catch ( DOMException e ) {
-			LOGGER.error( "Could not append '" + nameOfnewNode + "' to '" + parent.getNodeName() + "': ", e );
-			return null;
+			LOGGER.error( "Failed to add '" + childName + "' to '" + parent.getNodeName() + "'." );
 		}
 		return newNode;
 	}
diff --git a/src/main/java/org/openslx/util/vm/VboxMetaData.java b/src/main/java/org/openslx/util/vm/VboxMetaData.java
index f7378be..c3f0a43 100644
--- a/src/main/java/org/openslx/util/vm/VboxMetaData.java
+++ b/src/main/java/org/openslx/util/vm/VboxMetaData.java
@@ -101,8 +101,6 @@ public class VboxMetaData extends VmMetaData<VBoxSoundCardMeta, VBoxDDAccelMeta,
 		registerVirtualHW();
 
 		this.config.init();
-		LOGGER.debug( "DUMPING CONFIG: " );
-		LOGGER.debug( this.config.toString( true ) );
 		displayName = config.getDisplayName();
 		setOs( config.getOsName() );
 		this.isMachineSnapshot = config.isMachineSnapshot();
@@ -197,16 +195,17 @@ public class VboxMetaData extends VmMetaData<VBoxSoundCardMeta, VBoxDDAccelMeta,
 			newMachineUuid = UUID.randomUUID();
 		}
 		String machineUUid = "{" + newMachineUuid.toString() + "}";
-		config.changeAttribute( "/VirtualBox/Machine", "uuid", machineUUid );
-		return true;
+		return config.changeAttribute( "/VirtualBox/Machine", "uuid", machineUUid );
 	}
 
 	@Override
 	public boolean addDefaultNat()
 	{
-		config.addNewNode( "/VirtualBox/Machine/Hardware/Network/Adapter", "NAT" );
-		config.changeAttribute( "/VirtualBox/Machine/Hardware/Network/Adapter", "MACAddress", "080027B86D12" );
-		return true;
+		if ( config.addNewNode( "/VirtualBox/Machine/Hardware/Network/Adapter", "NAT" ) == null ) {
+			LOGGER.error( "Failed to set network adapter to NAT." );
+			return false;
+		}
+		return config.changeAttribute( "/VirtualBox/Machine/Hardware/Network/Adapter", "MACAddress", "080027B86D12" );
 	}
 
 	@Override
@@ -219,58 +218,87 @@ public class VboxMetaData extends VmMetaData<VBoxSoundCardMeta, VBoxDDAccelMeta,
 	@Override
 	public boolean addDisplayName( String name )
 	{
-		config.changeAttribute( "/VirtualBox/Machine", "name", name );
-		return true;
+		return config.changeAttribute( "/VirtualBox/Machine", "name", name );
 	}
 
 	@Override
 	public boolean addRam( int mem )
 	{
-		config.changeAttribute( "/VirtualBox/Machine/Hardware/Memory", "RAMSize", Integer.toString( mem ) );
-		return true;
+		return config.changeAttribute( "/VirtualBox/Machine/Hardware/Memory", "RAMSize", Integer.toString( mem ) );
 	}
 
 	@Override
 	public void addFloppy( int index, String image, boolean readOnly )
 	{
+		Element floppyController = null;
 		NodeList matches = (NodeList)config.findNodes( "/VirtualBox/Machine/StorageControllers/StorageController[@name='Floppy']" );
 		if ( matches == null || matches.getLength() == 0 ) {
-			Element controller = (Element)config.addNewNode( "/VirtualBox/Machine/StorageControllers", "StorageController" );
-			controller.setAttribute( "name", "Floppy" );
-			controller.setAttribute( "type", "I82078" );
-			controller.setAttribute( "PortCount", "1" );
-			controller.setAttribute( "useHostIOCache", "true" );
-			controller.setAttribute( "Bootable", "true" );
+			floppyController = (Element)config.addNewNode( "/VirtualBox/Machine/StorageControllers", "StorageController" );
+			if ( floppyController == null ) {
+				LOGGER.error( "Failed to add <Image> to floppy device." );
+				return;
+			}
+			floppyController.setAttribute( "name", "Floppy" );
+			floppyController.setAttribute( "type", "I82078" );
+			floppyController.setAttribute( "PortCount", "1" );
+			floppyController.setAttribute( "useHostIOCache", "true" );
+			floppyController.setAttribute( "Bootable", "false" );
 		}
+		// virtualbox only allows one controller per type
+		if ( matches.getLength() > 1 ) {
+			LOGGER.error( "Multiple floppy controllers detected, this should never happen! " );
+			return;
+		}
+		// so if we had any matches, we know we have exactly one
+		if ( floppyController == null )
+			floppyController = (Element)matches.item( 0 );
+
+		// add the floppy device
+		Element floppyDevice = (Element)config.addNewNode( floppyController, "AttachedDevice" );
+		if ( floppyDevice == null ) {
+			LOGGER.error( "Failed to add <Image> to floppy device." );
+			return;
+		}
+		floppyDevice.setAttribute( "type", "Floppy" );
+		floppyDevice.setAttribute( "hotpluggable", "false" );
+		floppyDevice.setAttribute( "port", "0" );
+		floppyDevice.setAttribute( "device", Integer.toString( index ) );
 
-		Element attachedDev = (Element)config.addNewNode( "/VirtualBox/Machine/StorageControllers/StorageController[@name='Floppy']", "AttachedDevice" );
-		attachedDev.setAttribute( "type", "Floppy" );
-		attachedDev.setAttribute( "hotpluggable", "false" );
-		attachedDev.setAttribute( "port", "0" );
-		attachedDev.setAttribute( "device", Integer.toString( index ) );
-
-		// now add the image to it, if one was given
+		// finally add the image to it, if one was given
 		if ( image != null ) {
-			Element imageTag = (Element)config.addNewNode( "/VirtualBox/Machine/StorageControllers/StorageController[@name='Floppy']", "Image" );
-			imageTag.setAttribute( "uuid", VboxConfig.PlaceHolder.FLOPPYUUID.toString() );
-			config.addNewNode( "/VirtualBox/Machine/MediaRegistry", "FloppyImages" );
-			Element floppyImageTag = (Element)config.addNewNode( "/VirtualBox/Machine/MediaRegistry/FloppyImages", "Image" );
-			floppyImageTag.setAttribute( "uuid", VboxConfig.PlaceHolder.FLOPPYUUID.toString() );
-			floppyImageTag.setAttribute( "location", VboxConfig.PlaceHolder.FLOPPYLOCATION.toString() );
+			Element floppyImage = (Element)config.addNewNode( floppyDevice, "Image" );
+			if ( floppyImage == null ) {
+				LOGGER.error( "Failed to add <Image> to floppy device." );
+				return;
+			}
+			floppyImage.setAttribute( "uuid", VboxConfig.PlaceHolder.FLOPPYUUID.toString() );
+			// register the image in the media registry
+			Element floppyImages = (Element)config.addNewNode( "/VirtualBox/Machine/MediaRegistry", "FloppyImages" );
+			if ( floppyImages == null ) {
+				LOGGER.error( "Failed to add <FloppyImages> to media registry." );
+				return;
+			}
+			Element floppyImageReg = (Element)config.addNewNode( "/VirtualBox/Machine/MediaRegistry/FloppyImages", "Image" );
+			if ( floppyImageReg == null ) {
+				LOGGER.error( "Failed to add <Image> to floppy images in the media registry." );
+				return;
+			}
+			floppyImageReg.setAttribute( "uuid", VboxConfig.PlaceHolder.FLOPPYUUID.toString() );
+			floppyImageReg.setAttribute( "location", VboxConfig.PlaceHolder.FLOPPYLOCATION.toString() );
 		}
 	}
 
 	@Override
 	public boolean addCdrom( String image )
 	{
+		// TODO - done in run-virt currently
 		return false;
 	}
 
 	@Override
 	public boolean addCpuCoreCount( int nrOfCores )
 	{
-		config.changeAttribute( "/VirtualBox/Machine/Hardware/CPU", "count", Integer.toString( nrOfCores ) );
-		return true;
+		return config.changeAttribute( "/VirtualBox/Machine/Hardware/CPU", "count", Integer.toString( nrOfCores ) );
 	}
 
 	@Override