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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 07/01/2015 06:38 PM, Ian Campbell wrote:
> 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.

Yes, for this part, I agree with you, will separate it in the next version!
thank you!

>
>>
>>>
>>> I think those changes should be two separate preparatory patches.
>>>
>>>
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> ---
>>>>    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);
>>>
>>>
>>> .
>>>
>>
>
>
> .
>

-- 
Thanks,
Yang.