Re: [Xen-devel] [PATCH v8 --for 4.6 COLO 06/25] tools/libxl: write colo_context records into the stream

[Yang Hongyang <yanghy@xxxxxxxxxxxxxx>]

On 07/16/2015 01:35 AM, Andrew Cooper wrote:
> On 15/07/15 10:18, Yang Hongyang wrote:
> > write colo_context records into the stream, used by both
> > primary and secondary to send colo context.
> >
> > Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
> > Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> > ---
> >   tools/libxl/libxl_internal.h     |  5 +++
> >   tools/libxl/libxl_stream_write.c | 87 ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 92 insertions(+)
> >
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index a83d6a5..2634836 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -3000,6 +3000,7 @@ struct libxl__stream_write_state {
> >       int rc;
> >       bool running;
> >       bool in_checkpoint;
> > +    bool in_colo_context;
> >       libxl__save_helper_state shs;
> >
> >       /* Main stream-writing data. */
> > @@ -3019,6 +3020,10 @@ _hidden void libxl__stream_write_start(libxl__egc *egc,
> >   _hidden void
> >   libxl__stream_write_start_checkpoint(libxl__egc *egc,
> >                                        libxl__stream_write_state *stream);
> > +_hidden void
> > +libxl__stream_write_colo_context(libxl__egc *egc,
> > +                                 libxl__stream_write_state *stream,
> > +                                 libxl_sr_colo_context *colo_context);
> >   _hidden void libxl__stream_write_abort(libxl__egc *egc,
> >                                          libxl__stream_write_state *stream,
> >                                          int rc);
> > diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
> > index df55277..e7a32c4 100644
> > --- a/tools/libxl/libxl_stream_write.c
> > +++ b/tools/libxl/libxl_stream_write.c
> > @@ -96,6 +96,16 @@ static void write_checkpoint_end_record(libxl__egc *egc,
> >   static void checkpoint_end_record_done(libxl__egc *egc,
> >                                          libxl__stream_write_state *stream);
> >
> > +/* COLO context */
> > +static void write_colo_context(libxl__egc *egc,
> > +                               libxl__stream_write_state *stream,
> > +                               libxl_sr_colo_context *colo_context);
> > +static void write_colo_context_done(libxl__egc *egc,
> > +                                    libxl__datacopier_state *dc,
> > +                                    int rc, int onwrite, int errnoval);
> > +static void colo_context_done(libxl__egc *egc,
> > +                              libxl__stream_write_state *stream, int rc);
> > +
> >   /*----- Helpers -----*/
> >
> >   static void write_done(libxl__egc *egc,
> > @@ -500,6 +510,11 @@ static void stream_complete(libxl__egc *egc,
> >           return;
> >       }
> >
> > +    if (stream->in_colo_context) {
> > +        colo_context_done(egc, stream, rc);
> > +        return;
> > +    }
> Please follow the same style as stream->in_checkpoint, asserting(rc) and
> explaining how the error comes back around.
>
> > +
> >       if (!stream->rc)
> >           stream->rc = rc;
> >       stream_done(egc, stream);
> > @@ -555,6 +570,78 @@ static void check_all_finished(libxl__egc *egc,
> >       stream->completion_callback(egc, stream, stream->rc);
> >   }
> >
> > +/*----- COLO context -----*/
> > +void libxl__stream_write_colo_context(libxl__egc *egc,
> > +                                      libxl__stream_write_state *stream,
> > +                                      libxl_sr_colo_context *colo_context)
> > +{
> > +    assert(stream->running);
> > +    assert(!stream->in_checkpoint);
> > +    assert(!stream->in_colo_context);
> > +    stream->in_colo_context = true;
> > +
> > +    write_colo_context(egc, stream, colo_context);
> Use setup_write() here, which will remove most of the rest of this
> patch, and cover all your error handling for your.
setup_write() was introduced from your v4 series I think? so I missed
that part when rebasing...

> You want a small pair of functions such as the write_checkpoint_end()
> pair.  See write_toolstack_record() for an example using setup_write()
> with a body.
>
> Also, to preempt Ian Jacksons review, use FILLZERO() over "= { ... }"
Ok, thank you for the kindly explaination, will address this in the
next version.

> ~Andrew
> .
--
Thanks,
Yang.

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