From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream Date: Mon, 13 Jul 2015 16:33:54 +0100 Message-ID: <21923.55906.835814.413355@mariner.uk.xensource.com> References: <1436788907-1921-1-git-send-email-andrew.cooper3@citrix.com> <1436788907-1921-18-git-send-email-andrew.cooper3@citrix.com> <21923.52187.738764.317319@mariner.uk.xensource.com> <55A3D0DE.3070905@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A3D0DE.3070905@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: Ross Lagerwall , Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org 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.