Re: [PATCH v2 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-10 at 11:47 +0100, Andrew Cooper wrote:

> >> +void libxl__stream_read_start(libxl__egc *egc,
> >> +                              libxl__stream_read_state *stream)
> >> +{
> >> +    libxl__datacopier_state *dc = &stream->dc;
> >> +    int ret = 0;
> >> +
> >> +    /* State initialisation. */
> >> +    assert(!stream->running);
> > Since running is declared private and there is no _init function (I
> > think _start is effectively filling that role) I'm not sure that the
> > caller can necessarily be expected to have initialised anything other
> > than the ao, fd and callback fields.
> 
> It was a sanity check that _start() doesn't get called twice (guess what
> I managed to do while developing).  It can probably be dropped.
> 
> >
> > You might choose to handle this as a request for a doc comment ("must
> > call LIBXL_FILLZERO on it to init"), or to add a separate init function
> > containing the memset or to do away with this check. I've not gotten to
> > the caller yet so I don't know which you will prefer.
> 
> It is all zeroed because of the way dcs is constructed.  I suppose I can
> also drop the zeroing of the dc.

Got it. I think a comment that libxl__stream_read_state should be init'd
to zero would still be worthwhile, even if the actual implementation of
that happens implicitly through its containing type.

Then your sanity check could legitimately remain.

> >
> >> +
> >> +    stream->completion_callback(egc, stream, stream->rc);
> >> +}
> >> +
> >> +static void stream_continue(libxl__egc *egc,
> >> +                            libxl__stream_read_state *stream)
> >> +{
> >> +    STATE_AO_GC(stream->ao);
> >> +
> >> +    /* Must not mutually recurse with process_record() */
> >> +    assert(stream->recursion_guard == false);
> >> +    stream->recursion_guard = true;
> > This smells a bit like it ought to be a SRS_PHASE_PROCESSING or some
> > such, but lets leave that alone...
> 
> This check is pre-emptively avoid the naive bug which would occur if
> process_record() called back into stream_continue() and there were many
> TOOLSTACK records back to back in the processing queue.
> 
> In that case (and potentially future records as well), the two functions
> would mutually recurse based on the contents of the stream.

Do you mean they would do so legitimately in that case, or in error?

> >> +        goto err;
> >> +    }
> >> +
> >> +    hdr->ident   = be64toh(hdr->ident);
> >> +    hdr->version = be32toh(hdr->version);
> >> +    hdr->options = be32toh(hdr->options);
> >> +
> >> +    if (hdr->ident != RESTORE_STREAM_IDENT) {
> >> +        ret = ERROR_FAIL;
> > Eventually I suspect the xapi people would like to see something more
> > specific at least for the general "SRS header fail" if not the
> > individual reasons.
> 
> If you don't object too strongly, I would prefer to leave that
> bikeshedding to the error value improvements work.

I don't mind, it's easy enough to change from ERROR_FAIL to something
more specific.

You might incur the wrath of Rob/Euan though ;-)

> >
> > _If_ you've compile tested this for both 32- and 64-bit and it works we
> > could perhaps leave that audit until later.
> >
> >> +static void setup_read_record(libxl__egc *egc,
> >> +                              libxl__stream_read_state *stream)
> >> +{
> >> +    STATE_AO_GC(stream->ao);
> >> +    libxl__sr_record_buf *rec = NULL;
> >> +    int ret;
> >> +
> >> +    assert(stream->incoming_record == NULL);
> >> +
> >> +    stream->incoming_record = rec = libxl__zalloc(NOGC, sizeof(*rec));
> > I recall Ian J and you discussing NOGC allocations on IRC. Was the
> > conclusion that it was OK, or that it could be fixed later, or that it
> > should be fixed now via an nested ao or something similar?
> >
> > Unless the answer is "fixed now" I think the reason for the NOGC should
> > be in either the commit log or a comment (in the header, around about
> > the definition of the allocated data structure).
> 
> I will add a note about in the commit message.  We agreed on IRC that
> NOGC was OK.  It might be possible to switch to some nested ao later,
> but that depends entirely on the COLO work.

ACK. Please remember to say why it is ok, not just that it is.

Ian.