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 ("[PATCH v3 17/28] tools/libxl: Infrastructure for reading 
a libxl migration v2 stream"):
...
> + * PHASE_NORMAL:
> + *   This phase is used for regular migration or resume from file.
> + *   Records are read one at time and immediately processed.  (The
> + *   record queue will not contain more than a single record.)

I see that you added better comments about the record state later in
the series, thanks.

I'm not entirely sure that it was right for that to be added later,
but I'm not going to quibble about its location in the series.  I am
however going to comment on the content:

 * Internal states:
 *                  running  in_checkpoint  phase
 * Undefined          -        -              -
 * Idle               false    false          -
 * Active             true     false          normal
 * Checkpoint(buf)    true     true           buffering
 * Checkpoint(unbuf)  true     true           unbuffering
 * Active             true     false          normal
 * Finished           false    false          -

The `Active' line here appears twice.  Surely some mistake.

What is the difference between Finished and Idle ?  I think none ?  So
Finished doesn't belong here.

Also you should probably explictly say that Checkpoint is a variant of
Active, from the outside.  (You don't explicitly list the outside
states, but as I say I think they are the usual Undefined / Idle /
Active.)

The use of `-' for `undefined value' is not proper, I think.  You
probably cribbed this from the `-' in one cell in the `spawn
implementation' comment in libxl_exec.c (near line 242).  But that `-'
means `no such object is allocated but the pointer has an undefined
value', which is rather special.

Maybe you want this instead:
 
 * Internal states:
 *             phase        running  in_checkpoint
 * Undefined   undef        undef    undef
 * Idle        undef        false    false
 * Active      NORMAL       true     false
 * Active      BUFFERING    true     true
 * Active      UNBUFFERING  true     true

I also asked for you to write down which state variables may take
which values in which state.  running and in_checkpoint are only part
of this.

For example, there is also incoming_record, and record_queue (which
may be undefined, empty, one entry, >=2 entries), and the datacopier.

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

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.


> +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 ?


Last time I wrote:

  > +    libxl__sr_record_buf *incoming_record;
  > +    LIBXL_STAILQ_HEAD(, libxl__sr_record_buf) record_queue;

  This comes from malloc, not from the gc.  That needs a comment.  (Is
  the same true of incoming_record ?  I think it is.)

I don't see that comment.  You have moved incoming_record away
record_queue so now it needs two comments.


Thanks,
Ian.

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