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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  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.

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 "= { ... }"

~Andrew