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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-31 at 01:05 +0000, rshriram@xxxxxxxxx wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@xxxxxxxxx>
> # Date 1327971511 28800
> # Node ID cd3d43d88c0568142dd061e744c34479e1a440f4
> # Parent  39f63438767a8225a3148a43139c10f55a62c669
> libxl: QMP stop/resume & refactor QEMU suspend/resume/save
> 
> Implement QMP stop and resume functionality and split
> device model save into 3 parts:
>  suspend_dm(domid)
>  save_dm(domid, fd)
>  resume_dm(domid)
> 
> Integrate Device model suspend into suspend_common_callback
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>
> 
> diff -r 39f63438767a -r cd3d43d88c05 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Mon Jan 30 16:58:24 2012 -0800
> +++ b/tools/libxl/libxl_dom.c Mon Jan 30 16:58:31 2012 -0800
> @@ -425,6 +425,61 @@ static int libxl__domain_suspend_common_
>      return rc ? 0 : 1;
>  }
>  
> +int libxl__domain_suspend_device_model(libxl__gc *gc, uint32_t domid)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int ret = 0, fd2 = -1;
> +    const char *filename = libxl__device_model_savefile(gc, domid);
> +
> +    switch (libxl__device_model_version_running(gc, domid)) {
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> +        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
> +                   "Saving device model state to %s", filename);
> +        libxl__qemu_traditional_cmd(gc, domid, "save");
> +        libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL, NULL);
> +        break;
> +    }
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +        if (libxl__qmp_stop(gc, domid))
> +            return ERROR_FAIL;
> +        fd2 = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | 
> S_IWUSR);
> +        if (fd2 < 0) {
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                             "Unable to create a QEMU save file\n");
> +            return ERROR_FAIL;
> +        }
> +        /* Save DM state into fd2 */
> +        ret = libxl__qmp_migrate(gc, domid, fd2);
> +        if (ret)
> +            unlink(filename);
> +        close(fd2);
> +        fd2 = -1;
> +        break;
> +    default:
> +        return ERROR_INVAL;
> +    }
> +
> +    return ret;
> +}
> +
> +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"?

> +        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?

> +    if (si->hvm) {
> +        ret = libxl__domain_suspend_device_model(si->gc, si->domid);
> +        if (ret) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                       "libxl__domain_suspend_device_model failed ret=%d", 
> ret);
> +            return 0;
> +        }
> +    }
> +    return 1;
>  }
>  

Ian.



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