From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v2 for-4.6 3/3] tools/libxl: Only continue stream operations if the stream is still in progress Date: Tue, 28 Jul 2015 16:59:04 +0100 Message-ID: <21943.42696.836949.936143@mariner.uk.xensource.com> References: <1438015647-25377-1-git-send-email-andrew.cooper3@citrix.com> <1438015647-25377-4-git-send-email-andrew.cooper3@citrix.com> <21943.34423.586101.713988@mariner.uk.xensource.com> <55B78A21.3070402@citrix.com> <21943.39885.704950.477219@mariner.uk.xensource.com> <55B79DEA.2040801@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55B79DEA.2040801@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: Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org Andrew Cooper writes ("Re: [PATCH v2 for-4.6 3/3] tools/libxl: Only continue stream operations if the stream is still in progress"): > On 28/07/15 16:12, Ian Jackson wrote: > > What do you think about putting the inuse check in stream_continue ? > > That would work on the stream_read side but not the stream_write side, > but is not really correct IMO. I have reread the code on the write side and I think I agree with you there. The next function to be called is write_toolstack_record so things are a lot less abstract. On the read side I looked at a lot of functions which contained: stream_continue(egc, stream); return; err: assert(rc); stream_complete(egc, stream, rc); and ISTM that if the analysis leading to these call sites for stream_continue is wrong, a similar hazard would exist. > The _inuse() check is needed because the save helper callback is not > sure whether the stream is in use or not. This is a property of the > save helper callback, rather than the stream. In this event-driven code, I prefer to have a situation where the next thing to do is obvious from the recorded state, and is calculated explicitly (see also the way that check_all_finished is now). This is much less confusing and error prone than code where the next thing to do depends on _both_ the recorded state, _and_ the call path (representing the most recent thing that happened). > Pushing the _inuse() check into the next layer would function, but it > adds extra _inuse() checks to other codepaths which should be fatal if > they failed in other contexts. I don't understand how an _inuse check in stream_continue could ever do the wrong thing. If the code is reorganised later, it seems more likely to me that there will introduced new situations where we need to do "see if we are still running, and if so do stream_continue, otherwise go to check_all_finished". Ian.