[PATCH v3 01/13] vbox: Cleanup partially-defined VM on failure

[Dawid Zamirski <dzamirski@xxxxxxxxx>]

Since the VBOX API requires to register an initial VM before proceeding
to attach any remaining devices to it, any failure to attach such
devices should result in automatic cleanup of the initially registered
VM so that the state of VBOX registry remains clean without any leftover
"aborted" VMs in it. Failure to cleanup of such partial VMs results in a
warning log so that actual define error stays on the top of the error
stack.
---
 src/vbox/vbox_common.c | 52 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index aa2563cf1..d93b0855f 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -1821,6 +1821,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     virDomainPtr ret = NULL;
     unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+    bool machineReady = false;
+
 
     virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
 
@@ -1830,12 +1832,11 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags
     if (!data->vboxObj)
         return ret;
 
-    VBOX_IID_INITIALIZE(&mchiid);
     if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt,
-                                        NULL, parse_flags))) {
-        goto cleanup;
-    }
+                                        NULL, parse_flags)))
+        return ret;
 
+    VBOX_IID_INITIALIZE(&mchiid);
     virUUIDFormat(def->uuid, uuidstr);
 
     rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);
@@ -1928,30 +1929,49 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags
     vboxAttachUSB(def, data, machine);
     vboxAttachSharedFolder(def, data, machine);
 
-    /* Save the machine settings made till now and close the
-     * session. also free up the mchiid variable used.
+    machineReady = true;
+
+ cleanup:
+    /* if machine wasn't even created, cleanup is trivial */
+    if (!machine) {
+        vboxIIDUnalloc(&mchiid);
+        virDomainDefFree(def);
+
+        return ret;
+    }
+
+    /* Save the machine settings made till now, even when jumped here on error,
+     * as otherwise unregister won't cleanup properly. For example, it won't
+     * close media that were partially attached. The VBOX SDK docs say that
+     * unregister implicitly calls saveSettings but evidently it's not so...
      */
     rc = gVBoxAPI.UIMachine.SaveSettings(machine);
     if (NS_FAILED(rc)) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("failed no saving settings, rc=%08x"), (unsigned)rc);
-        goto cleanup;
+                       _("Failed to save VM settings, rc=%08x"), rc);
+        machineReady = false;
     }
 
     gVBoxAPI.UISession.Close(data->vboxSession);
-    vboxIIDUnalloc(&mchiid);
-
-    ret = virGetDomain(conn, def->name, def->uuid, -1);
-    VBOX_RELEASE(machine);
 
-    virDomainDefFree(def);
+    if (machineReady) {
+        ret = virGetDomain(conn, def->name, def->uuid, -1);
+    } else {
+        /* Unregister incompletely configured VM to not leave garbage behind */
+        rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
 
-    return ret;
+        if (NS_SUCCEEDED(rc))
+            gVBoxAPI.deleteConfig(machine);
+        else
+            VIR_WARN("Could not cleanup partially created VM after failure, "
+                     "rc=%08x", rc);
+    }
 
- cleanup:
     VBOX_RELEASE(machine);
+    vboxIIDUnalloc(&mchiid);
     virDomainDefFree(def);
-    return NULL;
+
+    return ret;
 }
 
 static virDomainPtr
-- 
2.14.3

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list