Re: [PATCH v8 --for 4.6 COLO 08/25] tools/libxl: handle colo_context records in a libxl migration v2 read stream

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

On 15/07/15 10:18, Yang Hongyang wrote:
> Read a colo_context and call stream->checkpoint_callback to handle it.
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  tools/libxl/libxl_internal.h    |  3 +++
>  tools/libxl/libxl_stream_read.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 05cee04..1be2a4a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3369,6 +3369,7 @@ struct libxl__stream_read_state {
>      int rc;
>      bool running;
>      bool in_checkpoint;
> +    bool in_colo_context;
>      libxl__save_helper_state shs;
>      libxl__conversion_helper_state chs;
>  
> @@ -3396,6 +3397,8 @@ _hidden void libxl__stream_read_start(libxl__egc *egc,
>                                        libxl__stream_read_state *stream);
>  _hidden void libxl__stream_read_start_checkpoint(libxl__egc *egc,
>                                                   libxl__stream_read_state *stream);
> +_hidden void libxl__stream_read_colo_context(libxl__egc *egc,
> +                                             libxl__stream_read_state *stream);
>  _hidden void libxl__stream_read_abort(libxl__egc *egc,
>                                        libxl__stream_read_state *stream, int rc);
>  static inline bool
> diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
> index b924f05..ab47251 100644
> --- a/tools/libxl/libxl_stream_read.c
> +++ b/tools/libxl/libxl_stream_read.c
> @@ -152,6 +152,13 @@ static void write_emulator_done(libxl__egc *egc,
>                                  libxl__datacopier_state *dc,
>                                  int rc, int onwrite, int errnoval);
>  
> +/* Handlers for colo context mini-loop */
> +static void handle_colo_context(libxl__egc *egc,
> +                                libxl__stream_read_state *stream,
> +                                libxl__sr_record_buf *rec);
> +static void colo_context_done(libxl__egc *egc,
> +                              libxl__stream_read_state *stream, int rc);
> +
>  /*----- Helpers -----*/
>  
>  /* Helper to set up reading some data from the stream. */
> @@ -569,6 +576,15 @@ static bool process_record(libxl__egc *egc,
>          checkpoint_done(egc, stream, 0);
>          break;
>  
> +    case REC_TYPE_COLO_CONTEXT:
> +        if (!stream->in_colo_context) {
> +            LOG(ERROR, "Unexpected COLO_CONTEXT record in stream");
> +            rc = ERROR_FAIL;
> +            goto err;
> +        }
> +        handle_colo_context(egc, stream, rec);
> +        break;
> +
>      default:
>          LOG(ERROR, "Unrecognised record 0x%08x", rec->hdr.type);
>          rc = ERROR_FAIL;
> @@ -678,6 +694,11 @@ static void stream_complete(libxl__egc *egc,
>          return;
>      }
>  
> +    if (stream->in_colo_context) {
> +        colo_context_done(egc, stream, rc);
> +        return;
> +    }
> +
>      if (!stream->rc)
>          stream->rc = rc;
>      stream_done(egc, stream);
> @@ -794,6 +815,36 @@ static void check_all_finished(libxl__egc *egc,
>      stream->completion_callback(egc, stream, stream->rc);
>  }
>  
> +/*----- COLO context handlers -----*/
> +
> +void libxl__stream_read_colo_context(libxl__egc *egc,
> +                                     libxl__stream_read_state *stream)

A name like this makes the erroneous assumption that a COLO\_CONTEXT
record is what is going to be found next in the stream.

Where and when is a COLO\_CONTEXT record expected, and is it only in the
backchannel?

> +{
> +    assert(stream->running);
> +    assert(!stream->in_checkpoint);
> +    assert(!stream->in_colo_context);
> +    stream->in_colo_context = true;
> +
> +    setup_read_record(egc, stream);
> +}
> +
> +static void handle_colo_context(libxl__egc *egc,
> +                                libxl__stream_read_state *stream,
> +                                libxl__sr_record_buf *rec)
> +{
> +    libxl_sr_colo_context *colo_context = rec->body;
> +
> +    colo_context_done(egc, stream, colo_context->id);

A handler this trivial should just be done in the switch statement in
process_record().  No need for its own function for a single forward call.

~Andrew