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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2012-04-23 at 17:48 +0100, Ian Jackson wrote:
> >  * 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 ?

The difference between remove and destroy is documented in libxl.h and
this patch does not change those definitions:

 * libxl_device_<type>_remove(ctx, domid, device):
 *
 *   Removes the given device from the specified domain by performing
 *   an orderly unplug with guest co-operation. This requires that the
 *   guest is running.
 *
 *   This method is currently synchronous and therefore can block
 *   while interacting with the guest.
 *
 * libxl_device_<type>_destroy(ctx, domid, device):
 *
 *   Removes the given device from the specified domain without guest
 *   co-operation. It is guest specific what affect this will have on
 *   a running guest.
 *
 *   This function does not interact with the guest and therefore
 *   cannot block on the guest.

These descriptions must be updated to refer to the new async behaviour.

Does the new implementation of destroy meet these requirements (modulo
now being asynchronous)?

The documentation does not currently mention blocking on the backend
domain, could you please add a comment which describes what the new
requirements are in this respect. Likewise hotplug scripts are not
mentioned in these docs.

In principal I think I am happy with the possibility to block
temporarily with a timeout on a backend domain but ultimately we need a
timeout and a fallback to the "just kill it" mode.

What happens here in 4.3 when we split the hotplug state out according
to whatever becomes of the "Driver domains communication protocol
proposal" thread? At that point the hotplug script state will be
separate from the backend state and we can go back to the "just nuke it"
mode, can't we? Would there be a regression vs. xend for us to stick
with the nuke it mode for 4.2 and fix this properly in 4.3?

I think it is important in the context of this patch to be clear about
what is the desired long term behaviour compared with the short term
behaviour being implemented for 4.2 is. We should also be clear about
what is being done now in order to address xl regressions vs xend. We
should also be clear about what is just papering over an issue for 4.2
vs what the proper fix in 4.3 will be. We also need to know what is
actually new functionality or behaviour (i.e. not fixing an xl vs xend
regression). IOW we need to have clear descriptions of the reasons for
the changes not just what the changes.

I think all the above need to be written down explicitly in either the
commit message or the introductory email, otherwise the review of this
series is just going to continue to go round in circles -- the reasoning
behind these changes is just too complex for a reviewer (even one who is
familiar with all this stuff already) to hold in their head.

> > +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{HOTPLUG_GATE}="/libxl/$env{XENBUS_PATH}/disable_udev", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> ...
> > +# Hack to prevent the execution of hotplug scripts from udev if the domain
> > +# has been launched from libxl
> > +if [ -n "${HOTPLUG_GATE}" ] && \
> > +   `xenstore-read "$HOTPLUG_GATE" >/dev/null 2>&1`; then
> > +    exit 0
> > +fi
> 
> Oh I see.  Hmm.  Why should this string not be fixed ?  I'm not sure I
> like the idea of being able to pass some random other xenstore path.
> 
> Also: this xenstore path should be a relative path, ie one relative to
> the xenstore home of domain running this part of the tools.  That way
> the scripts can be easily and automatically disabled for dom0 and
> enabled in driver domains.

XENBUS_PATH contains elements for both the back- and frontend domains as
well as the specific device.

Or do you think the key should be global per-(backend-domain rather than
per-device?

> > +/* Action to pass to hotplug caller functions */
> > +typedef enum {
> > +    CONNECT = 1,
> > +    DISCONNECT = 2
> > +} libxl__hotplug_action;
> 
> These names are rather too generic, I think.

enums should also be declared in lixl_types_internal.idl

Ian.