From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v5 09/10] libxl: call hotplug scripts for nic devices from libxl Date: Mon, 11 Jun 2012 15:34:40 +0100 Message-ID: <4FD60200.2030802@citrix.com> References: <1338383275-21315-1-git-send-email-roger.pau@citrix.com> <1338383275-21315-10-git-send-email-roger.pau@citrix.com> <20432.48963.214910.33148@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.48963.214910.33148@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 09/10] libxl: call hotplug scripts for nic devices from libxl"): >> Since most of the needed work is already done in previous patches, >> this patch only contains the necessary code to call hotplug scripts >> for nic devices, that should be called when the device is added or >> removed from a guest. > >> @@ -1894,10 +1897,19 @@ _hidden void libxl__initiate_device_remove(libxl__egc *egc, >> *< 0: Error >> * 0: No need to execute hotplug script >> * 1: Execute hotplug script >> + * >> + * The last parameter, "num_exec" refeers to the number of times hotplug >> + * scripts have been called for this device. This is currently used for >> + * IOEMU nic interfaces on Linux, since we need to call hotplug scripts twice >> + * for the same device, the first one to add the vif interface, and the second >> + * time to add the tap interface, so: >> + * num_exec == 0: execute hotplug for vif interface. >> + * num_exec == 1: execute hotplug for the associated tap interface. >> */ > > I think you should add: > > * The main body of libxl will, for each device, keep calling > * libxl__get_hotplug_script_info, with incrementing values of > * num_exec, and executing the resulting script accordingly, > * until libxl__get_hotplug_script_info returns<=0. > > Or > > * The main body of libxl will call libxl__get_hotplug_script_info > * with exactly the stated values of num_exec, above. For device > * types not mentioned the main body calls it once with > * num_exec==0. > > Personally I'm inclined think the knowledge that nics need two > invocations while disks need only one should be confined to the > non-portable Linux code. Or do you think different platforms will > always do this the same way ? It seems a bit odd to have the > information about num_exec spread about like this. So I would prefer > the former comment (and the corresponding change to the code). > > That also avoids a nic-specific section in the main body of libxl's > hotplug script machinery. What do you think about having a function in the non-portable code that tells you the number of times you have to call libxl__get_hotplug_script_info? So we can do something like: aodev->total_exec = libxl__get_hotplug_num_exec(...) It's not pretty, but at least will allow us to abstract this code from Linux/NetBSD, since what I basically do now with NetBSD is return 0 on the second call, thus avoiding executing anything. But we still have the ugly Linux part of the code in libxl_device. >> +int libxl__nic_type(libxl__gc *gc, libxl__device *dev, libxl_nic_type *nictype) >> +{ > ... >> + } >> + >> +out: >> + return rc; >> +} >> + > > As a matter of good practice I think you should say > rc = 0; > just before out, on the success path, and not rely on it having > happened to be set to 0 by the previous successful call. > > Thanks, > Ian.