From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 29/31] libxl: ao: Convert libxl_run_bootloader Date: Wed, 11 Apr 2012 12:31:44 +0100 Message-ID: <1334143904.5394.139.camel@zakaz.uk.xensource.com> References: <1334084885-14474-1-git-send-email-ian.jackson@eu.citrix.com> <1334084885-14474-30-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1334084885-14474-30-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org [...] > +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