Re: [Xen-devel] [PATCH v3 COLOPre 12/26] tools/libxl: Update libxl_domain_unpause() to support qemu-xen

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-07-01 at 10:10 +0800, Yang Hongyang wrote:
> 
> On 06/30/2015 06:00 PM, Ian Campbell wrote:
> > On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
> >> Currently, libxl__domain_unpause() only supports
> >> qemu-xen-traditional. Update it to support qemu-xen.
> >> We use libxl__domain_resume_device_model to unpause guest dm.
> >
> > There appears to be at least two significant semantic changes to the
> > libxl__domain_resume_device_model function (namely it now deals
> > internally with stubdom and checks the current status before doing
> > anything). What is the impact on the existing callers of those changes?
> 
> This change was suggested by Wei on last round(v2), he thought it is an
> enhancement to the existing callers.

I don't think that contradicts what I've said. If multiple improvements
are needed to make libxl__domain_resume_device_model suitable for your
uses then there should be several patches. This will also give you an
opportunity to explain in each commit message why there is no impact (or
a positive impact) on the existing callers.

> 
> >
> > I think those changes should be two separate preparatory patches.
> >
> >
> >>
> >> Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
> >> ---
> >>   tools/libxl/libxl.c             | 15 +++++----------
> >>   tools/libxl/libxl_dom_suspend.c | 15 ++++++++++++---
> >>   tools/libxl/libxl_internal.h    |  1 +
> >>   3 files changed, 18 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> index 2ca59ea..59e2dfe 100644
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -938,8 +938,6 @@ out:
> >>   int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
> >>   {
> >>       GC_INIT(ctx);
> >> -    char *path;
> >> -    char *state;
> >>       int ret, rc = 0;
> >>
> >>       libxl_domain_type type = libxl__domain_type(gc, domid);
> >> @@ -949,14 +947,11 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t 
> >> domid)
> >>       }
> >>
> >>       if (type == LIBXL_DOMAIN_TYPE_HVM) {
> >> -        uint32_t dm_domid = libxl_get_stubdom_id(ctx, domid);
> >> -
> >> -        path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
> >> -        state = libxl__xs_read(gc, XBT_NULL, path);
> >> -        if (state != NULL && !strcmp(state, "paused")) {
> >> -            libxl__qemu_traditional_cmd(gc, domid, "continue");
> >> -            libxl__wait_for_device_model_deprecated(gc, domid, "running",
> >> -                                         NULL, NULL, NULL);
> >> +        rc = libxl__domain_resume_device_model(gc, domid);
> >> +        if (rc < 0) {
> >> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to unpause device 
> >> model "
> >> +                       "for domain %u:%d", domid, rc);
> >> +            goto out;
> >>           }
> >>       }
> >>       ret = xc_domain_unpause(ctx->xch, domid);
> >> diff --git a/tools/libxl/libxl_dom_suspend.c 
> >> b/tools/libxl/libxl_dom_suspend.c
> >> index 1c486c4..3edbd2e 100644
> >> --- a/tools/libxl/libxl_dom_suspend.c
> >> +++ b/tools/libxl/libxl_dom_suspend.c
> >> @@ -376,13 +376,22 @@ static void 
> >> domain_suspend_callback_common_done(libxl__egc *egc,
> >>
> >>   /*======================= Domain resume ========================*/
> >>
> >> -static int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t 
> >> domid)
> >> +int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
> >>   {
> >> +    char *path;
> >> +    char *state;
> >>
> >>       switch (libxl__device_model_version_running(gc, domid)) {
> >>       case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> >> -        libxl__qemu_traditional_cmd(gc, domid, "continue");
> >> -        libxl__wait_for_device_model_deprecated(gc, domid, "running", 
> >> NULL, NULL, NULL);
> >> +        uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
> >> +
> >> +        path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
> >> +        state = libxl__xs_read(gc, XBT_NULL, path);
> >> +        if (state != NULL && !strcmp(state, "paused")) {
> >> +            libxl__qemu_traditional_cmd(gc, domid, "continue");
> >> +            libxl__wait_for_device_model_deprecated(gc, domid, "running",
> >> +                                                    NULL, NULL, NULL);
> >> +        }
> >>           break;
> >>       }
> >>       case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> >> index 0adc9b4..6960280 100644
> >> --- a/tools/libxl/libxl_internal.h
> >> +++ b/tools/libxl/libxl_internal.h
> >> @@ -3326,6 +3326,7 @@ static inline bool libxl__save_helper_inuse(const 
> >> libxl__save_helper_state *shs)
> >>   /* Each time the dm needs to be saved, we must call suspend and then 
> >> save */
> >>   _hidden int libxl__domain_suspend_device_model(libxl__gc *gc,
> >>                                              libxl__domain_suspend_state 
> >> *dsps);
> >> +_hidden int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t 
> >> domid);
> >>   _hidden void libxl__domain_save_device_model(libxl__egc *egc,
> >>                                        libxl__domain_save_state *dss,
> >>                                        libxl__save_device_model_cb 
> >> *callback);
> >
> >
> > .
> >
> 



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