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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

[...]
> +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?

[...]
> +/*----- synchronous subroutines -----*/
> +
> +static int setup_xenconsoled_pty(libxl__egc *egc, libxl__bootloader_state 
> *bl,
> +                                 char *slave_path, size_t slave_path_len)
>  {
> +    STATE_AO_GC(bl);

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

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:

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>



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