Re: [Xen-devel] [PATCH 17/24] libxl: ao: convert libxl__spawn_*

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-04-18 at 11:52 +0100, Ian Jackson wrote:
> 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).

This sounds sensible to me.

Ian.

> 
> > > 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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel