From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation Date: Wed, 30 May 2012 10:54:06 +0100 Message-ID: <4FC5EE3E.5080701@citrix.com> References: <1337855045-10428-1-git-send-email-roger.pau@citrix.com> <1337855045-10428-6-git-send-email-roger.pau@citrix.com> <20420.57093.469702.580339@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: <20420.57093.469702.580339@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 v4 05/10] libxl: convert libxl_device_nic_add to an async operation"): >> This patch converts libxl_device_nic_add to an ao operation that >> waits for device backend to reach state XenbusStateInitWait and then >> marks the operation as completed. This is not really useful now, but >> will be used by latter patches that will launch hotplug scripts after >> we reached the desired xenbus state. > > Thanks. I'm afraid this is still tripping my repetition detector. > See below. > >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index 6c9bbc2..9933cc2 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -444,6 +444,24 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid, >> } >> } >> >> +void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid, >> + libxl_domain_config *d_config, >> + libxl__ao_device **devices, >> + libxl__device_callback callback) >> +{ >> + AO_GC; >> + GCNEW_ARRAY(*devices, d_config->num_vifs); >> + libxl__ao_device *aodev = *devices; >> + int i; >> + >> + for (i = 0; i< d_config->num_vifs; i++) { >> + libxl__prepare_ao_device(&aodev[i], ao, devices); >> + aodev[i].callback = callback; >> + libxl__device_nic_add(egc, domid,&d_config->vifs[i], >> +&aodev[i]); >> + } >> +} > > The same comment applies as before about libxl__add_disks and the two > loops. > > > But, this is another copy of libxl__add_disks. Perhaps you want a > macro to generate these functions ? Yes, a macro would be ok I think. > In general this patch seems to contain an awful lot of very similar > code to the previous one. Eg domcreate_nic_connected is practically > identical to domcreate_disk_connected. And domcreate_attach_nick is > practically identical to domcreate_attach_disk. > > > Another option would be to make the types of libxl__device_FOO_add > compatible (by replacing the actual config parameter with const void*) > and then pass them as function pointers. > > Or you could even have a table > { offsetof(disks, libxl_domain_config), sizeof(libxl_device_disk), > libxl__device_disk_add }, > { offsetof(vifs, libxl_domain_config), sizeof(libxl_device_vif), > libxl__device_nic_add }, > ... > which your domain creation logic could iterate over. That way you > would do away with domcreate_FOO_connected and you'd end up with > > static void domcreate_device_connected(libxl__egc *egc, > libxl__ao_device *aodev) > { > > tableentry = devicekindstable[dcs->currentdevicekind]; > > rc = libxl__ao_device_check_last(gc, aodev, dcs->devices, > dcs->num_devices); > > if (rc> 0) return; > if (rc) { > LOGE(ERROR, "error connecting devices"); > goto error_out; > } > > dcs->currentdevicekind++; > > if (!devicekindstable[dcs->currentdevicekind].add_function) { > domcreate_complete(egc, dcs, rc); > } else { > domcreate_attach_devices(egc, dcs); > } > > or something like it. We can really iterate over this to add the devices, since we might need to launch the device model between connecting the disks and connecting the nics, I will try to find a nicer way to do this, I will try to come up with some macros to define this in a nicer way. > Or something. Anyway, can you try to find a way to avoid me having to > review lots of nearly-identical code ? > > > Thanks, > Ian.