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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +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