From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 17/24] libxl: ao: convert libxl__spawn_* Date: Wed, 18 Apr 2012 11:52:39 +0100 Message-ID: <20366.40183.410388.447630@mariner.uk.xensource.com> References: <1334596686-8479-1-git-send-email-ian.jackson@eu.citrix.com> <1334596686-8479-18-git-send-email-ian.jackson@eu.citrix.com> <1334675843.23948.102.camel@zakaz.uk.xensource.com> <20365.41550.838083.825783@mariner.uk.xensource.com> <1334743970.23948.193.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1334743970.23948.193.camel@zakaz.uk.xensource.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: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Campbell writes ("Re: [Xen-devel] [PATCH 17/24] libxl: ao: convert libxl__spawn_*"): > On Tue, 2012-04-17 at 18:03 +0100, Ian Jackson wrote: > > We should consider another alternative: spawning a copy of xc_save, > > like xend does. > > I wondered about that too. Does it make things like error handling or > reporting any more difficult? With threading it's quite tricky. I think it would be rude to call even the application's logging callbacks on a private thread. Certainly we can't easily propagate errors from that private thread. We would have to arrange somehow to transfer the results to one of the application's threads, to avoid possibly calling back to the application on our private thread (which is a big no-no because it might subject a non-multithreaded application to reentrancy). And even with all that there are vexed problems like the possibility of generating an application-wide SIGPIPE on the migration fd. So I think it would make it easier, now that we have all the machinery in place, to spawn a separate process. That process should exec, even if the thing exec'd is a utility mostly using libxl, firstly to save memory (in case we're talking about a huge toolstack daemon) and also so we don't have to worry about nonconsensually generating long-running non-execing forks of the application (which would require us to provide a callback mirror of the postfork noexec downcall). > > libxl__stubdom_spawn_state is a struct containing a > > libxl__dm_spawn_state ("stubdom") as its first member. So the sdss's > > domid is in sdss.stubdom.guest_domid, and because stubdom is first > > this is the same as dmss->guest_domid. > > > > Maybe this needs a bigger comment ? > > At the very least, yes. > > Does combining thing in a union in this way really make some other bit > of code look substantially better? Not having the union and having a > shared libxl__dm_spawn_state in the parent struct seems more straight > forward at least from here. I will see if there is a better way to do this, perhaps without the union. And try to add more comments. > > xspath is no longer optional. > > So this comment on libxl__spawn_spawn is wrong? > > + * The inner child must soon exit or exec. If must also soon exit or > > + * notify the parent of its successful startup by writing to the > > + * xenstore path xspath (or by other means). xspath may be 0 to > > + * indicate that only other means are being used. Yes. It's out of date. We don't have any processes which do this other than with xenstore. If we introduce them we can make xspath maybe be 0. I've deleted the erroneous comment (and fixed the preceding sentence). > > Do you think I should s/stubdom/stubdm/ or something ? > > yes, or stub_dm for consistency with local_dm. I'd like to eradicate the > bare use of the term "stubdom" since it can mean several things > (admittedly not really in this context). I suspect I am fighting against > the tide here though... I see your point and will see what I can do. Ian.