[PATCH 0 of 3] libxl: domain shutdown cleanup

[PATCH 0 of 3] libxl: domain shutdown cleanup

[Ian Campbell]

The existing libxl_domain_shutdown is a bit odd, it takes an integer
"req" which can be used to indicate one of:
  * [0] = "poweroff",
  * [1] = "reboot",
  * [2] = "suspend",
  * [3] = "crash",
  * [4] = "halt",

"suspend" is not usable via this interface since it requires other
scaffolding, libxl_domain_suspend provides this already.

"halt" is the same as "poweroff".

"crash" is unused and at least Linux does not implement it. If a user
steps forward then libxl_domain_crash is trivial to add.

Therefore split libxl_domain_shutdown into libxl_domain_shutdown and
libxl_domain_reboot corresponding to "poweroff" and "reboot"
respectively.

Also push responsibility for dealing with lack of PV drivers into the
caller and at the same time improve the error messages presented to
the user when they try and "xl shutdown/reboot" an HVM guest with no
PV drivers and the corresponding documentation.

[PATCH 1 of 3] libxl: add libxl__domain_pvcontrol_{available, read, write}

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323793378 0
# Node ID 97677918e3a4c8e724d7c8a6aba9cec570400c99
# Parent  f2e14742b2a2c1f40bc36063a1485b4072865fd1
libxl: add libxl__domain_pvcontrol_{available,read,write}

Use instead of open coding.

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

diff -r f2e14742b2a2 -r 97677918e3a4 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 16:10:20 2011 +0000
+++ b/tools/libxl/libxl.c	Tue Dec 13 16:22:58 2011 +0000
@@ -542,6 +542,58 @@ int libxl_domain_unpause(libxl_ctx *ctx,
     return rc;
 }
 
+int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    unsigned long pvdriver = 0;
+    int ret;
+
+    if (LIBXL__DOMAIN_IS_TYPE(gc, domid, PV))
+        return 1;
+
+    ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ, &pvdriver);
+    if (ret<0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting HVM callback IRQ");
+        return ERROR_FAIL;
+    }
+    return !!pvdriver;
+}
+
+char * libxl__domain_pvcontrol_read(libxl__gc *gc, xs_transaction_t t,
+                                    uint32_t domid)
+{
+    const char *shutdown_path;
+    const char *dom_path;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    if (!dom_path)
+        return NULL;
+
+    shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+    if (!shutdown_path)
+        return NULL;
+
+    return libxl__xs_read(gc, t, shutdown_path);
+}
+
+int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
+                                  uint32_t domid, const char *cmd)
+{
+    const char *shutdown_path;
+    const char *dom_path;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    if (!dom_path)
+        return ERROR_FAIL;
+
+    shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
+    if (!shutdown_path)
+        return ERROR_FAIL;
+
+    return libxl__xs_write(gc, t, shutdown_path, "%s", cmd);
+}
+
 static char *req_table[] = {
     [0] = "poweroff",
     [1] = "reboot",
@@ -553,42 +605,29 @@ static char *req_table[] = {
 int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req)
 {
     GC_INIT(ctx);
-    char *shutdown_path;
-    char *dom_path;
+    int ret;
 
     if (req > ARRAY_SIZE(req_table)) {
         GC_FREE;
         return ERROR_INVAL;
     }
 
-    dom_path = libxl__xs_get_dompath(gc, domid);
-    if (!dom_path) {
-        GC_FREE;
-        return ERROR_FAIL;
+    ret = libxl__domain_pvcontrol_available(gc, domid);
+    if (ret < 0)
+        goto out;
+
+    if (!ret) {
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV shutdown control not available:"
+                   " graceful shutdown not possible, use destroy");
+        ret = ERROR_FAIL;
+        goto out;
     }
 
-    if (LIBXL__DOMAIN_IS_TYPE(gc,  domid, HVM)) {
-        unsigned long pvdriver = 0;
-        int ret;
-        ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ, &pvdriver);
-        if (ret<0) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting HVM callback IRQ");
-            GC_FREE;
-            return ERROR_FAIL;
-        }
-        if (!pvdriver) {
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV drivers:"
-                       " graceful shutdown not possible, use destroy");
-            GC_FREE;
-            return ERROR_FAIL;
-        }
-    }
-
-    shutdown_path = libxl__sprintf(gc, "%s/control/shutdown", dom_path);
-    xs_write(ctx->xsh, XBT_NULL, shutdown_path, req_table[req], strlen(req_table[req]));
-
+    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, req_table[req]);
+
+out:
     GC_FREE;
-    return 0;
+    return ret;
 }
 
 int libxl_get_wait_fd(libxl_ctx *ctx, int *fd)
diff -r f2e14742b2a2 -r 97677918e3a4 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Tue Dec 13 16:10:20 2011 +0000
+++ b/tools/libxl/libxl_dom.c	Tue Dec 13 16:22:58 2011 +0000
@@ -415,7 +415,7 @@ static int libxl__domain_suspend_common_
     struct suspendinfo *si = data;
     unsigned long hvm_s_state = 0, hvm_pvdrv = 0;
     int ret;
-    char *path, *state = "suspend";
+    char *state = "suspend";
     int watchdog;
     libxl_ctx *ctx = libxl__gc_owner(si->gc);
     xs_transaction_t t;
@@ -451,15 +451,14 @@ static int libxl__domain_suspend_common_
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "issuing %s suspend request via XenBus control node",
                    si->hvm ? "PVHVM" : "PV");
 
-        path = libxl__sprintf(si->gc, "%s/control/shutdown", libxl__xs_get_dompath(si->gc, si->domid));
-        libxl__xs_write(si->gc, XBT_NULL, path, "suspend");
+        libxl__domain_pvcontrol_write(si->gc, XBT_NULL, si->domid, "suspend");
 
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "wait for the guest to acknowledge suspend request");
         watchdog = 60;
         while (!strcmp(state, "suspend") && watchdog > 0) {
             usleep(100000);
 
-            state = libxl__xs_read(si->gc, XBT_NULL, path);
+            state = libxl__domain_pvcontrol_read(si->gc, XBT_NULL, si->domid);
             if (!state) state = "";
 
             watchdog--;
@@ -479,11 +478,11 @@ static int libxl__domain_suspend_common_
         retry_transaction:
             t = xs_transaction_start(ctx->xsh);
 
-            state = libxl__xs_read(si->gc, t, path);
+            state = libxl__domain_pvcontrol_read(si->gc, t, si->domid);
             if (!state) state = "";
 
             if (!strcmp(state, "suspend"))
-                libxl__xs_write(si->gc, t, path, "");
+                libxl__domain_pvcontrol_write(si->gc, t, si->domid, "");
 
             if (!xs_transaction_end(ctx->xsh, t, 0))
                 if (errno == EAGAIN)
diff -r f2e14742b2a2 -r 97677918e3a4 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Dec 13 16:10:20 2011 +0000
+++ b/tools/libxl/libxl_internal.h	Tue Dec 13 16:22:58 2011 +0000
@@ -247,6 +247,12 @@ _hidden int libxl__domain_suspend_common
 _hidden int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd);
 _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);
 
+_hidden int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid);
+_hidden char * libxl__domain_pvcontrol_read(libxl__gc *gc,
+                                            xs_transaction_t t, uint32_t domid);
+_hidden int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
+                                          uint32_t domid, const char *cmd);
+
 /* from xl_device */
 _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend backend);
 _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);

[PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323793444 0
# Node ID e0f020d3d812e00aeb0c271a9627206279da3964
# Parent  97677918e3a4c8e724d7c8a6aba9cec570400c99
libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

The other integer request types which shutdown supported are not useful. Specifically:

 * "suspend" is not usable via this interface since it requires other
   scaffolding, libxl_domain_suspend provides this already.
 * "halt" is the same as "poweroff".
 * "crash" is unused and at least Linux does not implement it. If a user
   steps forward then libxl_domain_crash is trivial to add.

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

diff -r 97677918e3a4 -r e0f020d3d812 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/libxl.c	Tue Dec 13 16:24:04 2011 +0000
@@ -594,36 +594,46 @@ int libxl__domain_pvcontrol_write(libxl_
     return libxl__xs_write(gc, t, shutdown_path, "%s", cmd);
 }
 
-static char *req_table[] = {
-    [0] = "poweroff",
-    [1] = "reboot",
-    [2] = "suspend",
-    [3] = "crash",
-    [4] = "halt",
-};
-
-int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req)
+int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid)
 {
     GC_INIT(ctx);
     int ret;
 
-    if (req > ARRAY_SIZE(req_table)) {
-        GC_FREE;
-        return ERROR_INVAL;
-    }
-
     ret = libxl__domain_pvcontrol_available(gc, domid);
     if (ret < 0)
         goto out;
 
     if (!ret) {
-        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV shutdown control not available:"
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
                    " graceful shutdown not possible, use destroy");
         ret = ERROR_FAIL;
         goto out;
     }
 
-    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, req_table[req]);
+    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "poweroff");
+
+out:
+    GC_FREE;
+    return ret;
+}
+
+int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
+{
+    GC_INIT(ctx);
+    int ret;
+
+    ret = libxl__domain_pvcontrol_available(gc, domid);
+    if (ret < 0)
+        goto out;
+
+    if (!ret) {
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
+                   " graceful reboot not possible, use destroy+create");
+        ret = ERROR_FAIL;
+        goto out;
+    }
+
+    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "reboot");
 
 out:
     GC_FREE;
diff -r 97677918e3a4 -r e0f020d3d812 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Dec 13 16:24:04 2011 +0000
@@ -268,7 +268,8 @@ void libxl_domain_config_dispose(libxl_d
 int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
                           uint32_t domid, int fd);
 int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid);
-int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req);
+int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
+int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int force);
 int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
 
diff -r 97677918e3a4 -r e0f020d3d812 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Tue Dec 13 16:24:04 2011 +0000
@@ -2265,7 +2265,7 @@ static void shutdown_domain(const char *
     int rc;
 
     find_domain(p);
-    rc=libxl_domain_shutdown(ctx, domid, 0);
+    rc=libxl_domain_shutdown(ctx, domid);
     if (rc) { fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1); }
 
     if (wait) {
@@ -2307,7 +2307,7 @@ static void reboot_domain(const char *p)
 {
     int rc;
     find_domain(p);
-    rc=libxl_domain_shutdown(ctx, domid, 1);
+    rc=libxl_domain_reboot(ctx, domid);
     if (rc) { fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1); }
 }

Re: [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

[Ian Campbell]

On Tue, 2011-12-13 at 16:24 +0000, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1323793444 0
> # Node ID e0f020d3d812e00aeb0c271a9627206279da3964
> # Parent  97677918e3a4c8e724d7c8a6aba9cec570400c99
> libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot
> 
> The other integer request types which shutdown supported are not useful. Specifically:
> 
>  * "suspend" is not usable via this interface since it requires other
>    scaffolding, libxl_domain_suspend provides this already.
>  * "halt" is the same as "poweroff".
>  * "crash" is unused and at least Linux does not implement it. If a user
>    steps forward then libxl_domain_crash is trivial to add.
> 

I forgot to build test the language bindings and of course this broke
them. Please stand by for an updated patch.

Re: [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

[Ian Campbell]

On Tue, 2011-12-13 at 17:11 +0000, Ian Campbell wrote:
> On Tue, 2011-12-13 at 16:24 +0000, Ian Campbell wrote:
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1323793444 0
> > # Node ID e0f020d3d812e00aeb0c271a9627206279da3964
> > # Parent  97677918e3a4c8e724d7c8a6aba9cec570400c99
> > libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot
> > 
> > The other integer request types which shutdown supported are not useful. Specifically:
> > 
> >  * "suspend" is not usable via this interface since it requires other
> >    scaffolding, libxl_domain_suspend provides this already.
> >  * "halt" is the same as "poweroff".
> >  * "crash" is unused and at least Linux does not implement it. If a user
> >    steps forward then libxl_domain_crash is trivial to add.
> > 
> 
> I forgot to build test the language bindings and of course this broke
> them. Please stand by for an updated patch.

8<------------------------------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323796506 0
# Node ID c03b6bccb479740f7c70b96f4b1c0951dd70e615
# Parent  97677918e3a4c8e724d7c8a6aba9cec570400c99
libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

The other integer request types which shutdown supported are not useful. Specifically:

 * "suspend" is not usable via this interface since it requires other
   scaffolding, libxl_domain_suspend provides this already.
 * "halt" is the same as "poweroff".
 * "crash" is unused and at least Linux does not implement it. If a user
   steps forward then libxl_domain_crash is trivial to add.

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

diff -r 97677918e3a4 -r c03b6bccb479 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/libxl.c	Tue Dec 13 17:15:06 2011 +0000
@@ -594,36 +594,46 @@ int libxl__domain_pvcontrol_write(libxl_
     return libxl__xs_write(gc, t, shutdown_path, "%s", cmd);
 }
 
-static char *req_table[] = {
-    [0] = "poweroff",
-    [1] = "reboot",
-    [2] = "suspend",
-    [3] = "crash",
-    [4] = "halt",
-};
-
-int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req)
+int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid)
 {
     GC_INIT(ctx);
     int ret;
 
-    if (req > ARRAY_SIZE(req_table)) {
-        GC_FREE;
-        return ERROR_INVAL;
-    }
-
     ret = libxl__domain_pvcontrol_available(gc, domid);
     if (ret < 0)
         goto out;
 
     if (!ret) {
-        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV shutdown control not available:"
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
                    " graceful shutdown not possible, use destroy");
         ret = ERROR_FAIL;
         goto out;
     }
 
-    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, req_table[req]);
+    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "poweroff");
+
+out:
+    GC_FREE;
+    return ret;
+}
+
+int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
+{
+    GC_INIT(ctx);
+    int ret;
+
+    ret = libxl__domain_pvcontrol_available(gc, domid);
+    if (ret < 0)
+        goto out;
+
+    if (!ret) {
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
+                   " graceful reboot not possible, use destroy+create");
+        ret = ERROR_FAIL;
+        goto out;
+    }
+
+    ret = libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, "reboot");
 
 out:
     GC_FREE;
diff -r 97677918e3a4 -r c03b6bccb479 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Dec 13 17:15:06 2011 +0000
@@ -268,7 +268,8 @@ void libxl_domain_config_dispose(libxl_d
 int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
                           uint32_t domid, int fd);
 int libxl_domain_resume(libxl_ctx *ctx, uint32_t domid);
-int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req);
+int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid);
+int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid, int force);
 int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
 
diff -r 97677918e3a4 -r c03b6bccb479 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Tue Dec 13 17:15:06 2011 +0000
@@ -2265,7 +2265,7 @@ static void shutdown_domain(const char *
     int rc;
 
     find_domain(p);
-    rc=libxl_domain_shutdown(ctx, domid, 0);
+    rc=libxl_domain_shutdown(ctx, domid);
     if (rc) { fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1); }
 
     if (wait) {
@@ -2307,7 +2307,7 @@ static void reboot_domain(const char *p)
 {
     int rc;
     find_domain(p);
-    rc=libxl_domain_shutdown(ctx, domid, 1);
+    rc=libxl_domain_reboot(ctx, domid);
     if (rc) { fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1); }
 }
 
diff -r 97677918e3a4 -r c03b6bccb479 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue Dec 13 16:22:58 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Tue Dec 13 17:15:06 2011 +0000
@@ -424,10 +424,10 @@ static PyObject *pyxl_domid_to_name(XlOb
 
 static PyObject *pyxl_domain_shutdown(XlObject *self, PyObject *args)
 {
-    int domid, req = 0;
-    if ( !PyArg_ParseTuple(args, "i|i", &domid, &req) )
+    int domid;
+    if ( !PyArg_ParseTuple(args, "i", &domid) )
         return NULL;
-    if ( libxl_domain_shutdown(self->ctx, domid, req) ) {
+    if ( libxl_domain_shutdown(self->ctx, domid) ) {
         PyErr_SetString(xl_error_obj, "cannot shutdown domain");
         return NULL;
     }
@@ -435,6 +435,19 @@ static PyObject *pyxl_domain_shutdown(Xl
     return Py_None;
 }
 
+static PyObject *pyxl_domain_reboot(XlObject *self, PyObject *args)
+{
+    int domid;
+    if ( !PyArg_ParseTuple(args, "i", &domid) )
+        return NULL;
+    if ( libxl_domain_reboot(self->ctx, domid) ) {
+        PyErr_SetString(xl_error_obj, "cannot reboot domain");
+        return NULL;
+    }
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+
 static PyObject *pyxl_domain_destroy(XlObject *self, PyObject *args)
 {
     int domid, force = 1;
@@ -637,6 +650,8 @@ static PyMethodDef pyxl_methods[] = {
          "Retrieve name from domain-id"},
     {"domain_shutdown", (PyCFunction)pyxl_domain_shutdown, METH_VARARGS,
          "Shutdown a domain"},
+    {"domain_reboot", (PyCFunction)pyxl_domain_reboot, METH_VARARGS,
+         "Reboot a domain"},
     {"domain_destroy", (PyCFunction)pyxl_domain_destroy, METH_VARARGS,
          "Destroy a domain"},
     {"domain_pause", (PyCFunction)pyxl_domain_pause, METH_VARARGS,

Re: [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot"):
> libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot
> 
> The other integer request types which shutdown supported are not useful. Specifically:
> 
>  * "suspend" is not usable via this interface since it requires other
>    scaffolding, libxl_domain_suspend provides this already.
>  * "halt" is the same as "poweroff".
>  * "crash" is unused and at least Linux does not implement it. If a user
>    steps forward then libxl_domain_crash is trivial to add.

The effect of this is to duplicate 25 lines of code.

If you really think we should split these into separate functions,
rather than just sorting out the faux-enum, the separate functions
probably want to turn out to be something like:

    int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
    {
        return libxl__domain_pvcontrol(ctx, domid, "reboot");
    }

Ian.

Re: [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot

[Ian Campbell]

On Thu, 2011-12-15 at 17:05 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2 of 3] libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot"):
> > libxl: split libxl_domain_shutdown into libxl_domain_shutdown & libxl_domain_reboot
> > 
> > The other integer request types which shutdown supported are not useful. Specifically:
> > 
> >  * "suspend" is not usable via this interface since it requires other
> >    scaffolding, libxl_domain_suspend provides this already.
> >  * "halt" is the same as "poweroff".
> >  * "crash" is unused and at least Linux does not implement it. If a user
> >    steps forward then libxl_domain_crash is trivial to add.
> 
> The effect of this is to duplicate 25 lines of code.

Ye, I should have rethought this after I pulled the fallback handling
out of the library into xl (originally the two function had different
fallbacks). I'll rework it.

Ian.

> 
> If you really think we should split these into separate functions,
> rather than just sorting out the faux-enum, the separate functions
> probably want to turn out to be something like:
> 
>     int libxl_domain_reboot(libxl_ctx *ctx, uint32_t domid)
>     {
>         return libxl__domain_pvcontrol(ctx, domid, "reboot");
>     }
> 
> Ian.

[PATCH 3 of 3] libxl: report failure to reboot/shutdown due to lackof PV interfaces to caller

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323793457 0
# Node ID 47606788dec2f110aecf5b356b250d65343070ac
# Parent  e0f020d3d812e00aeb0c271a9627206279da3964
libxl: report failure to reboot/shutdown due to lackof PV interfaces to caller

This allow the caller to react as they think is appropriate. xl now prints a
message much like the library did previously, although hopefully somewhat more
informative.

Update the xl(1) man page to be similarly more informative.

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

diff -r e0f020d3d812 -r 47606788dec2 docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Tue Dec 13 16:24:04 2011 +0000
+++ b/docs/man/xl.pod.1	Tue Dec 13 16:24:17 2011 +0000
@@ -368,7 +368,11 @@ Reboot a domain.  This acts just as if t
 command run from the console.  The command returns as soon as it has
 executed the reboot action, which may be significantly before the
 domain actually reboots.
-It requires PV drivers installed in your guest OS.
+
+For HVM domains this requires PV drivers to be installed in your guest
+OS. If PV drivers are not present but you have configured the guest OS
+to behave appropriately you may be able to use the I<button-press>
+subcommand to trigger a power button press.
 
 The behavior of what happens to a domain when it reboots is set by the
 B<on_reboot> parameter of the domain configuration file when the
@@ -424,9 +428,15 @@ Leave domain running after creating the 
 Gracefully shuts down a domain.  This coordinates with the domain OS
 to perform graceful shutdown, so there is no guarantee that it will
 succeed, and may take a variable length of time depending on what
-services must be shutdown in the domain.  The command returns
-immediately after signally the domain unless that B<-w> flag is used.
-For HVM domains it requires PV drivers to be installed in your guest OS.
+services must be shutdown in the domain.
+
+For HVM domains this requires PV drivers to be installed in your guest
+OS. If PV drivers are not present but you have configured the guest OS
+to behave appropriately you may be able to use the I<button-press>
+subcommand to trigger a power button press.
+
+The command returns immediately after signally the domain unless that
+B<-w> flag is used.
 
 The behavior of what happens to a domain when it reboots is set by the
 B<on_shutdown> parameter of the domain configuration file when the
diff -r e0f020d3d812 -r 47606788dec2 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 13 16:24:04 2011 +0000
+++ b/tools/libxl/libxl.c	Tue Dec 13 16:24:17 2011 +0000
@@ -604,9 +604,7 @@ int libxl_domain_shutdown(libxl_ctx *ctx
         goto out;
 
     if (!ret) {
-        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
-                   " graceful shutdown not possible, use destroy");
-        ret = ERROR_FAIL;
+        ret = ERROR_NOPARAVIRT;
         goto out;
     }
 
@@ -627,9 +625,7 @@ int libxl_domain_reboot(libxl_ctx *ctx, 
         goto out;
 
     if (!ret) {
-        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "PV control interface not available:"
-                   " graceful reboot not possible, use destroy+create");
-        ret = ERROR_FAIL;
+        ret = ERROR_NOPARAVIRT;
         goto out;
     }
 
diff -r e0f020d3d812 -r 47606788dec2 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Dec 13 16:24:04 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Dec 13 16:24:17 2011 +0000
@@ -222,6 +222,7 @@ enum {
     ERROR_BADFAIL = -7,
     ERROR_GUEST_TIMEDOUT = -8,
     ERROR_TIMEDOUT = -9,
+    ERROR_NOPARAVIRT = -10,
 };
 
 #define LIBXL_VERSION 0
diff -r e0f020d3d812 -r 47606788dec2 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Dec 13 16:24:04 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Tue Dec 13 16:24:17 2011 +0000
@@ -2266,7 +2266,15 @@ static void shutdown_domain(const char *
 
     find_domain(p);
     rc=libxl_domain_shutdown(ctx, domid);
-    if (rc) { fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1); }
+    if (rc) {
+        if (rc == ERROR_NOPARAVIRT) {
+            fprintf(stderr, "PV control interface not available:"
+                    " external graceful shutdown not possible.\n");
+            fprintf(stderr, "Use \"xl button-press <dom> power\" or"
+                    " \"xl destroy <dom>\".\n");
+        }
+        fprintf(stderr,"shutdown failed (rc=%d)\n",rc);exit(-1);
+    }
 
     if (wait) {
         libxl_waiter waiter;
@@ -2308,7 +2316,14 @@ static void reboot_domain(const char *p)
     int rc;
     find_domain(p);
     rc=libxl_domain_reboot(ctx, domid);
-    if (rc) { fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1); }
+    if (rc) {
+        if (rc == ERROR_NOPARAVIRT) {
+            fprintf(stderr, "PV control interface not available:"
+                    " external graceful reboot not possible.\n");
+            fprintf(stderr, "Use \"xl button-press <dom> power\" or"
+                    " \"xl destroy <dom>\".\n");
+        }
+        fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1); }
 }
 
 static void list_domains_details(const libxl_dominfo *info, int nb_domain)