[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 09/10] libxl: call hotplug scripts for nic devices from libxl



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.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.