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

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-09-14 at 17:15 +0100, Wei Liu wrote:
> FYI all other patches of this series were applied by Jan. You only need
> to resend this one.

Jan,

I appreciate you do not want to do so routinely for all commits you make
but when you are partially applying a series it would be very useful if you
could drop a line to the thread (i.e. the 00/NN mail) explaining what you
have applied vs not applied. Particularly when it is ten elevenths of the
series and so easy to miss that the eleventh patch has not been included.

I was under the incorrect impression that all 11 patches had gone in so had
removed this work from my queue.

Thanks,
Ian.

> 
> 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@redhat.com>
> > ---
> > 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@redhat.com>
> > ---
> >  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.