Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

[Wei Liu <wei.liu2@xxxxxxxxxx>]

Sorry for the delay.

FYI all other patches of this series were applied by Jan. You only need
to resend this one.

See below for a few comments.

On Fri, Sep 04, 2015 at 03:39:51PM +0200, Vitaly Kuznetsov wrote:
> Use existing create/restore path to perform 'soft reset' for HVM
> domains. Tear everything down, e.g. destroy domain's device model,
> remove the domain from xenstore, save toolstack record and start
> over.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> Changes since v10:
> - Adapt to 'migration v2' changes.
> - Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
>   LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
>   we connect consoles to both files on create.
> - Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
>  docs/man/xl.cfg.pod.5        |   8 +-
>  tools/libxl/libxl.c          |  22 ++++-
>  tools/libxl/libxl.h          |  15 ++++
>  tools/libxl/libxl_create.c   | 192 
> ++++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl_internal.h |   4 +
>  tools/libxl/libxl_types.idl  |   2 +
>  tools/libxl/xl.h             |   1 +
>  tools/libxl/xl_cmdimpl.c     |  25 +++++-
>  8 files changed, 242 insertions(+), 27 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index c6345b8..d8c4186 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -349,6 +349,12 @@ destroy the domain.
>  write a "coredump" of the domain to F</var/lib/xen/dump/NAME> and then
>  restart the domain.
>  
> +=item B<soft-reset>
> +
> +Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
> +it to reestablish these interfaces and continue executing the domain. PV
> +guest is not supported.
> +

And "non-Xen-aware HVM will crash" ?  If there is no definite answer to
guest state maybe just saying "PV guest and non-Xen-aware HVM guests are
not supported" ?

It's important to let user know about the consequence because libxl
doesn't actually stop you from soft-resetting a HVM guest that is not
Xen-aware.

[...]
> +static int do_domain_soft_reset(libxl_ctx *ctx,
> +                                libxl_domain_config *d_config,
> +                                uint32_t domid_soft_reset,
> +                                const libxl_asyncop_how *ao_how,
> +                                const libxl_asyncprogress_how
> +                                *aop_console_how)
> +{
> +    AO_CREATE(ctx, 0, ao_how);
> +    libxl__domain_soft_reset_state *srs;
> +    libxl__app_domain_create_state *cdcs;
> +    libxl__domain_create_state *dcs;
> +    libxl__domain_build_state *state;
> +    libxl__domain_suspend_state *dss;
> +    char *dom_path, *xs_store_mfn, *xs_console_mfn;
> +    uint32_t domid_out;
> +    int rc;
> +
> +    GCNEW(srs);
> +    cdcs = &srs->cdcs;
> +    dcs = &cdcs->dcs;
> +    state = &dcs->build_state;
> +    dss = &srs->dss;
> +
> +    srs->cdcs.dcs.ao = ao;
> +    srs->cdcs.dcs.guest_config = d_config;
> +    libxl_domain_config_init(&srs->cdcs.dcs.guest_config_saved);
> +    libxl_domain_config_copy(ctx, &srs->cdcs.dcs.guest_config_saved,
> +                             d_config);
> +    cdcs->dcs.restore_fd = -1;
> +    cdcs->dcs.domid_soft_reset = domid_soft_reset;
> +    cdcs->dcs.callback = domain_create_cb;
> +    libxl__ao_progress_gethow(&srs->cdcs.dcs.aop_console_how,
> +                              aop_console_how);
> +    cdcs->domid_out = &domid_out;
> +
> +    dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
> +    if (!dom_path) {
> +        LOG(ERROR, "failed to read domain path");
> +        return AO_CREATE_FAIL(ERROR_FAIL);

Sorry for not noticing this earlier. My bad and apologise.

This should be

       if (!dom_path) {
           LOG(...);
           rc = ERROR_FAIL;
           goto out;
       }

I.e. please use goto style error handling.

> +    }
> +
> +    xs_store_mfn = xs_read(ctx->xsh, XBT_NULL,
> +                           GCSPRINTF("%s/store/ring-ref", dom_path),
> +                           NULL);
> +    state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
> +    free(xs_store_mfn);
> +
> +    xs_console_mfn = xs_read(ctx->xsh, XBT_NULL,
> +                             GCSPRINTF("%s/console/ring-ref", dom_path),
> +                             NULL);
> +    state->console_mfn = xs_console_mfn ? atol(xs_console_mfn): 0;
> +    free(xs_console_mfn);
> +
> +    dss->ao = ao;
> +    dss->domid = domid_soft_reset;
> +    dss->dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
> +                                 domid_soft_reset);
> +
> +    rc = libxl__save_emulator_xenstore_data(dss, &srs->toolstack_buf,
> +                                            &srs->toolstack_len);
> +    if (rc) {
> +        LOG(ERROR, "failed to save toolstack record.");
> +        return AO_CREATE_FAIL(ERROR_FAIL);

Ditto, please use goto style error handling.

Furthermore please preserve rc from libxl__save_emulator_xenstore_data
instead of rewriting it with ERROR_FAIL.


> +    }
> +
> +    rc = libxl__domain_suspend_device_model(gc, dss);
> +    if (rc) {
> +        LOG(ERROR, "failed to suspend device model.");
> +        return AO_CREATE_FAIL(ERROR_FAIL);

Same here.

> +    }
> +
> +    /*
> +     * Ask all backends to disconnect by removing the domain from
> +     * xenstore. On the creation path the domain will be introduced to
> +     * xenstore again with probably different store/console/...
> +     * channels.
> +     */
> +    xs_release_domain(ctx->xsh, cdcs->dcs.domid_soft_reset);
> +
> +    srs->dds.ao = ao;
> +    srs->dds.domid = domid_soft_reset;
> +    srs->dds.callback = domain_soft_reset_cb;
> +    srs->dds.soft_reset = true;
> +    libxl__domain_destroy(egc, &srs->dds);
> +
> +    return AO_INPROGRESS;

out:
      return AO_CREATE_FAIL(rc);

> +}
> +

Wei.

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