From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op Date: Mon, 11 Jun 2012 13:33:11 +0100 Message-ID: <4FD5E587.8020005@citrix.com> References: <1338383275-21315-1-git-send-email-roger.pau@citrix.com> <1338383275-21315-5-git-send-email-roger.pau@citrix.com> <20432.37559.262031.887156@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20432.37559.262031.887156@mariner.uk.xensource.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 Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op"): >> This patch converts libxl_device_disk_add to an ao operation that > > In the subject line `asyn' should be `async'. > > >> @@ -1859,11 +1883,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk) >> ret = 0; >> >> libxl_device_disk_remove(ctx, domid, disks + i, 0); >> - libxl_device_disk_add(ctx, domid, disk); >> + /* fixme-ao */ >> + libxl_device_disk_add(ctx, domid, disk, 0); > > Is there going to be a later patch in a future version of the series > that fixes these ? > Not really, but I might add one. > >> +/* Macro to define callbacks of libxl__add_*, checks if device is last >> + * and calls the function passed as a parameter to this macro with the >> + * following syntax; func(libxl__egc *, parent_type *, int rc) */ >> +#define DEFINE_DEVICES_CALLBACK(callback_name, parent_type, array_name, \ >> + array_size_name, func_to_call) \ >> + static void callback_name(libxl__egc *egc, libxl__ao_device *aodev) \ >> + { \ >> + STATE_AO_GC(aodev->ao); \ >> + parent_type *p = CONTAINER_OF(aodev->base, *p, array_name); \ >> + int rc; \ >> + \ >> + rc = libxl__ao_device_check_last(gc, aodev, p->array_name, \ >> + p->array_size_name); \ >> + if (rc> 0) return; \ >> + func_to_call(egc, p, rc); \ >> + } > > This is acceptable IMO and better than the repetition. However the > use of a macro for this, so far from the invocation sites, is indeed > less than perfect. > > Looking at the content of this macro, the only thing it needs from the > parent is array_name and array_size_name. If you invented > > typedef struct libxl__ao_devices libxl__ao_devices; > struct libxl__ao_devices { > libxl__ao_device *array; > int size; > void (*callback)(libxl__egc*, libxl__ao_devices*, int rc); > }; > > Then aodev->base could be of type libxl__ao_devices* and the macro > above could be a function. And indeed it would have type > suitable for simply filling into aodev->callback. Would that work ? Yes, I've applied this change. > Doing this would also perhaps mean DEFINE_DEVICES_ADD's generated > libxl__add_FOOs functions could become one single function. AFAICT > the only macroish things it does are: (1) accessing > d_config->num_FOOs, which could be passed in as a parameter; > (2) passing d_config->FOOs[i] to libxl__device_FOO_add, which could be > dealt with by making libxl__device_FOO_add take a void* for the config > instead, and passing the start of the d_config->FOOs array, plus the > element size, and the add function, as arguments. I think this is really too complex for the benefits it introduces, libxl__add_{disks/nics} already takes a lot of arguments. Also take into account that libxl__device_disk_add is different from the rest now, because it takes the extra xs_transaction parameter. > Sorry to mess you about. > > > Whether you retain this as a macro or make it a function, > >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 2dc1cee..1dc8247 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -585,6 +585,13 @@ DEFINE_DEVICES_CALLBACK(domcreate_disk_connected, >> libxl__domain_create_state, >> devices, num_devices, domcreate_launch_dm) >> >> +static void domcreate_attach_pci(libxl__egc *egc, >> + libxl__domain_create_state *dcs, int rc); >> + >> +DEFINE_DEVICES_CALLBACK(domcreate_nic_connected, >> + libxl__domain_create_state, >> + devices, num_devices, domcreate_attach_pci) >> + >> static void domcreate_console_available(libxl__egc *egc, >> libxl__domain_create_state *dcs); >> > > These seem not to be in linear execution order ? These kind of > arrangements with a chain of callbacks should really have both the > declarations and the definitions in linear order. Sadly that means > writing out the declarations of domcreate_nic_connected etc. here in > the declarations section but I think that's a price worth paying. > (This is something you'd have to do anyway if you replace the macro > with a function as I suggest.) Done.