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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-04-11 at 12:39 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 29/31] libxl: ao: Convert 
> libxl_run_bootloader"):
> > [...]
> > > +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.

Oh, good then. I suppose that proves that ordering them that way is not
as helpful as I thought it would be ;-)

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

Thanks, sorry for prematurely Acking that patch.

FWIW, with the change above you could put it back...

> 
> > 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:
> 
> Thanks.
> 
> Ian.



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