[Libvir] PATCH: Don't leave a 'braindead' Xen VM when device hotplug fails

[Daniel P. Berrange]

Because of the way Xen guest creation works, the virDomainCreateLinux
implementation for Xen is really a three stage process - first we create
the raw domain, then we have to wait for device hotplug to complete,
finally unpausing the domain.

Currently if the device hotplug fails we error out, leaving a 'braindead'
VM lying around in the paused state. This is rather confusing for users
leading to people thinking everything was OK, and thus unpausing the
guest manually, then filing bug reports when they find they've no devices
in the guest VM !

The xm tool by comparison will tear down the VM if the device hotplug fails
so the user doesn't see the paused domain after the failure. The attached
patch re-arranges the Xen driver for creating new domains to do the same
kind of thing - upon any failure we explicitly destroy the halfdead domain

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
-------------- next part --------------
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.91
diff -u -p -r1.91 xend_internal.c
--- src/xend_internal.c	16 Feb 2007 16:04:54 -0000	1.91
+++ src/xend_internal.c	21 Feb 2007 16:17:08 -0000
@@ -2816,7 +2816,7 @@ xenDaemonCreateLinux(virConnectPtr conn,
     int ret;
     char *sexpr;
     char *name = NULL;
-    virDomainPtr dom;
+    virDomainPtr dom = NULL;
 
     if (!VIR_IS_CONNECT(conn)) {
         virXendError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
@@ -2841,32 +2841,30 @@ xenDaemonCreateLinux(virConnectPtr conn,
     ret = xenDaemonDomainCreateLinux(conn, sexpr);
     free(sexpr);
     if (ret != 0) {
-        fprintf(stderr, _("Failed to create domain %s\n"), name);
         goto error;
     }
 
-    ret = xend_wait_for_devices(conn, name);
-    if (ret != 0) {
-        fprintf(stderr, _("Failed to get devices for domain %s\n"), name);
+    /* This comes before wait_for_devices, to ensure that later
+       cleanup will destroy the domain upon failure */
+    if (!(dom = virDomainLookupByName(conn, name)))
         goto error;
-    }
 
-    dom = virDomainLookupByName(conn, name);
-    if (dom == NULL) {
+    if ((ret = xend_wait_for_devices(conn, name)) < 0)
         goto error;
-    }
 
-    ret = xenDaemonDomainResume(dom);
-    if (ret != 0) {
-        fprintf(stderr, _("Failed to resume new domain %s\n"), name);
-        xenDaemonDomainDestroy(dom);
+    if ((ret = xenDaemonDomainResume(dom)) < 0)
         goto error;
-    }
 
     free(name);
 
     return (dom);
+
   error:
+    /* Make sure we don't leave a still-born domain around */
+    if (dom != NULL) {
+        xenDaemonDomainDestroy(dom);
+        virFreeDomain(dom->conn, dom);
+    }
     if (name != NULL)
         free(name);
     return (NULL);
Index: src/xm_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xm_internal.c,v
retrieving revision 1.12
diff -u -p -r1.12 xm_internal.c
--- src/xm_internal.c	16 Feb 2007 16:04:54 -0000	1.12
+++ src/xm_internal.c	21 Feb 2007 16:17:11 -0000
@@ -1295,13 +1297,6 @@ int xenXMDomainCreate(virDomainPtr domai
     ret = xenDaemonDomainCreateLinux(domain->conn, sexpr);
     free(sexpr);
     if (ret != 0) {
-        fprintf(stderr, "Failed to create domain %s\n", domain->name);
-        return (-1);
-    }
-
-    ret = xend_wait_for_devices(domain->conn, domain->name);
-    if (ret != 0) {
-        fprintf(stderr, "Failed to get devices for domain %s\n", domain->name);
         return (-1);
     }
 
@@ -1310,15 +1305,20 @@ int xenXMDomainCreate(virDomainPtr domai
     }
     domain->id = ret;
 
-    ret = xenDaemonDomainResume(domain);
-    if (ret != 0) {
-        fprintf(stderr, "Failed to resume new domain %s\n", domain->name);
+    if ((ret = xend_wait_for_devices(domain->conn, domain->name)) < 0)
+        goto cleanup;
+
+    if ((ret = xenDaemonDomainResume(domain)) < 0)
+        goto cleanup;
+
+    return (0);
+
+ cleanup:
+    if (domain->id != -1) {
         xenDaemonDomainDestroy(domain);
         domain->id = -1;
-        return (-1);
     }
-
-    return (0);
+    return (-1);
 }