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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-04-17 at 18:03 +0100, Ian Jackson wrote:
> Thanks for the review.
> 
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 17/24] libxl: ao: convert 
> libxl__spawn_*"):
> > Neither this patch nor the rest of the series seems to handle the long
> > running nature of xc_domain_restore, is that expected at this stage?
> 
> This is a thorny problem.
> 
> > We discussed a similar thing in the context of xc_domain_save, and I
> > expect the required scaffolding (bumping an op over into a thread) will
> > be the same on both sides?
> 
> Yes.
> 
> The thread's activities will have to be severely restricted but I
> think that will be doable.
> 
> 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?

> > >  typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void 
> > > *priv);
> > > -int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config 
> > > *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
> > > -int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config 
> > > *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int 
> > > restore_fd);
> > > +  /* fixme-ao   Need to review this API.  If we keep it, the reentrancy
> > > +   * properties need to be documented but they may turn out to be too
> > > +   * awkward */
> > 
> > The reentrancy concerns here relate to the "cb" or to something else?
> 
> To the cb.  This is in fact fixed later in the series.

Great!

> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > @@ -635,13 +667,13 @@ static int do_domain_create(libxl__gc *gc,
> > > libxl_domain_config *d_config,
> > >          libxl_device_vkb_add(ctx, domid, &vkb);
> > >          libxl_device_vkb_dispose(&vkb);
> > > [...]
> > > +        dcs->dmss.guest_domid = domid;
> > 
> > dcs->dmss is part of a union with dcs->sdss. I'd rather this was done
> > inside the if once we've committed to one or the other, IYSWIM.
> > 
> > Actually the hunk above (which I've trimmed already, sorry) also
> > initialised dmss unconditionally.
> > 
> > (does sdss really not need guest_domid somewhere too? odd)
> 
> 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.

> > > +    rc = libxl__ev_xswatch_register(gc, &ss->xswatch,
> > > +                                    spawn_watch_event, ss->xspath);
> > 
> > IIRC xspath is optional, does libxl__ev_xswatch_register DTWT with NULL?
> 
> No, it would crash.
> 
> 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.

>   There can be no correct callers who
> don't arrange to be notified by their demonic child that it has
> started up, and the only current callers use xenstore for this.

Given that the callers and the code agree and the comment disagrees I
think you could just nuke that comment (or replace it with one requiring
non-NULL).

> > > +/* Stubdoms. */
> > > +
> > > +typedef struct {
> > > +    /* mixed - user must fill in public parts only: */
> > > +    libxl__dm_spawn_state stubdom; /* will always remain the first 
> > > member */
> > 
> > why? If you are playing casting tricks then you could use CONTAINER_OF.
> 
> See above.
> 
> > > +_hidden void libxl__spawn_stubdom(libxl__egc *egc, 
> > > libxl__stubdom_spawn_state*);
> > > +
> > 
> > Ah, I see now what local_dm meant, it's the non-stubdom DM.
> > 
> > stubdom is a bit of an overloaded term (we have grub stubdoms, xenstore
> > stubdoms etc too). stub_dm would be better.
> 
> 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...

> > >  /*
> > >   * Convenience macros.
> > >   */
> > 
> > Right, back to the top to start on the implementation...
> 
> There's no requirement to quote the patch in the order it appeared.
> You can rearrange in your email...

Yeah, I was just lazy ;-)

Ian.



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