[Xen-devel] [PATCH 2/2] libxl: treat "dying" domains as destroyed

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Rename the DOMAIN_DESTROY event to DOMAIN_DEATH and have it trigger
when the domain goes into the state indicated by the domaininfo flag
"dying".

This fixes a race which could leak a daemonised xl process, which
would have ignored the domain becoming "dying" and would then wait
forever to be told the domain was destroyed.

After the domain becomes "dying" we can't generate an event when it is
actually destroyed because xenstored will eat the relevant
VIRT_DOM_EXC virq and not generate an @releaseDomain, since xenstored
discards its own record of the domain's existence as soon as it sees
the domain "dying" and will not trigger @releaseDomain watches for
domains it knows nothing about.  Arguably this is a bug in xenstored,
and the whole @releaseDomain machinery is rather poor, but let us not
fix that now.

Anyway, xl does not really want to know when the domain is ultimately
destroyed.  It is enough for xl to know that it is on the way out, in
the "dying" state (which leads later to destruction by Xen).

Also fix a bug where domain_death_xswatch_callback might read one
domain beyond the valid data in its domaininfos array, by correctly
ordering the checks for empty domain list, end of domain list, and our
domain being missing.

Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
 tools/libxl/libxl.c         |   57 ++++++++++++++++++++++++++++--------------
 tools/libxl/libxl_types.idl |    4 +-
 tools/libxl/xl_cmdimpl.c    |    4 +-
 3 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2758d4c..b74a4d9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -685,6 +685,29 @@ int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
     return ret;
 }
 
+static void domain_death_occurred(libxl__egc *egc,
+                                  libxl_evgen_domain_death **evg_upd,
+                                  const char *why) {
+    /* Removes **evg from the list and advances *evg_upd to the next
+     * entry.  Call sites in _xswatch_callback must use "continue". */
+    EGC_GC;
+    libxl_evgen_domain_death *const evg = *evg_upd;
+
+    LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, " death %s", why);
+
+    libxl_evgen_domain_death *evg_next = LIBXL_TAILQ_NEXT(evg, entry);
+    *evg_upd = evg_next;
+
+    libxl_event *ev = NEW_EVENT(egc, DOMAIN_DEATH, evg->domid);
+    if (!ev) return;
+
+    libxl__event_occurred(egc, ev);
+
+    evg->death_reported = 1;
+    LIBXL_TAILQ_REMOVE(&CTX->death_list, evg, entry);
+    LIBXL_TAILQ_INSERT_HEAD(&CTX->death_reported, evg, entry);
+}
+
 static void domain_death_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch 
*w,
                                         const char *wpath, const char *epath) {
     EGC_GC;
@@ -728,32 +751,23 @@ static void domain_death_xswatch_callback(libxl__egc 
*egc, libxl__ev_xswatch *w,
                        evg, evg->domid, (int)(got - domaininfos),
                        got < gotend ? (long)got->domain : -1L);
 
-            if (!rc || got->domain > evg->domid) {
-                /* ie, the list doesn't contain evg->domid any more so
-                 * the domain has been destroyed */
-                libxl_evgen_domain_death *evg_next;
-
-                LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, " destroyed");
-
-                libxl_event *ev = NEW_EVENT(egc, DOMAIN_DESTROY, evg->domid);
-                if (!ev) goto out;
-
-                libxl__event_occurred(egc, ev);
-
-                evg->death_reported = 1;
-                evg_next = LIBXL_TAILQ_NEXT(evg, entry);
-                LIBXL_TAILQ_REMOVE(&CTX->death_list, evg, entry);
-                LIBXL_TAILQ_INSERT_HEAD(&CTX->death_reported, evg, entry);
-                evg = evg_next;
-
+            if (!rc) {
+                domain_death_occurred(egc, &evg, "empty list");
                 continue;
             }
-            
+
             if (got == gotend) {
                 LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, " got==gotend");
                 break;
             }
 
+            if (got->domain > evg->domid) {
+                /* ie, the list doesn't contain evg->domid any more so
+                 * the domain has been destroyed */
+                domain_death_occurred(egc, &evg, "missing from list");
+                continue;
+            }
+
             if (got->domain < evg->domid) {
                 got++;
                 continue;
@@ -764,6 +778,11 @@ static void domain_death_xswatch_callback(libxl__egc *egc, 
libxl__ev_xswatch *w,
                        " dominf.flags=%x",
                        evg->shutdown_reported, got->flags);
 
+            if (got->flags & XEN_DOMINF_dying) {
+                domain_death_occurred(egc, &evg, "dying");
+                continue;
+            }
+
             if (!evg->shutdown_reported &&
                 (got->flags & XEN_DOMINF_shutdown)) {
                 libxl_event *ev = NEW_EVENT(egc, DOMAIN_SHUTDOWN, got->domain);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index f438c9f..5492ce9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -396,7 +396,7 @@ libxl_sched_sedf = Struct("sched_sedf", [
 
 libxl_event_type = Enumeration("event_type", [
     (1, "DOMAIN_SHUTDOWN"),
-    (2, "DOMAIN_DESTROY"),
+    (2, "DOMAIN_DEATH"),
     (3, "DISK_EJECT"),
     (4, "OPERATION_COMPLETE"),
     ])
@@ -417,7 +417,7 @@ libxl_event = Struct("event",[
           [("domain_shutdown", Struct(None, [
                                              ("shutdown_reason", uint8),
                                       ])),
-           ("domain_destroy", Struct(None, [])),
+           ("domain_death", Struct(None, [])),
            ("disk_eject", Struct(None, [
                                         ("vdev", string),
                                         ("disk", libxl_device_disk),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8832b5a..d326588 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1909,7 +1909,7 @@ start:
                 abort();
             }
 
-        case LIBXL_EVENT_TYPE_DOMAIN_DESTROY:
+        case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
             LOG("Domain %d has been destroyed.", domid);
             ret = 0;
             goto out;
@@ -2445,7 +2445,7 @@ static void shutdown_domain(const char *p, int wait)
 
             switch (event->type) {
 
-            case LIBXL_EVENT_TYPE_DOMAIN_DESTROY:
+            case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
                 LOG("Domain %d has been destroyed", domid);
                 goto done;
 
-- 
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel