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@xxxxxxxxxxxxx>
Acked/Reviewd-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home