From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v2 18/27] tools/libxl: Convert a legacy stream if needed Date: Fri, 10 Jul 2015 13:41:44 +0100 Message-ID: <21919.48520.229623.377892@mariner.uk.xensource.com> References: <1436466413-25867-1-git-send-email-andrew.cooper3@citrix.com> <1436466413-25867-19-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436466413-25867-19-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > --- > 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.