[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 29/31] libxl: ao: Convert libxl_run_bootloader

Ian Campbell writes ("Re: [Xen-devel] [PATCH 29/31] libxl: ao: Convert 
> [...]
> > +static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op);
> > +static void bootloader_keystrokes_copyfail(libxl__egc *egc,
> > +       libxl__datacopier_state *dc, int onwrite, int errnoval);
> > +static void bootloader_display_copyfail(libxl__egc *egc,
> > +       libxl__datacopier_state *dc, int onwrite, int errnoval);
> > +static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
> > +                                pid_t pid, int status);
> I suppose that in here there is a sequence of callbacks which get
> chained together? Might be worth pulling those out and ordering them in
> the normal expected chain of execution?

Yes.  The two "normal" callbacks are gotptys and finished, which
happen in that order.  The copyfail callbacks may happen in between.
So these are already in execution order.

> I wondered about this in the patch which defined it but didn't really
> consider it til I saw this user.
> I didn't quite realise back then that STATE_AO_GC takes a pointer to a
> struct which is user (a libxl internal user, but still) defined and
> requires that it has a member "ao".

This seemed a fine restriction to me, but I'm not wedded to it.

> I don't mind that for cases where
> the struct is defined alongside the macro as part of a while API but for
> separately defined structs its a bit nasty. Could it be:
>       STATE_AO_GC(bl->ao)
> instead?

So I will do this.  (I have removed your ack from my copy of that
underlying patch.)

> I suppose that isn't really a criticism of this patch but rather of the
> patch which added the macro, so this patch in itself is:



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.