WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] libxl: Make domain_shutdown fail if graceful not possible
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Thu, 20 Jan 2011 16:52:44 +0000
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 20 Jan 2011 08:56:48 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <19768.25441.492749.84174@xxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <19767.6464.67438.970597@xxxxxxxxxxxxxxxxxxxxxxxx> <1295457314.14981.3660.camel@xxxxxxxxxxxxxxxxxxxxxx> <19768.25441.492749.84174@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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