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