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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> On Mon, 2012-04-16 at 18:17 +0100, Ian Jackson wrote:
> > libxl__spawn_spawn becomes a callback-style asynchronous function.
> > The implementation is now in terms of libxl__ev_* including
> > libxl_ev_child.
> 
> (nb: I actually skipped ahead and reviewed the libxl_internal.h change
> first so I could read the docs, if only diff could be taught such
> things ;-))

Quite.

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

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

> > +        return;
> > +
> >          break;
> 
> return then break? I think the break is redundant.

Yes.

> > -    if (dm_starting) {
> > +    assert(!dcs->dmss.guest_domid);
> > +    domcreate_devmodel_started(egc, &dcs->dmss, 0);
> 
> This is actually logically the "else" of the preceding need_qemu if,
> since all the other paths return before they get here. I spent quite a
> while figuring out why the problem with the "return; break;" I mentioned
> above was an extra break and not the return. Might be clearer to have
> this in the else?

Yes, good idea.

> > +static void domcreate_complete(libxl__egc *egc,
> > +                               libxl__domain_create_state *dcs,
> > +                               int rc) {
> 
> { should be on the next line.

Will fix.

> > +    /* convenience aliases */
> > +    libxl_domain_config *dm_config = &sdss->stubdom_config;
> > +    libxl_domain_config *guest_config = sdss->stubdom.guest_config;
> > +    int guest_domid = sdss->stubdom.guest_domid;
> 
> const? Just in case someone tries to modify it? (maybe there are similar
> cases in some of the previous blocks like this, I didn't notice).

OK.  (Yes, there are a couple of other similar blocks.)

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

> > -/* xl_exec */
> > +/*
> > + *----- spawn -----
> > + *
> > + * Higher-level double-fork and separate detach eg as for device models
> > + *
> > + * Each libxl__spawn_state is in one of the conventional states
> > + *    Undefined, Idle, Active
> 
> Conventional here means "as per event generation" I assume.

Yes.

> > + * The inner child must soon exit or exec.  If must also soon exit or
> 
>                                                It?

Yes.

> > + * In both children, the ctx is not fully useable: gc and logging
> 
>                                              usable
> 
> (apparently, I'm not convinced actually)

I always think "useable" is better but the rest of the code uses
"usable" elsewhere so I guess here should too.

> > + * longer repaorted via failure_cb.
> 
>              reported

Will fix.

> [...]
> 
> > +/*----- device model creation -----*/
> > +
> > +/* First layer; wraps libxl__spawn_spawn. */
> 
> The use of "First" here is terrifying to me as a reviewer ;-P

Sorry ...

> > +struct libxl__dm_spawn_state {
> > +    /* mixed - ao must be initialised by user; rest is private: */
> 
> I see no ao here, you mean spawn.ao?

Yes.  I have clarified this.

> > +/* 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 ?

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

Ian.

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