Re: [Xen-devel] [PATCH v11 14/27] secondary vm suspend/resume/checkpoint code

[Changlong Xie <xiecl.fnst@xxxxxxxxxxxxxx>]

On 03/05/2016 01:11 AM, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 14/27] secondary vm suspend/resume/checkpoint 
> code"):
> > From: Wen Congyang <wency@xxxxxxxxxxxxxx>
> >
> > Secondary vm is running in colo mode. So we will do
> > the following things again and again:
> I don't propose to review this in detail.  Skimreading it, it looks
> plausible.  I don't think a detailed review is needed.
>
> I will review the changes to the core code.
>
> > diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
> > index 5d9f497..2bfed64 100644
> > --- a/tools/libxc/xc_sr_common.h
> > +++ b/tools/libxc/xc_sr_common.h
> > @@ -184,10 +184,12 @@ struct xc_sr_context
> >        * migration stream
> >        * 0: Plain VM
> >        * 1: Remus
> > +     * 2: COLO
> >        */
> >       enum {
> >           MIG_STREAM_NONE, /* plain stream */
> >           MIG_STREAM_REMUS,
> > +        MIG_STREAM_COLO,
> I think this shows that the duplicated list (in the comment, above the
> enum) is a mistake.  I would prefer it to be removed.
We've sent out a patch to fix this issue first and wonder it can be 
merged, so we can rebase to it.
http://www.gossamer-threads.com/lists/xen/devel/423093

Thanks
        -Xie

> > +/* ===================== colo: common functions ===================== */
> > +static void colo_enable_logdirty(libxl__colo_restore_state *crs, libxl__egc 
> > *egc)
> Here's another missing blank line.  This seems to be a general theme:
> can you change this everywhere ?  Thanks.
>
> > @@ -994,6 +1011,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
> >       const int restore_fd = dcs->restore_fd;
> >       libxl__domain_build_state *const state = &dcs->build_state;
> >       const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
> > +    libxl__colo_restore_state *const crs = &dcs->crs;
> > +    libxl_domain_build_info *const info = &d_config->b_info;
> >
> >       if (rc) {
> >           domcreate_rebuild_done(egc, dcs, rc);
> > @@ -1022,6 +1041,13 @@ static void domcreate_bootloader_done(libxl__egc *egc,
> >
> >       /* Restore */
> >
> > +    /* COLO only supports HVM now */
> > +    if (info->type != LIBXL_DOMAIN_TYPE_HVM &&
> > +        checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) {
> > +        rc = ERROR_FAIL;
> > +        goto out;
> Please log something here, or it may be very mysterious.
>
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 6307b71..48b4858 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -87,6 +87,8 @@
> > @@ -3468,7 +3464,6 @@ libxl__stream_read_inuse(const libxl__stream_read_state 
> > *stream)
> >       return stream->running;
> >   }
> >
> > -
> >   struct libxl__domain_create_state {
> >       /* filled in by user */
> >       libxl__ao *ao;
> > @@ -3484,6 +3479,8 @@ struct libxl__domain_create_state {
> Unintentional whitespace change.
>
>
> Ian.
>
>
> .


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