Re: [PATCH v2 18/27] tools/libxl: Convert a legacy stream if needed

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Andrew Cooper writes ("[PATCH v2 18/27] tools/libxl: Convert a legacy stream if needed"):
> For backwards compatibility, a legacy stream needs converting before it can be
> read by the v2 stream logic.
> 
> This causes the v2 stream logic to need to juggle two parallel tasks.
> check_stream_finished() is introduced for the purpose of joining the tasks in
> both success and error cases.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_internal.h    |    7 +++
>  tools/libxl/libxl_stream_read.c |   98 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 68e7f02..1cf1884 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3274,6 +3274,7 @@ struct libxl__stream_read_state {
>      /* filled by the user */
>      libxl__ao *ao;
>      int fd;
> +    bool legacy;
>      void (*completion_callback)(libxl__egc *egc,
>                                  libxl__stream_read_state *srs,
>                                  int rc);
> @@ -3281,6 +3282,12 @@ struct libxl__stream_read_state {
>      int rc;
>      bool running;
>  
> +    /* Active-stuff handling */
> +    int joined_rc;
> +
> +    /* Conversion helper */
> +    libxl__carefd *v2_carefd;

This needs proper documentation of what states this is valid in.  See
my observations on 16/ about this.  I would expect that this patch
would add appropriate commentary documenting the
invariants/states/whatever for these.

> +static void check_stream_finished(libxl__egc *egc,
> +                                  libxl__stream_read_state *stream,
> +                                  int rc, const char *what)
> +{
> +    libxl__domain_create_state *dcs = CONTAINER_OF(stream, *dcs, srs);
> +    STATE_AO_GC(stream->ao);
> +
> +    LOG(DEBUG, "Task '%s' joining (rc %d)", what, rc);
> +
> +    if (rc && !stream->joined_rc) {
> +        bool skip = false;
> +        /* First reported failure from joining tasks.  Tear everything down */
> +        stream->joined_rc = rc;

This function has room for improvement, I think.

* You compute
     libxl__stream_read_inuse(&dcs->srs) ||
     libxl__convert_legacy_stream_inuse(&dcs->chs)
  twice, once in the if (rc ...) and once in the straight line.
  You could combine these.

* I think libxl__blah_abort are all no-ops on initialised but inactive
  objects.  (Ie, objects in state Idle.)  So you can call them
  unconditionally.

* I don't think joined_rc is particularly helpful.  Why not simply
  combine the incoming rc with the existing rc ?  That is, if nothing
  has gone wrong already, set the rc to the incoming one; otherwise
  keep the existing rc.  That assumes that the best rc value to report
  is the one from the first detected problem, which is probably
  correct.  (Consider ERROR_ABORTED.)
  

> +#if defined(__x86_64__) || defined(__i386__)
> +static void conversion_done(libxl__egc *egc,
> +                            libxl__conversion_helper_state *chs, int rc)
> +{
> +    STATE_AO_GC(chs->ao);
> +    libxl__domain_create_state *dcs = CONTAINER_OF(chs, *dcs, chs);
> +
> +    check_stream_finished(egc, &dcs->srs, rc, "conversion");
> +}
> +#endif

Again, I would prefer to avoid the ifdeffery if it's not terribly
awkward to do the other way (see libxl_no*.c for some examples).  If
you do invent ifdeffery it should definitely have its own #define to
trigger off, rather than directly using __x86_64__ etc.

Thanks,
Ian.