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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v4 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.
> 
> Changes since v2:
> 
>  * Change libxl__nic_type to return the value in a parameter passed by
>    the caller.
> 
>  * Rename vif_execute to num_exec, to represent the number of times
>    hotplug scripts have been called for that device.

This observation needs to be reworked into a suitable form and placed
as a comment here in the code:

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index da5b02b..766f1f3 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> @@ -1860,7 +1863,8 @@ _hidden void libxl__initiate_device_remove(libxl__egc 
> *egc,
>   */
>  _hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>                                             char ***args, char ***env,
> -                                           libxl__device_action action);
> +                                           libxl__device_action action,
> +                                           int num_exec);

And the semantics need to be clearly explained.  In particular, this
is an interface that is supposed to be implemented by multiple
providers for different platforms.

I _think_ what is happening is that `num_exec' is always 0 for
everything but vifs.  For vifs libxl__get_hotplug_script_info is
always called with num_exec==0 for the actual PV vif (Xen PV
interface) but may also be called with num_exec==1 for the ioemu
device, but I may be wrong.

> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 98cd25f..e1e2abe 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -34,7 +34,8 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device 
> *dev)
...
> +static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
> +                               char ***args, char ***env,
> +                               libxl__device_action action, int num_exec)
...
> +    switch (nictype) {
> +    case LIBXL_NIC_TYPE_IOEMU:
> +        if (num_exec == 0) goto execute_vif;

Urgh!  This is a pretty nasty use of goto.

Thanks,
Ian.

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