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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH v3 17/28] tools/libxl: Infrastructure for 
reading a libxl migration v2 stream"):
> On 13/07/15 15:31, Ian Jackson wrote:
> > For example, there is also incoming_record, and record_queue (which
> > may be undefined, empty, one entry, >=2 entries), and the datacopier.
> 
> These cannot be expressed in the above terms.
> 
> incoming_record (as already noted in libxl_internal.h) is only used
> while reading a record, which happens in both Active/NORMAL and
> Active/BUFFERING
> 
> record_queue is strictly 0 or 1 entries during NORMAL, grows from 0 to
> >=1 during BUFFERING, and shrinks back to 0 during UNBUFFERING.
> 
> How would you go about expressing this without a state transition
> diagram? (I really don't think an ascii art state transition diagram
> will help make this any clearer)

  *          phase     running   in_         record   incoming
  *                              checkpoint  _queue   _record
  *
  * Undefined undef       undef    undef       undef    undef
  * Idle      undef       false    false       0        0
  * Active    NORMAL      true     false       0/1      0/partial
  * Active    BUFFERING   true     true        any      0/partial
  * Active    UNBUFFERING true     true        any      0

Adjust to taste and to correspond to facts.


> > Is the datacopier always active ?  I think it is except
> >  - when we are processing one of our own callbacks, or
> >  - in BUFFERING/UNBUFFERING, when process_record is setting up
> >    its own callbacks
> 
> The datacopier is only active while reading a record from the stream,
> but that is only (logically) half of the work to do.  It will never be
> active when processing records.
> 
> There is a second datacopier for qemu use which is only active while
> processing an EMULATOR record.

Maybe this wants to be in the table, or maybe it wants a comment next
to the variable name in the struct.


> > Maybe you want to say in a comment that there is always _either_ a
> > datacopier callback to come, _or_ a callback set up by process_record.
> 
> ok.

I'm glad that I divined correctly that this is true :-).

> >> +static void stream_continue(libxl__egc *egc,
> >> +                            libxl__stream_read_state *stream)
> >> +{
> > ...
> >> +    switch (stream->phase) {
> >> +    case SRS_PHASE_NORMAL:
> >> +        /*
> >> +         * Normal phase (regular migration or restore from file):
> >> +         *
> >> +         * logically:
> >> +         *   do { read_record(); process_record(); } while ( not END );
> >> +         *
> >> +         * Alternate between reading a record from the stream, and
> >> +         * processing the record.  There should never be two records
> >> +         * in the queue.
> >> +         */
> >> +        if (LIBXL_STAILQ_EMPTY(&stream->record_queue))
> >> +            setup_read_record(egc, stream);
> >> +        else {
> >> +            if (process_record(egc, stream))
> >> +                setup_read_record(egc, stream);
> > 
> > Weren't you going to add an assert here, that the queue is empty ?
...
> It is in the paragraph below PHASE_NORMAL, which you have snipped.

I have grepped the whole of `stream_continue' in the tip of v3 of
your series (b8bf4572) and these are the results:

    assert(stream->recursion_guard == false);
        assert(stream->in_checkpoint);
        assert(stream->in_checkpoint);
    assert(stream->recursion_guard == true);

I meant a call to assert() which ensures that after process_record in
PHASE_NORMAL, the queue is empty.

Ian.

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