[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3 of 6] libxl: QMP stop/resume & refactor QEMU suspend/resume/save



On Tue, Jan 31, 2012 at 9:32 AM, Shriram Rajagopalan <rshriram@xxxxxxxxx> wrote:
On Tue, Jan 31, 2012 at 1:54 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
On Tue, 2012-01-31 at 01:05 +0000, rshriram@xxxxxxxxx wrote:
> +int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
> +{
> +
> +    switch (libxl__device_model_version_running(gc, domid)) {
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> +        libxl__qemu_traditional_cmd(gc, domid, "continue");

No libxl__wait_for_device_model -> "running"?


Nope. Thats how xend/remus also does it. There seems to be no reference to such
a state anywhere.
 
> +        break;
> +    }
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +        if (libxl__qmp_resume(gc, domid))
> +            return ERROR_FAIL;
> +    default:
> +        return ERROR_INVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static int libxl__domain_suspend_common_callback(void *data)
>  {
>      struct suspendinfo *si = data;
> @@ -454,7 +509,7 @@ static int libxl__domain_suspend_common_
>              return 0;
>          }
>          si->guest_responded = 1;
> -        return 1;
> +        goto suspend_dm;
>      }
>
>      if (si->hvm && (!hvm_pvdrv || hvm_s_state)) {
> @@ -532,7 +587,7 @@ static int libxl__domain_suspend_common_
>              shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask;
>              if (shutdown_reason == SHUTDOWN_suspend) {
>                  LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest has suspended");
> -                return 1;
> +                goto suspend_dm;
>              }
>          }
>
> @@ -541,6 +596,17 @@ static int libxl__domain_suspend_common_
>
>      LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest did not suspend");
>      return 0;
> +
> + suspend_dm:

The goto's make the code flow a bit tricky to follow (this function is
already a bit tricksy with the early exit for the evtchn suspend case).

Why not pass si to libxl__domain_suspend_device_model and let it contain
the "if !hvm return" and logging stuff so you can just call in in place
of the two goto statements?


will do.


I gave it a shot. Passing suspendinfo struct to suspend_device_model is not
feasible, as the function is declared in libxl_internal.h, but suspendinfo struct
declaration is local to libxl_dom.c.  I am not sure if its worthwhile to
declare a private struct like suspendinfo in libxl_internal.h, just to get rid of this goto.

OTOH, passing a dummy hvm parameter to the function looks a bit silly

libxl__domain_suspend_device_model(libxl__gc *gc, uint32_t domid, int hvm)

What do you think? goto needs to go?

shriram
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.