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

[Roger Pau Monne <roger.pau@citrix.com>]

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.