Re: [Xen-devel] Fwd: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd

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

Ian Campbell escribiÃ:
> On Thu, 2012-04-26 at 12:48 +0100, Roger Pau Monne wrote:
> > Ian Jackson escribiÃ:
> > > Thanks for this mammoth patch.  My comments are below.
> > >
> > > Roger Pau Monne writes ("[Xen-devel] [PATCH v3 3/5] libxl: call hotplug scripts from 
> > > libxl for vbd"):
> > > > This is a rather big change, that adds the necessary machinery to perform
> > > > hotplug script calling from libxl for Linux. This patch launches the necessary
> > > > scripts to attach and detach PHY and TAP backend types (Qemu doesn't
> > > > use hotplug scripts).
> > > >
> > > > Here's a list of the major changes introduced:
> > > Can you please wrap your commit messages to no more than 75
> > > characters ?  Some vcs's indent them.  And of course they get indented
> > > when we reply leading to wrap damage.
> > >
> > > >    * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
> > > >      calls libxl__initiate_device_remove, so we can disconnect the device and
> > > >      execute the necessary hotplug scripts instead of just deleting the backend
> > > >      entries. So libxl__devices_destroy now uses the event library, and so does
> > > >      libxl_domain_destroy.
> > > So we no longer have any function which will synchronously and
> > > immediately destroy a domain and throw away everything to do with it.
> > > Is it actually the case that you think such a function cannot be
> > > provided ?
> > It can be provided, but we should create another command or add an
> > option to "destroy" (like -f), that doesn't wait for backend
> > disconnection and just nukes both frontend/backend xenstore entries.
> > This will also prevent us from executing hotplug scripts, and could lead
> > to unexpected results.
> We have a plan of action to fix this properly in 4.3 (via the Driver Dom
> protocol, which separates the backend from the hotplug info).
>
> It would IMHO be acceptable for 4.2 to keep on simply nuking the
> backend, and accepting the downsides which this entails. AFAIK this is
> not a regression vs Xend -- xend also just nukes the backend (please
> correct me if I am wrong about this).
This is ok if we execute hotplug scripts with udev, because they are 
executed when the "physical" device is destroyed, but if we have to 
launch the scripts from the toolstack, the only way to know that the 
physical device is no longer in use is by watching xenstore backend 
entries, that's why we cannot nuke them unless it is our last option.
> > > >    * Added a check in xen-hotplug-common.sh, so scripts are only executed from
> > > >      udev when using xend. xl adds a disable_udev=y to xenstore private 
> > > > directory
> > > >      and with the modification of the udev rules every call from udev gets
> > > >      HOTPLUG_GATE passed, that points to disable_udev. If HOTPLUG_GATE is passed
> > > >      and points to a value, the hotplug script is not executed.
> > > WDYM "points to a value"?  Did you just mean "is set to a nonempty
> > > value" ?  I'm not convinced by the name of this variable.  Shouldn't
> > > it have Xen in the name, and specify its own sense ?  Eg,
> > > XEN_HOTPLUG_DISABLE or something ?
> > This was decided with Ian Campbell,
> Please do feel free to take my half-baked suggestions and make them
> sensible ;-)
>
> Ian.

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