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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

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