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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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