Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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