All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ian Campbell <Ian.Campbell@citrix.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl	for vbd
Date: Tue, 24 Apr 2012 10:16:38 +0100	[thread overview]
Message-ID: <1335258998.4347.59.camel@zakaz.uk.xensource.com> (raw)
In-Reply-To: <20373.34767.584624.953798@mariner.uk.xensource.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.

  reply	other threads:[~2012-04-24  9:16 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-20 13:23 [PATCH v3 0/5] libxl: call hotplug scripts from libxl Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 1/5] libxl: allow libxl__exec to take a parameter containing the env variables Roger Pau Monne
2012-04-23 15:37   ` Ian Jackson
2012-04-20 13:23 ` [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup Roger Pau Monne
2012-04-23 15:33   ` Ian Jackson
2012-04-23 15:42     ` Roger Pau Monne
2012-04-23 16:49       ` Ian Jackson
2012-04-23 16:52   ` Ian Jackson
2012-04-25 13:19     ` Roger Pau Monne
2012-05-02  9:44     ` Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 3/5] libxl: call hotplug scripts from libxl for vbd Roger Pau Monne
2012-04-23 16:48   ` Ian Jackson
2012-04-24  9:16     ` Ian Campbell [this message]
2012-04-24 10:09       ` Ian Jackson
2012-04-24 10:17         ` Ian Campbell
2012-04-24 10:27           ` Ian Jackson
2012-04-24 10:29             ` Ian Campbell
2012-04-24 10:37               ` Ian Jackson
2012-04-25 13:53                 ` Roger Pau Monne
2012-04-25 14:59                   ` Ian Campbell
     [not found]     ` <4F982F18.2080902@citrix.com>
2012-04-26 11:48       ` Fwd: " Roger Pau Monne
2012-04-26 12:06         ` Ian Campbell
2012-04-26 12:12           ` Roger Pau Monne
2012-04-26 13:02         ` Ian Jackson
2012-04-26 13:09           ` Ian Campbell
2012-05-11 16:06             ` Ian Jackson
2012-05-11 16:26               ` Ian Campbell
2012-04-20 13:23 ` [PATCH v3 4/5] libxl: call hotplug scripts from libxl for vif Roger Pau Monne
2012-04-23 16:59   ` Ian Jackson
2012-04-24  9:18     ` Ian Campbell
2012-04-25 11:19   ` Ian Campbell
2012-04-25 11:24     ` Roger Pau Monne
2012-04-20 13:23 ` [PATCH v3 5/5] libxl: add "downscript=no" to Qemu call Roger Pau Monne
2012-04-23 15:38   ` Ian Jackson
2012-04-23 12:12 ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Marek Marczykowski
2012-04-23 13:31   ` Roger Pau Monne
2012-04-23 13:47     ` Marek Marczykowski
2012-04-23 13:59       ` Non-dom0 block backends (was: Re: [PATCH v3 0/5] libxl: call hotplug scripts from libxl) Joanna Rutkowska
2012-04-23 14:02     ` [PATCH v3 0/5] libxl: call hotplug scripts from libxl Ian Campbell
2012-04-23 16:25       ` Roger Pau Monne
2012-04-24  9:18         ` Ian Campbell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1335258998.4347.59.camel@zakaz.uk.xensource.com \
    --to=ian.campbell@citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [PATCH v3 3/5] libxl: call hotplug scripts from libxl	for vbd' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.