[PATCH 0 of 9] xl: handle domain shutdown/reboot/crash in a user configurable way

[PATCH 0 of 9] xl: handle domain shutdown/reboot/crash in a user configurable way

[Ian Campbell]

Add support for on_poweroff, on_reboot and on_crash configuration
items. In addition to these options from xend also add on_watchdog.

[PATCH 1 of 9] libxl: Add LIBXL_EVENT namespace to enum libxl_event_type

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140562 -3600
# Node ID 7a0c37f2a9b51ac012a32bcf6ab222580e7cac94
# Parent  74c8b1f2c2da28fe00492add40a32a257e44dca7
libxl: Add LIBXL_EVENT namespace to enum libxl_event_type

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 74c8b1f2c2da -r 7a0c37f2a9b5 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -643,7 +643,7 @@ int libxl_wait_for_domain_death(struct l
 int libxl_wait_for_domain_death(struct libxl_ctx *ctx, uint32_t domid, libxl_waiter *waiter)
 {
     waiter->path = strdup("@releaseDomain");
-    if (asprintf(&(waiter->token), "%d", DOMAIN_DEATH) < 0)
+    if (asprintf(&(waiter->token), "%d", LIBXL_EVENT_DOMAIN_DEATH) < 0)
         return -1;
     if (!xs_watch(ctx->xsh, waiter->path, waiter->token))
         return -1;
@@ -663,7 +663,7 @@ int libxl_wait_for_disk_ejects(struct li
                      libxl_xs_get_dompath(ctx, domid),
                      device_disk_dev_number(disks[i].virtpath)) < 0)
             return -1;
-        if (asprintf(&(waiter[i].token), "%d", DISK_EJECT) < 0)
+        if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
             return -1;
         xs_watch(ctx->xsh, waiter->path, waiter->token);
     }
@@ -711,7 +711,7 @@ int libxl_event_get_domain_death_info(st
 {
     int rc = 0, ret;
 
-    if (event && event->type == DOMAIN_DEATH) {
+    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
         ret = xc_domain_getinfolist(ctx->xch, domid, 1, info);
         if (ret == 1 && info->domain == domid) {
                 if (info->flags & XEN_DOMINF_running ||
@@ -730,7 +730,7 @@ out:
 
 int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk)
 {
-    if (event && event->type == DISK_EJECT) {
+    if (event && event->type == LIBXL_EVENT_DISK_EJECT) {
         char *path;
         char *backend;
         char *value = libxl_xs_read(ctx, XBT_NULL, event->path);
diff -r 74c8b1f2c2da -r 7a0c37f2a9b5 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
@@ -357,8 +357,8 @@ char *libxl_uuid2string(struct libxl_ctx
 /* events handling */
 
 typedef enum {
-    DOMAIN_DEATH,
-    DISK_EJECT,
+    LIBXL_EVENT_DOMAIN_DEATH,
+    LIBXL_EVENT_DISK_EJECT,
 } libxl_event_type;
 
 typedef struct {
diff -r 74c8b1f2c2da -r 7a0c37f2a9b5 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -1348,7 +1348,7 @@ start:
             continue;
         libxl_get_event(&ctx, &event);
         switch (event.type) {
-            case DOMAIN_DEATH:
+            case LIBXL_EVENT_DOMAIN_DEATH:
                 if (libxl_event_get_domain_death_info(&ctx, domid, &event, &info)) {
                     LOG("Domain %d is dead", domid);
                     if (info.flags & XEN_DOMINF_dying || (info.flags & XEN_DOMINF_shutdown && (((info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) != SHUTDOWN_suspend))) {
@@ -1374,7 +1374,7 @@ start:
                     exit(0);
                 }
                 break;
-            case DISK_EJECT:
+            case LIBXL_EVENT_DISK_EJECT:
                 if (libxl_event_get_disk_eject_info(&ctx, domid, &event, &disk))
                     libxl_cdrom_insert(&ctx, domid, &disk);
                 break;

[PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140562 -3600
# Node ID f6300d42a667cf6a1a02fc065ecd9eaea0e10ecc
# Parent  7a0c37f2a9b51ac012a32bcf6ab222580e7cac94
libxl: return libxl_dominfo from libxl_event_get_domain_death_info

Removes a libxc data type from the libxl interface.


Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -405,8 +405,6 @@ static void xcinfo2xlinfo(const xc_domai
 static void xcinfo2xlinfo(const xc_domaininfo_t *xcinfo,
                           struct libxl_dominfo *xlinfo)
 {
-    unsigned int shutdown_reason;
-
     memcpy(&(xlinfo->uuid), xcinfo->handle, sizeof(xen_domain_handle_t));
     xlinfo->domid = xcinfo->domain;
 
@@ -416,12 +414,28 @@ static void xcinfo2xlinfo(const xc_domai
     xlinfo->blocked  = !!(xcinfo->flags&XEN_DOMINF_blocked);
     xlinfo->running  = !!(xcinfo->flags&XEN_DOMINF_running);
     xlinfo->crashed  = 0;
+    xlinfo->shutdown_reason  = -1;
 
-    shutdown_reason = (xcinfo->flags>>XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask;
-
-    if ( xlinfo->shutdown && (shutdown_reason == SHUTDOWN_crash) ) {
-        xlinfo->shutdown = 0;
-        xlinfo->crashed  = 1;
+    if (xlinfo->shutdown) {
+        switch ((xcinfo->flags>>XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) {
+        case SHUTDOWN_poweroff:
+            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_POWEROFF;
+            break;
+        case SHUTDOWN_reboot:
+            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_REBOOT;
+            break;
+        case SHUTDOWN_suspend:
+            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_SUSPEND;
+            break;
+        case SHUTDOWN_crash:
+            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_CRASH;
+            xlinfo->shutdown = 0;
+            xlinfo->crashed  = 1;
+            break;
+        case SHUTDOWN_watchdog:
+            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_WATCHDOG;
+            break;
+        }
     }
 
     xlinfo->max_memkb = PAGE_TO_MEMKB(xcinfo->tot_pages);
@@ -707,20 +721,19 @@ int libxl_free_waiter(libxl_waiter *wait
     return 0;
 }
 
-int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, xc_domaininfo_t *info)
+int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
 {
     int rc = 0, ret;
+    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
+        ret = libxl_domain_info(ctx, info, domid);
 
-    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
-        ret = xc_domain_getinfolist(ctx->xch, domid, 1, info);
-        if (ret == 1 && info->domain == domid) {
-                if (info->flags & XEN_DOMINF_running ||
-                    (!(info->flags & XEN_DOMINF_shutdown) && !(info->flags & XEN_DOMINF_dying)))
+        if (ret == 0 && info->domid == domid) {
+            if (info->running || (!info->shutdown && !info->dying && !info->crashed))
                     goto out;
                 rc = 1;
                 goto out;
         }
-        memset(info, 0, sizeof(xc_dominfo_t));
+        memset(info, 0, sizeof(*info));
         rc = 1;
         goto out;
     }
diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
@@ -22,6 +22,14 @@
 #include <xs.h>
 #include <sys/wait.h> /* for pid_t */
 
+enum libxl_shutdown_reason {
+    LIBXL_SHUTDOWN_POWEROFF,  /* Domain exited normally. Clean up and kill. */
+    LIBXL_SHUTDOWN_REBOOT,    /* Clean up, kill, and then restart.          */
+    LIBXL_SHUTDOWN_SUSPEND,   /* Clean up, save suspend info, kill.         */
+    LIBXL_SHUTDOWN_CRASH,     /* Tell controller we've crashed.             */
+    LIBXL_SHUTDOWN_WATCHDOG,  /* Restart because watchdog time expired.     */
+};
+
 struct libxl_dominfo {
     uint8_t uuid[16];
     uint32_t domid;
@@ -31,6 +39,8 @@ struct libxl_dominfo {
     uint8_t shutdown:1;
     uint8_t crashed:1;
     uint8_t dying:1;
+    enum libxl_shutdown_reason shutdown_reason;
+
     uint64_t max_memkb;
     uint64_t cpu_time;
     uint32_t vcpu_max_id;
@@ -385,7 +395,7 @@ int libxl_free_event(libxl_event *event)
 int libxl_free_event(libxl_event *event);
 int libxl_free_waiter(libxl_waiter *waiter);
 
-int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, xc_domaininfo_t *info);
+int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info);
 int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk);
 
 int libxl_domain_rename(struct libxl_ctx *ctx, uint32_t domid,
diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -1335,7 +1335,7 @@ start:
     while (1) {
         int ret;
         fd_set rfds;
-        xc_domaininfo_t info;
+        struct libxl_dominfo info;
         libxl_event event;
         libxl_device_disk disk;
         memset(&info, 0x00, sizeof(xc_domaininfo_t));
@@ -1351,11 +1351,10 @@ start:
             case LIBXL_EVENT_DOMAIN_DEATH:
                 if (libxl_event_get_domain_death_info(&ctx, domid, &event, &info)) {
                     LOG("Domain %d is dead", domid);
-                    if (info.flags & XEN_DOMINF_dying || (info.flags & XEN_DOMINF_shutdown && (((info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) != SHUTDOWN_suspend))) {
+                    if (info.crashed || info.dying || (info.shutdown && info.shutdown_reason != SHUTDOWN_suspend)) {
                         LOG("Domain %d needs to be clean: destroying the domain", domid);
                         libxl_domain_destroy(&ctx, domid, 0);
-                        if (info.flags & XEN_DOMINF_shutdown &&
-                            (((info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) == SHUTDOWN_reboot)) {
+                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
                             libxl_free_waiter(w1);
                             libxl_free_waiter(w2);
                             free(w1);

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1280140562 -3600
> # Node ID f6300d42a667cf6a1a02fc065ecd9eaea0e10ecc
> # Parent  7a0c37f2a9b51ac012a32bcf6ab222580e7cac94
> libxl: return libxl_dominfo from libxl_event_get_domain_death_info
> 
> Removes a libxc data type from the libxl interface.
> 
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
> @@ -405,8 +405,6 @@ static void xcinfo2xlinfo(const xc_domai
>  static void xcinfo2xlinfo(const xc_domaininfo_t *xcinfo,
>                            struct libxl_dominfo *xlinfo)
>  {
> -    unsigned int shutdown_reason;
> -
>      memcpy(&(xlinfo->uuid), xcinfo->handle, sizeof(xen_domain_handle_t));
>      xlinfo->domid = xcinfo->domain;
>  
> @@ -416,12 +414,28 @@ static void xcinfo2xlinfo(const xc_domai
>      xlinfo->blocked  = !!(xcinfo->flags&XEN_DOMINF_blocked);
>      xlinfo->running  = !!(xcinfo->flags&XEN_DOMINF_running);
>      xlinfo->crashed  = 0;
> +    xlinfo->shutdown_reason  = -1;
>  
> -    shutdown_reason = (xcinfo->flags>>XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask;
> -
> -    if ( xlinfo->shutdown && (shutdown_reason == SHUTDOWN_crash) ) {
> -        xlinfo->shutdown = 0;
> -        xlinfo->crashed  = 1;
> +    if (xlinfo->shutdown) {
> +        switch ((xcinfo->flags>>XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) {
> +        case SHUTDOWN_poweroff:
> +            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_POWEROFF;
> +            break;
> +        case SHUTDOWN_reboot:
> +            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_REBOOT;
> +            break;
> +        case SHUTDOWN_suspend:
> +            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_SUSPEND;
> +            break;
> +        case SHUTDOWN_crash:
> +            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_CRASH;
> +            xlinfo->shutdown = 0;
> +            xlinfo->crashed  = 1;

do we still need xlinfo->crashed, now that we have
xlinfo->shutdown_reason?

> +            break;
> +        case SHUTDOWN_watchdog:
> +            xlinfo->shutdown_reason = LIBXL_SHUTDOWN_WATCHDOG;
> +            break;
> +        }
>      }
>  
>      xlinfo->max_memkb = PAGE_TO_MEMKB(xcinfo->tot_pages);
> @@ -707,20 +721,19 @@ int libxl_free_waiter(libxl_waiter *wait
>      return 0;
>  }
>  
> -int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, xc_domaininfo_t *info)
> +int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
>  {
>      int rc = 0, ret;
> +    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
> +        ret = libxl_domain_info(ctx, info, domid);
>  
> -    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
> -        ret = xc_domain_getinfolist(ctx->xch, domid, 1, info);
> -        if (ret == 1 && info->domain == domid) {
> -                if (info->flags & XEN_DOMINF_running ||
> -                    (!(info->flags & XEN_DOMINF_shutdown) && !(info->flags & XEN_DOMINF_dying)))
> +        if (ret == 0 && info->domid == domid) {
> +            if (info->running || (!info->shutdown && !info->dying && !info->crashed))
>                      goto out;
>                  rc = 1;
>                  goto out;
>          }
> -        memset(info, 0, sizeof(xc_dominfo_t));
> +        memset(info, 0, sizeof(*info));
>          rc = 1;
>          goto out;
>      }
> diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/libxl.h	Mon Jul 26 11:36:02 2010 +0100
> @@ -22,6 +22,14 @@
>  #include <xs.h>
>  #include <sys/wait.h> /* for pid_t */
>  
> +enum libxl_shutdown_reason {
> +    LIBXL_SHUTDOWN_POWEROFF,  /* Domain exited normally. Clean up and kill. */
> +    LIBXL_SHUTDOWN_REBOOT,    /* Clean up, kill, and then restart.          */
> +    LIBXL_SHUTDOWN_SUSPEND,   /* Clean up, save suspend info, kill.         */
> +    LIBXL_SHUTDOWN_CRASH,     /* Tell controller we've crashed.             */
> +    LIBXL_SHUTDOWN_WATCHDOG,  /* Restart because watchdog time expired.     */
> +};
> +
>  struct libxl_dominfo {
>      uint8_t uuid[16];
>      uint32_t domid;
> @@ -31,6 +39,8 @@ struct libxl_dominfo {
>      uint8_t shutdown:1;
>      uint8_t crashed:1;
>      uint8_t dying:1;
> +    enum libxl_shutdown_reason shutdown_reason;
> +
>      uint64_t max_memkb;
>      uint64_t cpu_time;
>      uint32_t vcpu_max_id;
> @@ -385,7 +395,7 @@ int libxl_free_event(libxl_event *event)
>  int libxl_free_event(libxl_event *event);
>  int libxl_free_waiter(libxl_waiter *waiter);
>  
> -int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, xc_domaininfo_t *info);
> +int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info);
>  int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk);
>  
>  int libxl_domain_rename(struct libxl_ctx *ctx, uint32_t domid,
> diff -r 7a0c37f2a9b5 -r f6300d42a667 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
> @@ -1335,7 +1335,7 @@ start:
>      while (1) {
>          int ret;
>          fd_set rfds;
> -        xc_domaininfo_t info;
> +        struct libxl_dominfo info;
>          libxl_event event;
>          libxl_device_disk disk;
>          memset(&info, 0x00, sizeof(xc_domaininfo_t));
> @@ -1351,11 +1351,10 @@ start:
>              case LIBXL_EVENT_DOMAIN_DEATH:
>                  if (libxl_event_get_domain_death_info(&ctx, domid, &event, &info)) {
>                      LOG("Domain %d is dead", domid);
> -                    if (info.flags & XEN_DOMINF_dying || (info.flags & XEN_DOMINF_shutdown && (((info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) != SHUTDOWN_suspend))) {
> +                    if (info.crashed || info.dying || (info.shutdown && info.shutdown_reason != SHUTDOWN_suspend)) {

shouldn't this be LIBXL_SHUTDOWN_SUSPEND

>                          LOG("Domain %d needs to be clean: destroying the domain", domid);
>                          libxl_domain_destroy(&ctx, domid, 0);
> -                        if (info.flags & XEN_DOMINF_shutdown &&
> -                            (((info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) == SHUTDOWN_reboot)) {
> +                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {

shouldn't this be LIBXL_SHUTDOWN_REBOOT?

It seems that both SHUDOWN_ are removed in following patches anyway, in
that case it is Ok I guess.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Campbell]

On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> On Mon, 26 Jul 2010, Ian Campbell wrote:
> do we still need xlinfo->crashed, now that we have
> xlinfo->shutdown_reason?

I guess we could get rid of it, the only other place it is used (AFAIK)
is in the "xl list" output I sent the other day.

> shouldn't this be LIBXL_SHUTDOWN_SUSPEND
[...]
> shouldn't this be LIBXL_SHUTDOWN_REBOOT?

Yes in both cases.

> It seems that both SHUDOWN_ are removed in following patches anyway, in
> that case it is Ok I guess.

Phew!

I can regenerate the series if necessary though.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2010, Ian Campbell wrote:
> > do we still need xlinfo->crashed, now that we have
> > xlinfo->shutdown_reason?
> 
> I guess we could get rid of it, the only other place it is used (AFAIK)
> is in the "xl list" output I sent the other day.

All right let's do that.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> libxl: return libxl_dominfo from libxl_event_get_domain_death_info
> Removes a libxc data type from the libxl interface.

I don't think this is the right approach, mainly because shutdown
reasons aren't a libxc datatype, but a Xen one.  My view is that libxl
should hide libxc, but that it is allowed to expose Xen.

So libxl callers are allowed to #include xen/include/public/sched.h.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Campbell]

On Mon, 2010-07-26 at 16:26 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> > libxl: return libxl_dominfo from libxl_event_get_domain_death_info
> > Removes a libxc data type from the libxl interface.
> 
> I don't think this is the right approach, mainly because shutdown
> reasons aren't a libxc datatype, but a Xen one.  My view is that libxl
> should hide libxc, but that it is allowed to expose Xen.
> 
> So libxl callers are allowed to #include xen/include/public/sched.h.

I'm happy with exposing Xen datatypes if that is the approach we want to
take but not with returning an xc_domaininfo_t.

I think the right compromise is therefore to declare that
libxl_dominfo.shutdown_reason contains the values defined in
xen/.../sched.h rather than a new libxl enum and retain the switch to
returning libxl_dominfo.

(Maybe that's what you meant anyhow)

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> I'm happy with exposing Xen datatypes if that is the approach we want to
> take but not with returning an xc_domaininfo_t.

Yes.

> I think the right compromise is therefore to declare that
> libxl_dominfo.shutdown_reason contains the values defined in
> xen/.../sched.h rather than a new libxl enum and retain the switch to
> returning libxl_dominfo.

Yes, exactly.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> > libxl: return libxl_dominfo from libxl_event_get_domain_death_info
> > Removes a libxc data type from the libxl interface.
> 
> I don't think this is the right approach, mainly because shutdown
> reasons aren't a libxc datatype, but a Xen one.  My view is that libxl
> should hide libxc, but that it is allowed to expose Xen.
> 
> So libxl callers are allowed to #include xen/include/public/sched.h.
> 

if libxl should hide libxc then allowing clients to include a Xen header
is an even worse layering violation.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> if libxl should hide libxc then allowing clients to include a Xen header
> is an even worse layering violation.

I don't think so.  The purpose of the doctrine that libxl callers
should not need to call libxc is so that libxl is complete.  And
anything which libxc exports is contingent.

I certainly think that libxl callers should not make Xen hypercalls,
need to know SCHEDOP numbers, etc. etc.  But I think it is OK for
libxl to expose it its callers anything that forms part of the
abstract interface to Xen guests.  So that includes:
 * shutdown reasons
 * the difference between various kinds of HV and PV block devices
     (hda, xvda, sda, ...), including the facility to specify the
     actual xenstore interface combined PV block device number
 * the fact that a guest may have multiple consoles (PV and
     emulated serial) - even if the current toolstack implementation
     does not support all possible combinations

The concrete interface (event channels, rings, frame numbers, etc.)
should remain opaque but there is no point abstracting away and
translating the numerical values of a Xen public enum whose
semantics we _do_ want to expose.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> > if libxl should hide libxc then allowing clients to include a Xen header
> > is an even worse layering violation.
> 
> I don't think so.  The purpose of the doctrine that libxl callers
> should not need to call libxc is so that libxl is complete.  And
> anything which libxc exports is contingent.
> 
> I certainly think that libxl callers should not make Xen hypercalls,
> need to know SCHEDOP numbers, etc. etc.  But I think it is OK for
> libxl to expose it its callers anything that forms part of the
> abstract interface to Xen guests.  So that includes:
>  * shutdown reasons
>  * the difference between various kinds of HV and PV block devices
>      (hda, xvda, sda, ...), including the facility to specify the
>      actual xenstore interface combined PV block device number
>  * the fact that a guest may have multiple consoles (PV and
>      emulated serial) - even if the current toolstack implementation
>      does not support all possible combinations
> 
> The concrete interface (event channels, rings, frame numbers, etc.)
> should remain opaque but there is no point abstracting away and
> translating the numerical values of a Xen public enum whose
> semantics we _do_ want to expose.
> 

enum alone would OK, but most header files contains other stuff as well,
so including features.h would be fine but including platform.h would not.
In order to have a clear policy we should just say that including Xen
header files should be avoided.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> enum alone would OK, but most header files contains other stuff as well,
> so including features.h would be fine but including platform.h would not.
> In order to have a clear policy we should just say that including Xen
> header files should be avoided.

I don't think there is any difficulty with libxl callers #including
Xen public header files, no matter that those Xen header files have
lots of other stuff in that is not relevant to them.

It's not as if #including the Xen public headers exposes functions for
making hypercalls, or indeed any functions or objects at all.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Ian Campbell]

On Mon, 2010-07-26 at 17:38 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> > enum alone would OK, but most header files contains other stuff as well,
> > so including features.h would be fine but including platform.h would not.
> > In order to have a clear policy we should just say that including Xen
> > header files should be avoided.
> 
> I don't think there is any difficulty with libxl callers #including
> Xen public header files, no matter that those Xen header files have
> lots of other stuff in that is not relevant to them.
> 
> It's not as if #including the Xen public headers exposes functions for
> making hypercalls, or indeed any functions or objects at all.

In my repost of the series I will use the SHUTDOWN_* values from
sched.h. We can always update it if consensus goes the other way.

Ian.

Re: [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 2 of 9] libxl: return libxl_dominfo from libxl_event_get_domain_death_info"):
> > enum alone would OK, but most header files contains other stuff as well,
> > so including features.h would be fine but including platform.h would not.
> > In order to have a clear policy we should just say that including Xen
> > header files should be avoided.
> 
> I don't think there is any difficulty with libxl callers #including
> Xen public header files, no matter that those Xen header files have
> lots of other stuff in that is not relevant to them.
> 
> It's not as if #including the Xen public headers exposes functions for
> making hypercalls, or indeed any functions or objects at all.
> 
 
No, but they include the shared_info page struct, the start_info page
struct and hypercall numbers and arguments, that is as close
as you can get.

I still think that callers shouldn't be required to include xen header
files.

[PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140562 -3600
# Node ID 1e0b63948031587b958ea307410d19c7b2be9614
# Parent  f6300d42a667cf6a1a02fc065ecd9eaea0e10ecc
libxl: signal caller if domain already destroyed on domain death event

Currently libxl_event_get_domain_death_info returns 0 if the event was
not a domain death event and 1 if it was but does not infom the user
if someone else has already cleaned up the domain, which means the
caller must replicate some of the logic from within libxl.

Instead have the libxl_event_get_XXX_info functions required that the
event is of the right type (the caller must have recently switched on
event->type anyway).

This allows the return codes to be used in an event specific way and
we take advantage of this by returning an error from
libxl_event_get_domain_death_info if the domain is not dying.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -721,54 +721,54 @@ int libxl_free_waiter(libxl_waiter *wait
     return 0;
 }
 
+/*
+ * Returns:
+ *  - 0 if the domain is dead and there is no cleanup to be done. e.g because someone else has already done it.
+ *  - 1 if the domain is dead and there is cleanup to be done.
+ *
+ * Can return error if the domain exists and is still running
+ */
 int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
 {
-    int rc = 0, ret;
-    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
-        ret = libxl_domain_info(ctx, info, domid);
+    if (libxl_domain_info(ctx, info, domid) < 0)
+        return 0;
 
-        if (ret == 0 && info->domid == domid) {
-            if (info->running || (!info->shutdown && !info->dying && !info->crashed))
-                    goto out;
-                rc = 1;
-                goto out;
-        }
-        memset(info, 0, sizeof(*info));
-        rc = 1;
-        goto out;
-    }
-out:
-    return rc;
+    if (info->running || (!info->shutdown && !info->dying && !info->crashed))
+        return ERROR_INVAL;
+
+    return 1;
 }
 
+/*
+ * Returns true and fills *disk if the caller should eject the disk
+ */
 int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk)
 {
-    if (event && event->type == LIBXL_EVENT_DISK_EJECT) {
-        char *path;
-        char *backend;
-        char *value = libxl_xs_read(ctx, XBT_NULL, event->path);
+    char *path;
+    char *backend;
+    char *value;
 
-        if (!value || strcmp(value,  "eject"))
-            return 0;
+    value = libxl_xs_read(ctx, XBT_NULL, event->path);
 
-        path = strdup(event->path);
-        path[strlen(path) - 6] = '\0';
-        backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", path));
+    if (!value || strcmp(value,  "eject"))
+        return 0;
 
-        disk->backend_domid = 0;
-        disk->domid = domid;
-        disk->physpath = NULL;
-        disk->phystype = 0;
-        /* this value is returned to the user: do not free right away */
-        disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/dev", backend));
-        disk->unpluggable = 1;
-        disk->readwrite = 0;
-        disk->is_cdrom = 1;
+    path = strdup(event->path);
+    path[strlen(path) - 6] = '\0';
+    backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", path));
 
-        free(path);
-        return 1;
-    }
-    return 0;
+    disk->backend_domid = 0;
+    disk->domid = domid;
+    disk->physpath = NULL;
+    disk->phystype = 0;
+    /* this value is returned to the user: do not free right away */
+    disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/dev", backend));
+    disk->unpluggable = 1;
+    disk->readwrite = 0;
+    disk->is_cdrom = 1;
+
+    free(path);
+    return 1;
 }
 
 static int libxl_destroy_device_model(struct libxl_ctx *ctx, uint32_t domid)
diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
@@ -1338,7 +1338,6 @@ start:
         struct libxl_dominfo info;
         libxl_event event;
         libxl_device_disk disk;
-        memset(&info, 0x00, sizeof(xc_domaininfo_t));
 
         FD_ZERO(&rfds);
         FD_SET(fd, &rfds);
@@ -1349,12 +1348,17 @@ start:
         libxl_get_event(&ctx, &event);
         switch (event.type) {
             case LIBXL_EVENT_DOMAIN_DEATH:
-                if (libxl_event_get_domain_death_info(&ctx, domid, &event, &info)) {
-                    LOG("Domain %d is dead", domid);
-                    if (info.crashed || info.dying || (info.shutdown && info.shutdown_reason != SHUTDOWN_suspend)) {
+                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
+
+                if (ret < 0) continue;
+
+                LOG("Domain %d is dead", domid);
+
+                if (ret) {
+                    if (info.shutdown_reason != SHUTDOWN_suspend) {
                         LOG("Domain %d needs to be clean: destroying the domain", domid);
                         libxl_domain_destroy(&ctx, domid, 0);
-                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
+                        if (info.shutdown_reason == SHUTDOWN_reboot) {
                             libxl_free_waiter(w1);
                             libxl_free_waiter(w2);
                             free(w1);

Re: [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1280140562 -3600
> # Node ID 1e0b63948031587b958ea307410d19c7b2be9614
> # Parent  f6300d42a667cf6a1a02fc065ecd9eaea0e10ecc
> libxl: signal caller if domain already destroyed on domain death event
> 
> Currently libxl_event_get_domain_death_info returns 0 if the event was
> not a domain death event and 1 if it was but does not infom the user
> if someone else has already cleaned up the domain, which means the
> caller must replicate some of the logic from within libxl.
> 
> Instead have the libxl_event_get_XXX_info functions required that the
> event is of the right type (the caller must have recently switched on
> event->type anyway).
> 
> This allows the return codes to be used in an event specific way and
> we take advantage of this by returning an error from
> libxl_event_get_domain_death_info if the domain is not dying.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
> @@ -721,54 +721,54 @@ int libxl_free_waiter(libxl_waiter *wait
>      return 0;
>  }
>  
> +/*
> + * Returns:
> + *  - 0 if the domain is dead and there is no cleanup to be done. e.g because someone else has already done it.
> + *  - 1 if the domain is dead and there is cleanup to be done.
> + *
> + * Can return error if the domain exists and is still running
> + */
>  int libxl_event_get_domain_death_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
>  {
> -    int rc = 0, ret;
> -    if (event && event->type == LIBXL_EVENT_DOMAIN_DEATH) {
> -        ret = libxl_domain_info(ctx, info, domid);
> +    if (libxl_domain_info(ctx, info, domid) < 0)
> +        return 0;
>  
> -        if (ret == 0 && info->domid == domid) {
> -            if (info->running || (!info->shutdown && !info->dying && !info->crashed))
> -                    goto out;
> -                rc = 1;
> -                goto out;
> -        }
> -        memset(info, 0, sizeof(*info));
> -        rc = 1;
> -        goto out;
> -    }
> -out:
> -    return rc;
> +    if (info->running || (!info->shutdown && !info->dying && !info->crashed))
> +        return ERROR_INVAL;
> +
> +    return 1;
>  }
>  
> +/*
> + * Returns true and fills *disk if the caller should eject the disk
> + */
>  int libxl_event_get_disk_eject_info(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk)
>  {
> -    if (event && event->type == LIBXL_EVENT_DISK_EJECT) {
> -        char *path;
> -        char *backend;
> -        char *value = libxl_xs_read(ctx, XBT_NULL, event->path);
> +    char *path;
> +    char *backend;
> +    char *value;
>  
> -        if (!value || strcmp(value,  "eject"))
> -            return 0;
> +    value = libxl_xs_read(ctx, XBT_NULL, event->path);
>  
> -        path = strdup(event->path);
> -        path[strlen(path) - 6] = '\0';
> -        backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", path));
> +    if (!value || strcmp(value,  "eject"))
> +        return 0;
>  
> -        disk->backend_domid = 0;
> -        disk->domid = domid;
> -        disk->physpath = NULL;
> -        disk->phystype = 0;
> -        /* this value is returned to the user: do not free right away */
> -        disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/dev", backend));
> -        disk->unpluggable = 1;
> -        disk->readwrite = 0;
> -        disk->is_cdrom = 1;
> +    path = strdup(event->path);
> +    path[strlen(path) - 6] = '\0';
> +    backend = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", path));
>  
> -        free(path);
> -        return 1;
> -    }
> -    return 0;
> +    disk->backend_domid = 0;
> +    disk->domid = domid;
> +    disk->physpath = NULL;
> +    disk->phystype = 0;
> +    /* this value is returned to the user: do not free right away */
> +    disk->virtpath = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/dev", backend));
> +    disk->unpluggable = 1;
> +    disk->readwrite = 0;
> +    disk->is_cdrom = 1;
> +
> +    free(path);
> +    return 1;
>  }
>  
>  static int libxl_destroy_device_model(struct libxl_ctx *ctx, uint32_t domid)
> diff -r f6300d42a667 -r 1e0b63948031 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:02 2010 +0100
> @@ -1338,7 +1338,6 @@ start:
>          struct libxl_dominfo info;
>          libxl_event event;
>          libxl_device_disk disk;
> -        memset(&info, 0x00, sizeof(xc_domaininfo_t));
>  
>          FD_ZERO(&rfds);
>          FD_SET(fd, &rfds);
> @@ -1349,12 +1348,17 @@ start:
>          libxl_get_event(&ctx, &event);
>          switch (event.type) {
>              case LIBXL_EVENT_DOMAIN_DEATH:
> -                if (libxl_event_get_domain_death_info(&ctx, domid, &event, &info)) {
> -                    LOG("Domain %d is dead", domid);
> -                    if (info.crashed || info.dying || (info.shutdown && info.shutdown_reason != SHUTDOWN_suspend)) {
> +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
> +
> +                if (ret < 0) continue;
> +
> +                LOG("Domain %d is dead", domid);
> +
> +                if (ret) {
> +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
>                          LOG("Domain %d needs to be clean: destroying the domain", domid);
>                          libxl_domain_destroy(&ctx, domid, 0);
> -                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
> +                        if (info.shutdown_reason == SHUTDOWN_reboot) {

Isn't still possible to get here and have info.shutdown == 0 (and even
info.dying == 0, after reading the fourth patch)?
If so, the previous test is probably clearer.

The rest of the series looks fine.

Re: [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Ian Campbell]

On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
> > +
> > +                if (ret < 0) continue;
> [....]
> > +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
> >                          LOG("Domain %d needs to be clean: destroying the domain", domid);
> >                          libxl_domain_destroy(&ctx, domid, 0);
> > -                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
> > +                        if (info.shutdown_reason == SHUTDOWN_reboot) {
> 
> Isn't still possible to get here and have info.shutdown == 0 (and even
> info.dying == 0, after reading the fourth patch)?
> If so, the previous test is probably clearer.

Umm...

I think libxl_event_get_domain_death_info should have returned
ERROR_INVAL in that case and we'd have take the earlier if (ret < 0)
continue.

> The rest of the series looks fine.

Thanks.

Re: [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > > +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
> > > +
> > > +                if (ret < 0) continue;
> > [....]
> > > +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
> > >                          LOG("Domain %d needs to be clean: destroying the domain", domid);
> > >                          libxl_domain_destroy(&ctx, domid, 0);
> > > -                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
> > > +                        if (info.shutdown_reason == SHUTDOWN_reboot) {
> > 
> > Isn't still possible to get here and have info.shutdown == 0 (and even
> > info.dying == 0, after reading the fourth patch)?
> > If so, the previous test is probably clearer.
> 
> Umm...
> 
> I think libxl_event_get_domain_death_info should have returned
> ERROR_INVAL in that case and we'd have take the earlier if (ret < 0)
> continue.
 
I think we should at least write in a comment in libxl.h that
shutdown_reason is valid when (shutdown || dying) and that when
libxl_event_get_domain_death_info returns 1 shutdown_reason is always
set. 
Also I still like the old test more, just for clarity.

Re: [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Ian Campbell]

On Mon, 2010-07-26 at 15:58 +0100, Stefano Stabellini wrote:
> On Mon, 26 Jul 2010, Ian Campbell wrote:
> > On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > > > +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
> > > > +
> > > > +                if (ret < 0) continue;
> > > [....]
> > > > +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
> > > >                          LOG("Domain %d needs to be clean: destroying the domain", domid);
> > > >                          libxl_domain_destroy(&ctx, domid, 0);
> > > > -                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
> > > > +                        if (info.shutdown_reason == SHUTDOWN_reboot) {
> > > 
> > > Isn't still possible to get here and have info.shutdown == 0 (and even
> > > info.dying == 0, after reading the fourth patch)?
> > > If so, the previous test is probably clearer.
> > 
> > Umm...
> > 
> > I think libxl_event_get_domain_death_info should have returned
> > ERROR_INVAL in that case and we'd have take the earlier if (ret < 0)
> > continue.
>  
> I think we should at least write in a comment in libxl.h that
> shutdown_reason is valid when (shutdown || dying) and that when
> libxl_event_get_domain_death_info returns 1 shutdown_reason is always
> set. 

Will do.

> Also I still like the old test more, just for clarity.

It would need to be "(info.shutdown||info.crashed||info.dying) ||
info.shutdown_reason ....." which duplicates the logic in libxl, which I
wanted to get rid of (I think that was fully realised the next patch or
so)

Ian.

Re: [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Stefano Stabellini]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> On Mon, 2010-07-26 at 15:58 +0100, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2010, Ian Campbell wrote:
> > > On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > > > > +                ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
> > > > > +
> > > > > +                if (ret < 0) continue;
> > > > [....]
> > > > > +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
> > > > >                          LOG("Domain %d needs to be clean: destroying the domain", domid);
> > > > >                          libxl_domain_destroy(&ctx, domid, 0);
> > > > > -                        if (info.shutdown && info.shutdown_reason == SHUTDOWN_reboot) {
> > > > > +                        if (info.shutdown_reason == SHUTDOWN_reboot) {
> > > > 
> > > > Isn't still possible to get here and have info.shutdown == 0 (and even
> > > > info.dying == 0, after reading the fourth patch)?
> > > > If so, the previous test is probably clearer.
> > > 
> > > Umm...
> > > 
> > > I think libxl_event_get_domain_death_info should have returned
> > > ERROR_INVAL in that case and we'd have take the earlier if (ret < 0)
> > > continue.
> >  
> > I think we should at least write in a comment in libxl.h that
> > shutdown_reason is valid when (shutdown || dying) and that when
> > libxl_event_get_domain_death_info returns 1 shutdown_reason is always
> > set. 
> 
> Will do.
> 
> > Also I still like the old test more, just for clarity.
> 
> It would need to be "(info.shutdown||info.crashed||info.dying) ||
> info.shutdown_reason ....." which duplicates the logic in libxl, which I
> wanted to get rid of (I think that was fully realised the next patch or
> so)
> 

I only meant the second test:

if ((info.shutdown || info.dying) && info.shutdown_reason == SHUTDOWN_reboot)

or we can set shutdown_reason to LIBXL_SHUTDOWN_INVAL by default and
keep this test as it is.
I wouldn't want users to believe that shutdown_reason
is LIBXL_SHUTDOWN_POWEROFF just because is 0.

[PATCH 4 of 9] libxl: should consider shutdown_reason for dying as well as shutdown domains

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID 982bf44850029802b7395a03659bf440d191be2f
# Parent  1e0b63948031587b958ea307410d19c7b2be9614
libxl: should consider shutdown_reason for dying as well as shutdown domains.

A dying domain is one which has notified the hypervisor (with
SCHEDOP_shutdown) and provided a reason code but which hasn't actually
shutdown yet.

Therefore a domain which is dying will always have a valid
shutdown_reason and therefore we should obey it.

Failure to do so means there is a race between domain cleanup in the
hypervisor and the toolstack notification which can mean we may
destroy a domain which actually wanted to be rebooted.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 1e0b63948031 -r 982bf4485002 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 26 11:36:02 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -416,7 +416,7 @@ static void xcinfo2xlinfo(const xc_domai
     xlinfo->crashed  = 0;
     xlinfo->shutdown_reason  = -1;
 
-    if (xlinfo->shutdown) {
+    if (xlinfo->dying || xlinfo->shutdown) {
         switch ((xcinfo->flags>>XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask) {
         case SHUTDOWN_poweroff:
             xlinfo->shutdown_reason = LIBXL_SHUTDOWN_POWEROFF;
@@ -429,7 +429,7 @@ static void xcinfo2xlinfo(const xc_domai
             break;
         case SHUTDOWN_crash:
             xlinfo->shutdown_reason = LIBXL_SHUTDOWN_CRASH;
-            xlinfo->shutdown = 0;
+            xlinfo->shutdown = xlinfo->dying = 0;
             xlinfo->crashed  = 1;
             break;
         case SHUTDOWN_watchdog:

[PATCH 5 of 9] libxl: add libxl_domain_preserve

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID 85b50d34d7cc8a70c083c0acdc4c2e7ead675029
# Parent  982bf44850029802b7395a03659bf440d191be2f
libxl: add libxl_domain_preserve

This method is intended to preserve an existing domain (for debugging
purposes) in such a way that the domain can also be restarted.

There is likely to be more required to achieve this aim than the
function currently does.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 982bf4485002 -r 85b50d34d7cc tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -399,6 +399,61 @@ int libxl_domain_resume(struct libxl_ctx
                         domid);
         return ERROR_FAIL;
     }
+    return 0;
+}
+
+/*
+ * Preserves a domain but rewrites xenstore etc to make it unique so
+ * that the domain can be restarted.
+ *
+ * Does not modify info so that it may be reused.
+ */
+int libxl_domain_preserve(struct libxl_ctx *ctx, uint32_t domid,
+                          libxl_domain_create_info *info, const char *name_suffix, uint8_t new_uuid[16])
+{
+    struct xs_permissions roperm[2];
+    xs_transaction_t t;
+    char *preserved_name;
+    char *uuid_string;
+    char *vm_path;
+    char *dom_path;
+
+    int rc;
+
+    preserved_name = libxl_sprintf(ctx, "%s%s", info->name, name_suffix);
+    if (!preserved_name) return ERROR_NOMEM;
+
+    uuid_string = libxl_uuid2string(ctx, new_uuid);
+    if (!uuid_string) return ERROR_NOMEM;
+
+    dom_path = libxl_xs_get_dompath(ctx, domid);
+    if (!dom_path) return ERROR_FAIL;
+
+    vm_path = libxl_sprintf(ctx, "/vm/%s", uuid_string);
+    if (!vm_path) return ERROR_FAIL;
+
+    roperm[0].id = 0;
+    roperm[0].perms = XS_PERM_NONE;
+    roperm[1].id = domid;
+    roperm[1].perms = XS_PERM_READ;
+
+ retry_transaction:
+    t = xs_transaction_start(ctx->xsh);
+
+    xs_rm(ctx->xsh, t, vm_path);
+    xs_mkdir(ctx->xsh, t, vm_path);
+    xs_set_permissions(ctx->xsh, t, vm_path, roperm, ARRAY_SIZE(roperm));
+
+    xs_write(ctx->xsh, t, libxl_sprintf(ctx, "%s/vm", dom_path), vm_path, strlen(vm_path));
+    rc = libxl_domain_rename(ctx, domid, info->name, preserved_name, t);
+    if (rc) return rc;
+
+    xs_write(ctx->xsh, t, libxl_sprintf(ctx, "%s/uuid", vm_path), uuid_string, strlen(uuid_string));
+
+    if (!xs_transaction_end(ctx->xsh, t, 0))
+        if (errno == EAGAIN)
+            goto retry_transaction;
+
     return 0;
 }
 
diff -r 982bf4485002 -r 85b50d34d7cc tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/libxl.h	Mon Jul 26 11:36:03 2010 +0100
@@ -340,6 +340,7 @@ int libxl_domain_resume(struct libxl_ctx
 int libxl_domain_resume(struct libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_shutdown(struct libxl_ctx *ctx, uint32_t domid, int req);
 int libxl_domain_destroy(struct libxl_ctx *ctx, uint32_t domid, int force);
+int libxl_domain_preserve(struct libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, uint8_t new_uuid[16]);
 
 int libxl_file_reference_map(struct libxl_ctx *ctx, libxl_file_reference *f);
 int libxl_file_reference_unmap(struct libxl_ctx *ctx, libxl_file_reference *f);

Re: [PATCH 5 of 9] libxl: add libxl_domain_preserve

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 5 of 9] libxl: add libxl_domain_preserve"):
> This method is intended to preserve an existing domain (for debugging
> purposes) in such a way that the domain can also be restarted.

This sounds interesting, but I'm not quite sure what the semantics are
intended to be.  I guess it works on an existing running domain or a
paused one.  Does it stop the domain's execution ?

The current code seems mainly to change the name and the uuid in
xenstore - to a specified new uuid - which seems a strange thing to
do.

Ian.

Re: [PATCH 5 of 9] libxl: add libxl_domain_preserve

[Ian Campbell]

On Mon, 2010-07-26 at 16:30 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 5 of 9] libxl: add libxl_domain_preserve"):
> > This method is intended to preserve an existing domain (for debugging
> > purposes) in such a way that the domain can also be restarted.
> 
> This sounds interesting, but I'm not quite sure what the semantics are
> intended to be.  I guess it works on an existing running domain or a
> paused one.  Does it stop the domain's execution ?

It was intended to be used to implement the rename-restart action for
the on_reboot (and on_crash I guess) actions which I introduce in a
later patch.

I only added this option because xend has it -- I'd be just as happy to
only implement simple restart until someone who wants the behaviour
comes along. In the meantime preserve or coredump-restart are pretty
good substitutes for rename-restart. 

Anyhow the actual actual semantics of libxl_domain_preserve is that it
acts on a shutdown (but not destroyed) domain. The name and/or comments
could certainly better reflect this.

> The current code seems mainly to change the name and the uuid in
> xenstore - to a specified new uuid - which seems a strange thing to
> do.

It's (a subset of) what xend does ;-) (For reference its
XendDomainInfo._preserveForRestart from
tools/python/xen/xend/XendDomainInfo.py)

I suspect the reason is something to do with not nuking /vm/<uuid> for a
running domain when destroying a previously preserved instance of that
domain.

Ian.

[PATCH 6 of 9] xl: do not try and auto re-connect console on reboot

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID 333254f8e11bc7bfa6901fe5e5585abb25e31398
# Parent  85b50d34d7cc8a70c083c0acdc4c2e7ead675029
xl: do not try and auto re-connect console on reboot

It is not possible to run the console client if we are rebooting a
guest via the backgrounded xl process so we may as well turn off
console autoconnect after the first boot.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 85b50d34d7cc -r 333254f8e11b tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -1188,6 +1188,12 @@ start:
      */
     dom_info->console_autoconnect = 0;
 
+    /*
+     * Do not attempt to reconnect if we come round again due to a
+     * guest reboot -- the stdin/out will be disconnected by then.
+     */
+    dom_info->console_autoconnect = 0;
+
     ret = libxl_run_bootloader(&ctx, &b_info, num_disks > 0 ? &disks[0] : NULL, domid);
     if (ret) {
         fprintf(stderr, "failed to run bootloader: %d\n", ret);

[PATCH 7 of 9] xl: Add function to generate random uuid and use it

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID 8a13008e84ae463436fb304c50a2892324d0baaf
# Parent  333254f8e11bc7bfa6901fe5e5585abb25e31398
xl: Add function to generate random uuid and use it.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 333254f8e11b -r 8a13008e84ae tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -211,15 +211,19 @@ static void init_build_info(libxl_domain
     }
 }
 
+static void random_uuid(uint8_t *uuid)
+{
+    int i;
+    for (i = 0; i < 16; i++)
+        uuid[i] = rand();
+}
+
 static void init_dm_info(libxl_device_model_info *dm_info,
         libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
 {
-    int i;
     memset(dm_info, '\0', sizeof(*dm_info));
 
-    for (i = 0; i < 16; i++) {
-        dm_info->uuid[i] = rand();
-    }
+    random_uuid(&dm_info->uuid[0]);
 
     dm_info->dom_name = c_info->name;
     dm_info->device_model = "qemu-dm";
@@ -493,7 +497,7 @@ static void parse_config_data(const char
     XLU_ConfigList *vbds, *nics, *pcis, *cvfbs, *net2s;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 1;
-    int i, e;
+    int e;
 
     config= xlu_cfg_init(stderr, configfile_filename_report);
     if (!config) {
@@ -521,9 +525,7 @@ static void parse_config_data(const char
         c_info->name = strdup(buf);
     else
         c_info->name = "test";
-    for (i = 0; i < 16; i++) {
-        c_info->uuid[i] = rand();
-    }
+    random_uuid(&c_info->uuid[0]);
 
     if (!xlu_cfg_get_long(config, "oos", &l))
         c_info->oos = l;

[PATCH 8 of 9] xl: Factor out domain death handling into a separate function

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID ee265e700eede4111e429a3c55d2c78f31805028
# Parent  8a13008e84ae463436fb304c50a2892324d0baaf
xl: Factor out domain death handling into a separate function.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 8a13008e84ae -r ee265e700eed tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -988,6 +988,18 @@ int autoconnect_console(int hvm)
     _exit(1);
 }
 
+/* Returns 1 if domain should be restarted */
+static int handle_domain_death(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
+{
+    if (info->shutdown_reason != SHUTDOWN_suspend) {
+        LOG("Domain %d needs to be clean: destroying the domain", domid);
+        libxl_domain_destroy(ctx, domid, 0);
+        if (info->shutdown_reason == SHUTDOWN_reboot)
+            return 1;
+    }
+    return 0;
+}
+
 struct domain_create {
     int debug;
     int daemonize;
@@ -1363,25 +1375,20 @@ start:
                 LOG("Domain %d is dead", domid);
 
                 if (ret) {
-                    if (info.shutdown_reason != SHUTDOWN_suspend) {
-                        LOG("Domain %d needs to be clean: destroying the domain", domid);
-                        libxl_domain_destroy(&ctx, domid, 0);
-                        if (info.shutdown_reason == SHUTDOWN_reboot) {
-                            libxl_free_waiter(w1);
-                            libxl_free_waiter(w2);
-                            free(w1);
-                            free(w2);
-                            LOG("Done. Rebooting now");
-                            /*
-                             * XXX FIXME: If this sleep is not there then
-                             * domain re-creation fails sometimes.
-                             */
-                            sleep(2);
-                            goto start;
-                        }
-                        LOG("Done. Exiting now");
+                    if (handle_domain_death(&ctx, domid, &event, &info)) {
+                        libxl_free_waiter(w1);
+                        libxl_free_waiter(w2);
+                        free(w1);
+                        free(w2);
+                        /*
+                         * XXX FIXME: If this sleep is not there then domain
+                         * re-creation fails sometimes.
+                         */
+                        LOG("Done. Rebooting now");
+                        sleep(2);
+                        goto start;
                     }
-                    LOG("Domain %d does not need to be clean, exiting now", domid);
+                    LOG("Done. Exiting now");
                     exit(0);
                 }
                 break;

[PATCH 9 of 9] xl: support on_{poweroff, reboot, crash} domain configuration options

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1280140563 -3600
# Node ID 113b04a7e60718c8730368a045a49ddd56e64fac
# Parent  ee265e700eede4111e429a3c55d2c78f31805028
xl: support on_{poweroff,reboot,crash} domain configuration options.

Adds on_watchdog compared to xend.

I have further plans for struct domain_config so it isn't as pointless
as it first looks.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r ee265e700eed -r 113b04a7e607 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 26 11:36:03 2010 +0100
@@ -99,6 +99,38 @@ struct save_file_header {
     uint32_t optional_data_len; /* skip, or skip tail, if not understood */
 };
 
+
+enum action_on_shutdown {
+    ACTION_DESTROY,
+
+    ACTION_RESTART,
+    ACTION_RESTART_RENAME,
+
+    ACTION_PRESERVE,
+
+    ACTION_COREDUMP_DESTROY,
+    ACTION_COREDUMP_RESTART,
+};
+
+static char *action_on_shutdown_names[] = {
+    [ACTION_DESTROY] = "destroy",
+
+    [ACTION_RESTART] = "restart",
+    [ACTION_RESTART_RENAME] = "rename-restart",
+
+    [ACTION_PRESERVE] = "preserve",
+
+    [ACTION_COREDUMP_DESTROY] = "coredump-destroy",
+    [ACTION_COREDUMP_RESTART] = "coredump-restart",
+};
+
+struct domain_config {
+    enum action_on_shutdown on_poweroff;
+    enum action_on_shutdown on_reboot;
+    enum action_on_shutdown on_watchdog;
+    enum action_on_shutdown on_crash;
+};
+
 /* Optional data, in order:
  *   4 bytes uint32_t  config file size
  *   n bytes           config file in Unix text file format
@@ -472,11 +504,28 @@ static void printf_info(int domid,
        printf(")\n");
 }
 
+static int parse_action_on_shutdown(const char *buf, enum action_on_shutdown *a)
+{
+    int i;
+    const char *n;
+
+    for (i = 0; i < sizeof(action_on_shutdown_names) / sizeof(action_on_shutdown_names[0]); i++) {
+        n = action_on_shutdown_names[i];
+
+        if (strcmp(buf, n) == 0) {
+            *a = i;
+            return 1;
+        }
+    }
+    return 0;
+}
+
 static void parse_config_data(const char *configfile_filename_report,
                               const char *configfile_data,
                               int configfile_len,
                               libxl_domain_create_info *c_info,
                               libxl_domain_build_info *b_info,
+                              struct domain_config *d_config,
                               libxl_device_disk **disks,
                               int *num_disks,
                               libxl_device_nic **vifs,
@@ -551,6 +600,35 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "memory", &l)) {
         b_info->max_memkb = l * 1024;
         b_info->target_memkb = b_info->max_memkb;
+    }
+
+    if (xlu_cfg_get_string (config, "on_poweroff", &buf))
+        buf = "destroy";
+    if (!parse_action_on_shutdown(buf, &d_config->on_poweroff)) {
+        fprintf(stderr, "Unknown on_poweroff action \"%s\" specified\n", buf);
+        exit(1);
+    }
+
+    if (xlu_cfg_get_string (config, "on_reboot", &buf))
+        buf = "restart";
+    if (!parse_action_on_shutdown(buf, &d_config->on_reboot)) {
+        fprintf(stderr, "Unknown on_reboot action \"%s\" specified\n", buf);
+        exit(1);
+    }
+
+    if (xlu_cfg_get_string (config, "on_watchdog", &buf))
+        buf = "destroy";
+    if (!parse_action_on_shutdown(buf, &d_config->on_watchdog)) {
+        fprintf(stderr, "Unknown on_watchdog action \"%s\" specified\n", buf);
+        exit(1);
+    }
+
+
+    if (xlu_cfg_get_string (config, "on_crash", &buf))
+        buf = "destroy";
+    if (!parse_action_on_shutdown(buf, &d_config->on_crash)) {
+        fprintf(stderr, "Unknown on_crash action \"%s\" specified\n", buf);
+        exit(1);
     }
 
     /* libxl_get_required_shadow_memory() must be called after final values
@@ -988,16 +1066,112 @@ int autoconnect_console(int hvm)
     _exit(1);
 }
 
-/* Returns 1 if domain should be restarted */
-static int handle_domain_death(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event, struct libxl_dominfo *info)
-{
-    if (info->shutdown_reason != SHUTDOWN_suspend) {
-        LOG("Domain %d needs to be clean: destroying the domain", domid);
+/* Returns 1 if domain should be restarted, 2 if domain should be renamed then restarted  */
+static int handle_domain_death(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event,
+                               libxl_domain_create_info *c_info,
+                               struct domain_config *d_config, struct libxl_dominfo *info)
+{
+    int restart = 0;
+    enum action_on_shutdown action;
+
+    switch (info->shutdown_reason) {
+    case LIBXL_SHUTDOWN_POWEROFF:
+        action = d_config->on_poweroff;
+        break;
+    case LIBXL_SHUTDOWN_REBOOT:
+        action = d_config->on_reboot;
+        break;
+    case LIBXL_SHUTDOWN_SUSPEND:
+        return 0;
+    case LIBXL_SHUTDOWN_CRASH:
+        action = d_config->on_crash;
+        break;
+    case LIBXL_SHUTDOWN_WATCHDOG:
+        action = d_config->on_watchdog;
+        break;
+    }
+
+    LOG("Action for shutdown reason code %d is %s", info->shutdown_reason, action_on_shutdown_names[action]);
+
+    if (action == ACTION_COREDUMP_DESTROY || action == ACTION_COREDUMP_RESTART) {
+        char *corefile;
+        int rc;
+
+        if (asprintf(&corefile, "/var/xen/dump/%s", c_info->name) < 0) {
+            LOG("failed to construct core dump path");
+        } else {
+            LOG("dumping core to %s", corefile);
+            rc=libxl_domain_core_dump(ctx, domid, corefile);
+            if (rc) LOG("core dump failed (rc=%d).", rc);
+        }
+        /* No point crying over spilled milk, continue on failure. */
+
+        if (action == ACTION_COREDUMP_DESTROY)
+            action = ACTION_DESTROY;
+        else
+            action = ACTION_RESTART;
+    }
+
+    switch (action) {
+    case ACTION_PRESERVE:
+        break;
+
+    case ACTION_RESTART_RENAME:
+        restart = 2;
+        break;
+
+    case ACTION_RESTART:
+        restart = 1;
+        /* fall-through */
+    case ACTION_DESTROY:
+        LOG("Domain %d needs to be cleaned up: destroying the domain", domid);
         libxl_domain_destroy(ctx, domid, 0);
-        if (info->shutdown_reason == SHUTDOWN_reboot)
-            return 1;
-    }
-    return 0;
+        break;
+
+    case ACTION_COREDUMP_DESTROY:
+    case ACTION_COREDUMP_RESTART:
+        /* Already handled these above. */
+        abort();
+    }
+
+    return restart;
+}
+
+static int preserve_domain(struct libxl_ctx *ctx, uint32_t domid, libxl_event *event,
+                           libxl_domain_create_info *c_info,
+                           struct domain_config *d_config, struct libxl_dominfo *info)
+{
+    time_t now;
+    struct tm tm;
+    char stime[24];
+
+    uint8_t new_uuid[16];
+
+    int rc;
+
+    now = time(NULL);
+    if (now == ((time_t) -1)) {
+        LOG("Failed to get current time for domain rename");
+        return 0;
+    }
+
+    tzset();
+    if (gmtime_r(&now, &tm) == NULL) {
+        LOG("Failed to convert time to UTC");
+        return 0;
+    }
+
+    if (!strftime(&stime[0], sizeof(stime), "-%Y%m%dT%H%MZ", &tm)) {
+        LOG("Failed to format time as a string");
+        return 0;
+    }
+
+    random_uuid(&new_uuid[0]);
+
+    LOG("Preserving domain %d %s with suffix%s", domid, c_info->name, stime);
+    rc = libxl_domain_preserve(ctx, domid, c_info, stime, new_uuid);
+
+    return rc == 0 ? 1 : 0;
 }
 
 struct domain_create {
@@ -1016,6 +1190,8 @@ struct domain_create {
 
 static int create_domain(struct domain_create *dom_info)
 {
+    struct domain_config d_config;
+
     libxl_domain_create_info c_info;
     libxl_domain_build_info b_info;
     libxl_domain_build_state state;
@@ -1150,7 +1326,7 @@ static int create_domain(struct domain_c
     if (!dom_info->quiet)
         printf("Parsing config file %s\n", config_file);
 
-    parse_config_data(config_file, config_data, config_len, &c_info, &b_info, &disks, &num_disks, &vifs, &num_vifs, &vif2s, &num_vif2s, &pcidevs, &num_pcidevs, &vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
+    parse_config_data(config_file, config_data, config_len, &c_info, &b_info, &d_config, &disks, &num_disks, &vifs, &num_vifs, &vif2s, &num_vif2s, &pcidevs, &num_pcidevs, &vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
 
     if (dom_info->dryrun)
         return 0;
@@ -1369,13 +1545,20 @@ start:
         switch (event.type) {
             case LIBXL_EVENT_DOMAIN_DEATH:
                 ret = libxl_event_get_domain_death_info(&ctx, domid, &event, &info);
-
                 if (ret < 0) continue;
 
                 LOG("Domain %d is dead", domid);
 
                 if (ret) {
-                    if (handle_domain_death(&ctx, domid, &event, &info)) {
+                    switch (handle_domain_death(&ctx, domid, &event, &c_info, &d_config, &info)) {
+                    case 2:
+                        if (!preserve_domain(&ctx, domid, &event, &c_info, &d_config, &info))
+                            /* If we fail then exit leaving the old domain in place. */
+                            exit(-1);
+
+                        /* Otherwise fall through and restart. */
+                    case 1:
+
                         libxl_free_waiter(w1);
                         libxl_free_waiter(w2);
                         free(w1);
@@ -1387,9 +1570,10 @@ start:
                         LOG("Done. Rebooting now");
                         sleep(2);
                         goto start;
+                    case 0:
+                        LOG("Done. Exiting now");
+                        exit(0);
                     }
-                    LOG("Done. Exiting now");
-                    exit(0);
                 }
                 break;
             case LIBXL_EVENT_DISK_EJECT:
@@ -1898,6 +2082,8 @@ void list_domains_details(void)
 void list_domains_details(void)
 {
     struct libxl_dominfo *info;
+    struct domain_config d_config;
+
     char *config_file;
     uint8_t *data;
     int nb_domain, i, len, rc;
@@ -1923,7 +2109,7 @@ void list_domains_details(void)
         if (rc)
             continue;
         CHK_ERRNO(asprintf(&config_file, "<domid %d data>", info[i].domid));
-        parse_config_data(config_file, (char *)data, len, &c_info, &b_info, &disks, &num_disks, &vifs, &num_vifs, &vif2s, &num_vif2s, &pcidevs, &num_pcidevs, &vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
+        parse_config_data(config_file, (char *)data, len, &c_info, &b_info, &d_config, &disks, &num_disks, &vifs, &num_vifs, &vif2s, &num_vif2s, &pcidevs, &num_pcidevs, &vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
         printf_info(info[i].domid, &c_info, &b_info, disks, num_disks, vifs, num_vifs, pcidevs, num_pcidevs, vfbs, num_vfbs, vkbs, num_vkbs, &dm_info);
         free(data);
         free(config_file);

[PATCH 0 of 9] xl: handle domain shutdown/reboot/crash in a user configurable way

[Ian Campbell]

Add support for on_poweroff, on_reboot and on_crash configuration
items. In addition to these options from xend also add on_watchdog.

Since previous posting I have:
* clarified the semantics of libxl_dominfo WRT when shutdown_reason is
  valid and is not valid
* dropped the crashed bit field from libxl_dominfo
* clarified the when libxl_event_get_domain_death_info returns a valid
  libxl_dominfo.
* moved libxl_domain_preserve + the restart-rename handling in xl to
  the end of the series so it can be ommitted if desired