Re: [Xen-devel] [PATCH 17/19] libxl: suspend: Fix suspend wait corner cases

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-03-04 at 14:56 +0000, Ian Jackson wrote:
> When we are waiting for a guest to suspend, this suspend operation
> would continue to wait (until the timeout) if the guest was destroyed
> or shut down for another reason, or if xc_domain_getinfolist failed.
> 
> Handle these cases correctly, as errors.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_dom.c |   42 ++++++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 6291115..88700ee 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1208,24 +1208,42 @@ static void 
> suspend_common_wait_guest_watch(libxl__egc *egc,
>      STATE_AO_GC(dss->ao);
>      xc_domaininfo_t info;
>      int ret;
> +    int shutdown_reason;
>  
>      /* Convenience aliases */
>      const uint32_t domid = dss->domid;
>  
>      ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info);
> -    if (ret == 1 && info.domain == domid &&
> -        (info.flags & XEN_DOMINF_shutdown)) {
> -        int shutdown_reason;
> -
> -        shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
> -            & XEN_DOMINF_shutdownmask;
> -        if (shutdown_reason == SHUTDOWN_suspend) {
> -            LOG(DEBUG, "guest has suspended");
> -            domain_suspend_common_guest_suspended(egc, dss);
> -            return;
> -        }
> +    if (ret < 0) {
> +        LOGE(ERROR, "unable to check for status of guest %"PRId32"", domid);
> +        goto err;
> +        domain_suspend_common_failed(egc, dss);

You don't want this here.

> +    }
> +
> +    if (!(ret == 1 && info.domain == domid)) {
> +        LOGE(ERROR, "guest %"PRId32" we were suspending has been destroyed",
> +             domid);

Is there an (unlikely) race here where a new domain gets created with
the same domid? Not that I have any suggestion what to do about that...

> +        goto err;
> +    }
> +
> +    if (!(info.flags & XEN_DOMINF_shutdown))
> +        /* keep waiting */
> +        return;
> +
> +    shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
> +        & XEN_DOMINF_shutdownmask;
> +    if (shutdown_reason != SHUTDOWN_suspend) {
> +        LOG(DEBUG, "guest %"PRId32" we were suspending has shut down"
> +            " with unexpected reason code %d", domid, shutdown_reason);
> +        goto err;
>      }
> -    /* otherwise, keep waiting */
> +
> +    LOG(DEBUG, "guest has suspended");
> +    domain_suspend_common_guest_suspended(egc, dss);
> +    return;
> +
> + err:
> +    domain_suspend_common_failed(egc, dss);
>  }
>  
>  static void suspend_common_wait_guest_timeout(libxl__egc *egc,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel