From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v2 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream Date: Fri, 10 Jul 2015 14:09:12 +0100 Message-ID: <21919.50168.907707.872191@mariner.uk.xensource.com> References: <1436466413-25867-1-git-send-email-andrew.cooper3@citrix.com> <1436466413-25867-17-git-send-email-andrew.cooper3@citrix.com> <21919.47036.210181.374687@mariner.uk.xensource.com> <559FC105.9030506@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559FC105.9030506@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 v2 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream"): ... > > This function has one call site. As an alternative to abolishing > > stream_failed, maybe stream_failed should be renamed to > > `stream_complete' and then the call site for stream_success could > > simply say `stream_complete(egc, stream, 0)' ? > > I have to admit that I find that paragdim specifically confusing to > read, which is why I deliberately split the _success() and _failed() > cases. I'm not sure why it's confusing. It means you have a unified exit path. > >> +static void stream_failed(libxl__egc *egc, > >> + libxl__stream_read_state *stream, int rc) > >> +{ ... > >> + if (stream->running) { > >> + stream_done(egc, stream); > > Is the check for running necessary here ? > > Yes. An _abort() call from outside can happen at any arbitrary time. > We must not deliver the callback twice. Obviously. But it is surely a serious bug if stream_failed is called when the stream is not running, because stream_failed is then in flight in circumstances where the caller might reuse the libxl__stream_read_state. > >> + sprintf(path, XC_DEVICE_MODEL_RESTORE_FILE".%u", dcs->guest_domid); > >> + > >> + libxl__carefd_begin(); > >> + writefd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600); > > Does this need to be a carefd ? This is going to be a small amount of > > data, and the worst that happens is some other process inherits an fd > > onto a file which might otherwise be deleted sooner. > > Who says the file will be deleted? That will be down to the exact > semantics of the device model. If the file is not going to be deleted then it is even less necessary. > (I was asked to switch from a plain open() to a carefd as part of review > from v1). I tried to find this and all I found was Ian C saying: Also, should consider whether this fd needs to be subject to the carefd machinery. I think it doesn't. But overuse of the carefd machinery is just pointless and not actually incorrect. So: > > If it does, then please use libxl__carefd_opened. (Best would be to > > libxl__carefd_opened to save and restore errno and then you can call > > it unconditionally after the open.) > > I will go down this route. ... if you do this it won't be a blocker. Ian.