Re: [Xen-devel] [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/07/15 14:42, Ian Campbell wrote:
> On Mon, 2015-07-13 at 13:01 +0100, Andrew Cooper wrote:
>
>> +void libxl__stream_read_start(libxl__egc *egc,
>> +                              libxl__stream_read_state *stream)
>> +{
>> +    libxl__datacopier_state *dc = &stream->dc;
>> +    int rc = 0;
>> +
>> +    stream->running = true;
> Did you drop the assert prior to this on purpose?

I think it got lost while splitting the _init() out.  I shall re-introduce.

>
>> +
>> +static void stream_header_done(libxl__egc *egc,
>> +                               libxl__datacopier_state *dc,
>> +                               int rc, int onwrite, int errnoval)
>> +{
>> +    libxl__stream_read_state *stream = CONTAINER_OF(dc, *stream, dc);
>> +    libxl__sr_hdr *hdr = &stream->hdr;
>> +    STATE_AO_GC(dc->ao);
>> +
>> +    if (rc || onwrite || errnoval) {
>> +        rc = ERROR_FAIL;
> Sorry for not noticing this before but if we come here because of rc
> then this clobbers the existing value, which I think is undesirable.
>
> So I think the "if (rc) goto err" should be pulled out of this check.
> (the one line form is acceptable in this case).
>
> This no doubt applies to most of these functions.

Urgh yes - I fixed this in some places (mostly in the write side
infrastructure) but forgot to check for other occurrences.

>
>> +static void stream_continue(libxl__egc *egc,
>> +                            libxl__stream_read_state *stream)
>> +{
>> +    STATE_AO_GC(stream->ao);
>> +
>> +    /*
>> +     * Must not mutually recurse with process_record().
>> +     *
>> +     * For records whose processing function is synchronous
>> +     * (e.g. TOOLSTACK), process_record() does not start another async
>> +     * operation, and a further operation should be started.
>> +     *
>> +     * A naive solution, which would function in general, would be for
>> +     * process_record() to call stream_continue().
> Wouldn't the more obvious naive thing to do be to call
> setup_read_record?

That would also be a naive thing to do.  During a remus checkpoint, by
the time we are processing TOOLSTACK records, the next thing in the pipe
is a libxc record.

This is why everything must come back around to stream_continue() rather
than attempting to be clever.

> That's also potentially recursing I think since the
> chain of callbacks may also end in a stream_continue.

I am fairly sure the reentrancy guarantees prevent that.

>
>>   However, this
>> +     * would allow the content of the stream to cause mutual
>> +     * recursion, and possibly for us to fall off our stack.
>> +     *
>> +     * Instead, process_record() indicates with its return value
>> +     * whether it a further operation needs to start, and the
> "whether it"? 

s/ it//

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel