Ian Jackson
2011-Jan-19 17:02 UTC
[Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
Currently "xl shutdown" (like "xm shutdown") is not capable
of doing
the proper ACPI negotiation with an HVM no-pv-drivers guest which
would be necessary for a graceful shutdown.
Instead (following the ill-advised lead of "xm shutdown") it simply
shoots the guest in the head.
This patch changes the behaviour so that "xl shutdown" fails if the
domain cannot be shut down gracefully for this reason and suggests in
the error message using destroy instead.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7ffb985..2cc2d21 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -510,29 +510,19 @@ int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid,
int req)
xs_write(ctx->xsh, XBT_NULL, shutdown_path, req_table[req],
strlen(req_table[req]));
if (libxl__domain_is_hvm(ctx,domid)) {
- unsigned long acpi_s_state = 0;
unsigned long pvdriver = 0;
int ret;
- ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE,
&acpi_s_state);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting ACPI
S-state");
- libxl__free_all(&gc);
- return ERROR_FAIL;
- }
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");
libxl__free_all(&gc);
return ERROR_FAIL;
}
- if (!pvdriver || acpi_s_state != 0) {
- ret = xc_domain_shutdown(ctx->xch, domid, req);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unpausing
domain");
- libxl__free_all(&gc);
- return ERROR_FAIL;
- }
- }
+ if (!pvdriver) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV
drivers:"
+ " graceful shutdown not possible, use
destroy");
+ return ERROR_FAIL;
+ }
}
libxl__free_all(&gc);
return 0;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-19 17:13 UTC
[Xen-devel] Re: [PATCH] libxl: Make domain_shutdown fail if graceful not possible
I wrote:> Currently "xl shutdown" (like "xm shutdown") is not capable of doing > the proper ACPI negotiation with an HVM no-pv-drivers guest which > would be necessary for a graceful shutdown.That patch leaks the gc. Here''s a revised version. Ian. libxl: Make domain_shutdown fail if graceful not possible Currently "xl shutdown" (like "xm shutdown") is not capable of doing the proper ACPI negotiation with an HVM no-pv-drivers guest which would be necessary for a graceful shutdown. Instead (following the ill-advised lead of "xm shutdown") it simply shoots the guest in the head. This patch changes the behaviour so that "xl shutdown" fails if the domain cannot be shut down gracefully for this reason and suggests in the error message using destroy instead. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 7ffb985..00a68bb 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -510,29 +510,20 @@ int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req) xs_write(ctx->xsh, XBT_NULL, shutdown_path, req_table[req], strlen(req_table[req])); if (libxl__domain_is_hvm(ctx,domid)) { - unsigned long acpi_s_state = 0; unsigned long pvdriver = 0; int ret; - ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE, &acpi_s_state); + ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ, &pvdriver); if (ret<0) { - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting ACPI S-state"); + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting HVM callback IRQ"); libxl__free_all(&gc); return ERROR_FAIL; } - 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"); + if (!pvdriver) { + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV drivers:" + " graceful shutdown not possible, use destroy"); libxl__free_all(&gc); return ERROR_FAIL; } - if (!pvdriver || acpi_s_state != 0) { - ret = xc_domain_shutdown(ctx->xch, domid, req); - if (ret<0) { - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unpausing domain"); - libxl__free_all(&gc); - return ERROR_FAIL; - } - } } libxl__free_all(&gc); return 0; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-19 17:15 UTC
Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
On Wed, 2011-01-19 at 17:02 +0000, Ian Jackson wrote:> Currently "xl shutdown" (like "xm shutdown") is not capable of doing > the proper ACPI negotiation with an HVM no-pv-drivers guest which > would be necessary for a graceful shutdown. > > Instead (following the ill-advised lead of "xm shutdown") it simply > shoots the guest in the head. > > This patch changes the behaviour so that "xl shutdown" fails if the > domain cannot be shut down gracefully for this reason and suggests in > the error message using destroy instead. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>I strongly agree with the principal that graceful commands shouldn''t fallback automatically to a non-graceful variant! Acked/Reviewed-by: Ian Campbell <ian.campbell@citrix.com> Perhaps the check should be before the xs_write though -- i.e. check for pvdriver support before attempting to use it? Ian.> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 7ffb985..2cc2d21 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -510,29 +510,19 @@ int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid, int req) > > xs_write(ctx->xsh, XBT_NULL, shutdown_path, req_table[req], strlen(req_table[req])); > if (libxl__domain_is_hvm(ctx,domid)) { > - unsigned long acpi_s_state = 0; > unsigned long pvdriver = 0; > int ret; > - ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE, &acpi_s_state); > - if (ret<0) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting ACPI S-state"); > - libxl__free_all(&gc); > - return ERROR_FAIL; > - } > 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"); > libxl__free_all(&gc); > return ERROR_FAIL; > } > - if (!pvdriver || acpi_s_state != 0) { > - ret = xc_domain_shutdown(ctx->xch, domid, req); > - if (ret<0) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unpausing domain"); > - libxl__free_all(&gc); > - return ERROR_FAIL; > - } > - } > + if (!pvdriver) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV drivers:" > + " graceful shutdown not possible, use destroy"); > + return ERROR_FAIL; > + } > } > libxl__free_all(&gc); > return 0; > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-20 16:31 UTC
Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown
fail if graceful not possible"):> Perhaps the check should be before the xs_write though -- i.e. check for
> pvdriver support before attempting to use it?
A sensible suggestion.
Ian.
libxl: Make domain_shutdown fail if graceful not possible
Currently "xl shutdown" (like "xm shutdown") is not capable
of doing
the proper ACPI negotiation with an HVM no-pv-drivers guest which
would be necessary for a graceful shutdown.
Instead (following the ill-advised lead of "xm shutdown") it simply
shoots the guest in the head.
This patch changes the behaviour so that "xl shutdown" fails if the
domain cannot be shut down gracefully for this reason and suggests in
the error message using destroy instead.
Also, check whether the PV shutdown protocol is available before we
try to use it.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 051a1b1b8f8a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Wed Jan 19 18:24:26 2011 +0000
+++ b/tools/libxl/libxl.c Thu Jan 20 16:29:54 2011 +0000
@@ -506,34 +506,26 @@ int libxl_domain_shutdown(libxl_ctx *ctx
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]));
if (libxl__domain_is_hvm(ctx,domid)) {
- unsigned long acpi_s_state = 0;
unsigned long pvdriver = 0;
int ret;
- ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE,
&acpi_s_state);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting ACPI
S-state");
- libxl__free_all(&gc);
- return ERROR_FAIL;
- }
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");
libxl__free_all(&gc);
return ERROR_FAIL;
}
- if (!pvdriver || acpi_s_state != 0) {
- ret = xc_domain_shutdown(ctx->xch, domid, req);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unpausing
domain");
- libxl__free_all(&gc);
- return ERROR_FAIL;
- }
- }
+ if (!pvdriver) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV
drivers:"
+ " graceful shutdown not possible, use
destroy");
+ libxl__free_all(&gc);
+ 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]));
+
libxl__free_all(&gc);
return 0;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-20 16:52 UTC
Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
On Thu, 2011-01-20 at 16:31 +0000, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible"): > > Perhaps the check should be before the xs_write though -- i.e. check for > > pvdriver support before attempting to use it? > > A sensible suggestion. > > Ian. > > libxl: Make domain_shutdown fail if graceful not possible > > Currently "xl shutdown" (like "xm shutdown") is not capable of doing > the proper ACPI negotiation with an HVM no-pv-drivers guest which > would be necessary for a graceful shutdown. > > Instead (following the ill-advised lead of "xm shutdown") it simply > shoots the guest in the head. > > This patch changes the behaviour so that "xl shutdown" fails if the > domain cannot be shut down gracefully for this reason and suggests in > the error message using destroy instead. > > Also, check whether the PV shutdown protocol is available before we > try to use it. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>Acked/Reviewd-by: Ian Campbell <ian.campbell@citrix.com> Not related to this patch, other than I happened to notice it while reviewingm but the "req" paramter to libxl_domain_shutdown is pretty nasty. It can apparently taken a number from 0-4 inclusive, with no symbolic names, but which are translated per: static char *req_table[] = { [0] = "poweroff", [1] = "reboot", [2] = "suspend", [3] = "crash", [4] = "halt", }; However only 0 and 1 are ever actually passed by xl and I''d wager that only 0, 1 and 4 are even vaguely valid in this context. suspend has it''s own libxl function and iirc crash is something only a guest does... I''m not sure what the distinction between poweroff and halt is (pvops Linux apparently treats them the same) but I''d suggest that having libxl_domain_shutdown => "poweroff" and libxl_domain_reboot => "reboot" with the existing function becoming a common internal backend makes sense. Ian.> > diff -r 051a1b1b8f8a tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Wed Jan 19 18:24:26 2011 +0000 > +++ b/tools/libxl/libxl.c Thu Jan 20 16:29:54 2011 +0000 > @@ -506,34 +506,26 @@ int libxl_domain_shutdown(libxl_ctx *ctx > 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])); > if (libxl__domain_is_hvm(ctx,domid)) { > - unsigned long acpi_s_state = 0; > unsigned long pvdriver = 0; > int ret; > - ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE, &acpi_s_state); > - if (ret<0) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting ACPI S-state"); > - libxl__free_all(&gc); > - return ERROR_FAIL; > - } > 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"); > libxl__free_all(&gc); > return ERROR_FAIL; > } > - if (!pvdriver || acpi_s_state != 0) { > - ret = xc_domain_shutdown(ctx->xch, domid, req); > - if (ret<0) { > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unpausing domain"); > - libxl__free_all(&gc); > - return ERROR_FAIL; > - } > - } > + if (!pvdriver) { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "HVM domain without PV drivers:" > + " graceful shutdown not possible, use destroy"); > + libxl__free_all(&gc); > + 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])); > + > libxl__free_all(&gc); > return 0; > }_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-20 17:05 UTC
Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown
fail if graceful not possible"):> Acked/Reviewd-by: Ian Campbell <ian.campbell@citrix.com>
Thanks, I''ve applied it.
> Not related to this patch, other than I happened to notice it while
> reviewingm but the "req" paramter to libxl_domain_shutdown is
pretty
> nasty. It can apparently taken a number from 0-4 inclusive, with no
> symbolic names, but which are translated per:
>
> static char *req_table[] = {
> [0] = "poweroff",
> [1] = "reboot",
> [2] = "suspend",
> [3] = "crash",
> [4] = "halt",
> };
Yuk.
> However only 0 and 1 are ever actually passed by xl and I''d wager
that
> only 0, 1 and 4 are even vaguely valid in this context. suspend has
it''s
> own libxl function and iirc crash is something only a guest does...
Yes.
> I''m not sure what the distinction between poweroff and halt is
(pvops
> Linux apparently treats them the same) but I''d suggest that having
> libxl_domain_shutdown => "poweroff" and libxl_domain_reboot
=> "reboot"
> with the existing function becoming a common internal backend makes
> sense.
Yes, but for 4.2 I think.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-20 17:07 UTC
Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
On Thu, 2011-01-20 at 17:05 +0000, Ian Jackson wrote:> > I''m not sure what the distinction between poweroff and halt is (pvops > > Linux apparently treats them the same) but I''d suggest that having > > libxl_domain_shutdown => "poweroff" and libxl_domain_reboot => "reboot" > > with the existing function becoming a common internal backend makes > > sense. > > Yes, but for 4.2 I think.ACK. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel