[PATCH v2 00/10] libxl: add driver domain backend daemon

[PATCH v2 00/10] libxl: add driver domain backend daemon

[Roger Pau Monne]

This patch series introduces support to launch device backends from
driver domains using libxl, removing the need to setup udev on driver
domains.

The daemon currently supports vbds, vifs and qdisks as backends.

Patches from 1 to 9 focus on getting libxl to work on driver domains,
and add several helper functions that will be used to build the
infraestructure needed for libxl to work on domains different than
Dom0.

The last patch (10) adds a new xl command, "devd", that can be used to
launch a daemon that will listen to changes in the xenstore "backend"
directory for the domain and perform the necessary actions to attach
the devices.

*Acked
 [PATCH v2 01/10] libxl: Introduce nested async operations (nested
 [PATCH v2 02/10] libxl: create a local xenstore libxl and
*[PATCH v2 03/10] libxl: don't remove device frontend path from
 [PATCH v2 04/10] libxl: synchronize device removal when using driver
 [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain
 [PATCH v2 06/10] libxl: don't launch Qemu on Dom0 for Qdisk devices
 [PATCH v2 07/10] libxl: add Qdisk backend launch helper
 [PATCH v2 08/10] xl: put daemonize code in it's own function
 [PATCH v2 09/10] libxl: revert 326a7b74
 [PATCH v2 10/10] libxl: add device backend listener in order to

[PATCH v2 01/10] libxl: Introduce nested async operations (nested ao)

[Roger Pau Monne]

This allows a long-running ao to avoid accumulating memory.  Each
nested ao has its own gc.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_event.c    |   35 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   24 +++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 0fe4428..54d15db 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1572,6 +1572,7 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc)
     LOG(DEBUG,"ao %p: complete, rc=%d",ao,rc);
     assert(ao->magic == LIBXL__AO_MAGIC);
     assert(!ao->complete);
+    assert(!ao->nested);
     ao->complete = 1;
     ao->rc = rc;
 
@@ -1736,6 +1737,7 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
         const libxl_asyncprogress_how *how, libxl_event *ev)
 {
     AO_GC;
+    assert(!ao->nested);
     if (how->callback == dummy_asyncprogress_callback_ignore) {
         LOG(DEBUG,"ao %p: progress report: ignored",ao);
         libxl_event_free(CTX,ev);
@@ -1756,6 +1758,39 @@ void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
 }
 
 
+/* nested ao */
+
+_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent)
+{
+    /* We only use the parent to get the ctx.  However, we require the
+     * caller to provide us with an ao, not just a ctx, to prove that
+     * they are already in an asynchronous operation.  That will avoid
+     * people using this to (for example) make an ao in a non-ao_how
+     * function somewhere in the middle of libxl. */
+    libxl__ao *child = NULL;
+    libxl_ctx *ctx = libxl__gc_owner(&parent->gc);
+
+    assert(parent->magic == LIBXL__AO_MAGIC);
+
+    child = libxl__zalloc(&ctx->nogc_gc, sizeof(*child));
+    child->magic = LIBXL__AO_MAGIC;
+    child->nested = 1;
+    LIBXL_INIT_GC(child->gc, ctx);
+    libxl__gc *gc = &child->gc;
+
+    LOG(DEBUG,"ao %p: nested ao, parent %p", child, parent);
+    return child;
+}
+
+_hidden void libxl__nested_ao_free(libxl__ao *child)
+{
+    assert(child->magic == LIBXL__AO_MAGIC);
+    assert(child->nested);
+    libxl_ctx *ctx = libxl__gc_owner(&child->gc);
+    libxl__ao__destroy(ctx, child);
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4729566..6ccd826 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -427,7 +427,7 @@ struct libxl__ao {
      * only in libxl__ao_complete.)
      */
     uint32_t magic;
-    unsigned constructing:1, in_initiator:1, complete:1, notified:1;
+    unsigned constructing:1, in_initiator:1, complete:1, notified:1, nested:1;
     int progress_reports_outstanding;
     int rc;
     libxl__gc gc;
@@ -1780,6 +1780,28 @@ _hidden void libxl__ao_complete_check_progress_reports(libxl__egc*, libxl__ao*);
 
 
 /*
+ * Short-lived sub-ao, aka "nested ao".
+ *
+ * Some asynchronous operations are very long-running.  Generally,
+ * since an ao has a gc, any allocations made in that ao will live
+ * until the ao is completed.  When this is not desirable, these
+ * functions may be used to manage a "sub-ao".
+ *
+ * The returned sub-ao is suitable for passing to gc-related functions
+ * and macros such as libxl__ao_inprogress_gc, AO_GC, and STATE_AO_GC.
+ *
+ * It MUST NOT be used with AO_INPROGRESS, AO_ABORT,
+ * libxl__ao_complete, libxl__ao_progress_report, and so on.
+ *
+ * The caller must ensure that all of the sub-ao's are freed before
+ * the parent is.
+ */
+
+_hidden libxl__ao *libxl__nested_ao_create(libxl__ao *parent); /* cannot fail */
+_hidden void libxl__nested_ao_free(libxl__ao *child);
+
+
+/*
  * File descriptors and CLOEXEC
  */
 
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests

[Roger Pau Monne]

If libxl is executed inside a guest domain it needs write access to
the local libxl xenstore dir (/local/<domid>/libxl) to store internal
data. This also applies to Qemu which needs a
/local/<domid>/device-model xenstore directory.

This patch creates the mentioned directories for each guest launched
from libxl.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since v1:
 * Change documentation for the xs paths.
 * Add a "driver_domain" option that gates the creation of the
   specifc xs paths.

Changes since RFC:
 * Add documentation for the new paths.
---
 docs/man/xl.cfg.pod.5             |    5 +++++
 docs/misc/xenstore-paths.markdown |   10 ++++++++++
 tools/libxl/libxl.h               |    9 +++++++++
 tools/libxl/libxl_create.c        |   17 +++++++++++++++++
 tools/libxl/libxl_types.idl       |    1 +
 tools/libxl/xl_cmdimpl.c          |    2 ++
 6 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index d2d8921..d260895 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -307,6 +307,11 @@ which are incompatible with migration. Currently this is limited to
 enabling the invariant TSC feature flag in cpuid results when TSC is
 not emulated.
 
+=item B<driver_domain=BOOLEAN>
+
+Specify that this domain is a driver domain. This enables certain
+features needed in order to run a driver domain.
+
 =back
 
 =head2 Devices
diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 1c634b5..a0fc003 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -318,6 +318,16 @@ protocol definition.
 
 A domain writable path. Available for arbitrary domain use.
 
+### Paths private to the toolstack
+
+#### ~/device-model/$DOMID/state [w]
+
+Contains the status of the device models running on the domain.
+
+#### ~/libxl/$DOMID/qdisk-backend-pid [w]
+
+Contains the PIDs of the device models running on the domain.
+
 ## Virtual Machine Paths
 
 The /vm/$UUID namespace is used by toolstacks to store various
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1c6675d..9e5651f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -369,6 +369,15 @@
  */
 #define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS 1
 
+/*
+ * LIBXL_HAVE_DRIVER_DOMAIN_CREATION 1
+ *
+ * If this is defined, libxl_domain_create_info contains a driver_domain
+ * field that can be used to tell libxl that the domain that is going
+ * to be created is a driver domain, so the necessary actions are taken.
+ */
+#define LIBXL_HAVE_DRIVER_DOMAIN_CREATION 1
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9d793ba..4e2391d 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -36,6 +36,7 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     }
 
     libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
+    libxl_defbool_setdefault(&c_info->driver_domain, false);
 
     return 0;
 }
@@ -533,6 +534,22 @@ retry_transaction:
     libxl__xs_mkdir(gc, t,
                     libxl__sprintf(gc, "%s/data", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
+
+    if (libxl_defbool_val(info->driver_domain)) {
+        /*
+         * Create a local "libxl" directory for each guest, since we might want
+         * to use libxl from inside the guest
+         */
+        libxl__xs_mkdir(gc, t, GCSPRINTF("%s/libxl", dom_path), rwperm,
+                        ARRAY_SIZE(rwperm));
+        /*
+         * Create a local "device-model" directory for each guest, since we
+         * might want to use Qemu from inside the guest
+         */
+        libxl__xs_mkdir(gc, t, GCSPRINTF("%s/device-model", dom_path), rwperm,
+                        ARRAY_SIZE(rwperm));
+    }
+
     if (info->type == LIBXL_DOMAIN_TYPE_HVM)
         libxl__xs_mkdir(gc, t,
             libxl__sprintf(gc, "%s/hvmloader/generation-id-address", dom_path),
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5c43d6f..f9e4ea2 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -257,6 +257,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
     ("platformdata", libxl_key_value_list),
     ("poolid",       uint32),
     ("run_hotplug_scripts",libxl_defbool),
+    ("driver_domain",libxl_defbool),
     ], dir=DIR_IN)
 
 libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 40feb7d..b9831bc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -848,6 +848,8 @@ static void parse_config_data(const char *config_source,
     if (!xlu_cfg_get_long(config, "max_event_channels", &l, 0))
         b_info->event_channels = l;
 
+    xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
+
     switch(b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (!xlu_cfg_get_string (config, "kernel", &buf, 0))
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests"):
> If libxl is executed inside a guest domain it needs write access to
> the local libxl xenstore dir (/local/<domid>/libxl) to store internal
> data. This also applies to Qemu which needs a
> /local/<domid>/device-model xenstore directory.
>
> This patch creates the mentioned directories for each guest launched
> from libxl.
...
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index d2d8921..d260895 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -307,6 +307,11 @@ which are incompatible with migration. Currently this is limited to
>  enabling the invariant TSC feature flag in cpuid results when TSC is
>  not emulated.
>  
> +=item B<driver_domain=BOOLEAN>
> +
> +Specify that this domain is a driver domain. This enables certain
> +features needed in order to run a driver domain.
> +

"Certain features" is a bit vague.  The definition of "driver domain"
could be expaneded perhaps.  At the very least the manpage should
explain whether a domain gets additional capabilities as a result of
this option.  You should also say why not to set it always.

Is it the case that setting this option allows a guest to pretend to
have backends for other guests ?

Ian.

Re: [PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests

[Roger Pau Monné]

On 11/11/13 16:24, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests"):
>> If libxl is executed inside a guest domain it needs write access to
>> the local libxl xenstore dir (/local/<domid>/libxl) to store internal
>> data. This also applies to Qemu which needs a
>> /local/<domid>/device-model xenstore directory.
>>
>> This patch creates the mentioned directories for each guest launched
>> from libxl.
> ...
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index d2d8921..d260895 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -307,6 +307,11 @@ which are incompatible with migration. Currently this is limited to
>>  enabling the invariant TSC feature flag in cpuid results when TSC is
>>  not emulated.
>>  
>> +=item B<driver_domain=BOOLEAN>
>> +
>> +Specify that this domain is a driver domain. This enables certain
>> +features needed in order to run a driver domain.
>> +
> 
> "Certain features" is a bit vague.  The definition of "driver domain"
> could be expaneded perhaps.  At the very least the manpage should
> explain whether a domain gets additional capabilities as a result of
> this option.  You should also say why not to set it always.

"Certain features" means two writeable xenstore paths inside of the
guest local path. IanC felt that it was best to set this as an option,
but I don't see any problem with enabling it for all guests (the guest
can already write to some local xenstore paths).

I've written this as a vague description because I don't know if we
might wish to expand this further at some point in the future.

> Is it the case that setting this option allows a guest to pretend to
> have backends for other guests ?

A guest can not pretend to have backends for other guests unless the
control domain writes the appropriate backend path inside of the guest,
this has not changed with this path.

Re: [PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v2 02/10] libxl: create a local xenstore libxl and device-model dir for guests"):
> On 11/11/13 16:24, Ian Jackson wrote:
...
> > "Certain features" is a bit vague.  The definition of "driver domain"
> > could be expaneded perhaps.  At the very least the manpage should
> > explain whether a domain gets additional capabilities as a result of
> > this option.  You should also say why not to set it always.
> 
> "Certain features" means two writeable xenstore paths inside of the
> guest local path. IanC felt that it was best to set this as an option,
> but I don't see any problem with enabling it for all guests (the guest
> can already write to some local xenstore paths).
> 
> I've written this as a vague description because I don't know if we
> might wish to expand this further at some point in the future.

Hmm.  I guess it will do.  I think I would have written something
different but I don't want to argue about this too much.

> > Is it the case that setting this option allows a guest to pretend to
> > have backends for other guests ?
> 
> A guest can not pretend to have backends for other guests unless the
> control domain writes the appropriate backend path inside of the guest,
> this has not changed with this path.

OK, good.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH v2 03/10] libxl: don't remove device frontend path from driver domains

[Roger Pau Monne]

A domain different than LIBXL_TOOLSTACK_DOMID should not try to remove
the frontend paths of a device.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * Toolstack domain is in charge of cleaning both the frontend and the
   backend paths, driver domain only removes the first directory of
   the backend path.
 * I have decided to not take IanC Ack on this patch because it has
   changed since RFC.
---
 tools/libxl/libxl_device.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 48acc92..d726f0b 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -562,6 +562,10 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
     const char *tapdisk_params;
     xs_transaction_t t = 0;
     int rc;
+    uint32_t domid;
+
+    rc = libxl__get_domid(gc, &domid);
+    if (rc) goto out;
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
@@ -571,8 +575,20 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
         rc = libxl__xs_read_checked(gc, t, tapdisk_path, &tapdisk_params);
         if (rc) goto out;
 
-        libxl__xs_path_cleanup(gc, t, fe_path);
-        libxl__xs_path_cleanup(gc, t, be_path);
+        if (domid == LIBXL_TOOLSTACK_DOMID) {
+            /*
+             * The toolstack domain is in charge for removing both the
+             * frontend and the backend path
+             */
+            libxl__xs_path_cleanup(gc, t, fe_path);
+            libxl__xs_path_cleanup(gc, t, be_path);
+        } else if (dev->backend_domid == domid) {
+            /*
+             * The driver domain is in charge for removing what it can
+             * from the backend path
+             */
+            libxl__xs_path_cleanup(gc, t, be_path);
+        }
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH v2 04/10] libxl: synchronize device removal when using driver domains

[Roger Pau Monne]

Synchronize the clean up of the backend from the toolstack domain when
the driver domain has actually finished closing the backend for the
device.

This is accomplished by waiting for the driver domain to  remove the
directory containing the backend keys, then the toolstack domain will
finish the cleanup by removing the empty folders on the backend path.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since v1:
 * Place cleanup of aodev events in device_hotplug_clean, get rid of
   some code duplication.

Changes since RFC:
 * This patch has been reworked to synchronize the toolstack and the
   driver domain, by making the driver domain only remove the first
   directory of the backend path, and the toolstack domain remove the
   rest.
---
 tools/libxl/libxl_device.c   |   81 ++++++++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h |    2 +
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index d726f0b..8987434 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -443,6 +443,11 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
     aodev->num_exec = 0;
     /* Initialize timer for QEMU Bodge and hotplug execution */
     libxl__ev_time_init(&aodev->timeout);
+    /*
+     * Initialize xs_watch, because it's not used on all possible
+     * execution paths, but it's unconditionally destroyed when finished.
+     */
+    libxl__ev_xswatch_init(&aodev->xs_watch);
     aodev->active = 1;
     /* We init this here because we might call device_hotplug_done
      * without actually calling any hotplug script */
@@ -722,6 +727,14 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
                                           libxl__ev_child *child,
                                           pid_t pid, int status);
 
+static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                         const struct timeval *requested_abs);
+
+static void device_destroy_be_watch_cb(libxl__egc *egc,
+                                       libxl__ev_xswatch *watch,
+                                       const char *watch_path,
+                                       const char *event_path);
+
 static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev);
 
 static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev);
@@ -781,12 +794,14 @@ void libxl__initiate_device_remove(libxl__egc *egc,
         LOG(ERROR, "unable to get info for domain %d", domid);
         goto out;
     }
+
     if (QEMU_BACKEND(aodev->dev) &&
         (info.paused || info.dying || info.shutdown)) {
         /*
          * TODO: 4.2 Bodge due to QEMU, see comment on top of
          * libxl__initiate_device_remove in libxl_internal.h
          */
+
         rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
                                          device_qemu_timeout,
                                          LIBXL_QEMU_BODGE_TIMEOUT * 1000);
@@ -932,8 +947,28 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
      */
     rc = libxl__get_domid(gc, &domid);
     if (rc) goto out;
-    if (aodev->dev->backend_domid != domid)
-        goto out;
+    if (aodev->dev->backend_domid != domid) {
+        if (aodev->action != LIBXL__DEVICE_ACTION_REMOVE)
+            goto out;
+
+        rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+                                         device_destroy_be_timeout_cb,
+                                         LIBXL_DESTROY_TIMEOUT * 1000);
+        if (rc) {
+            LOG(ERROR, "setup of xs watch timeout failed");
+            goto out;
+        }
+
+        rc = libxl__ev_xswatch_register(gc, &aodev->xs_watch,
+                                        device_destroy_be_watch_cb,
+                                        be_path);
+        if (rc) {
+            LOG(ERROR, "setup of xs watch for %s failed", be_path);
+            libxl__ev_time_deregister(gc, &aodev->timeout);
+            goto out;
+        }
+        return;
+    }
 
     /* Check if we have to execute hotplug scripts for this device
      * and return the necessary args/env vars for execution */
@@ -1051,6 +1086,47 @@ error:
     device_hotplug_done(egc, aodev);
 }
 
+static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                         const struct timeval *requested_abs)
+{
+    libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
+    STATE_AO_GC(aodev->ao);
+
+    LOG(ERROR, "timed out while waiting for %s to be removed",
+               libxl__device_backend_path(gc, aodev->dev));
+
+    aodev->rc = ERROR_TIMEDOUT;
+
+    device_hotplug_done(egc, aodev);
+    return;
+}
+
+static void device_destroy_be_watch_cb(libxl__egc *egc,
+                                       libxl__ev_xswatch *watch,
+                                       const char *watch_path,
+                                       const char *event_path)
+{
+    libxl__ao_device *aodev = CONTAINER_OF(watch, *aodev, xs_watch);
+    STATE_AO_GC(aodev->ao);
+    const char *dir;
+    int rc;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, watch_path, &dir);
+    if (rc) {
+        LOG(ERROR, "unable to read backend path: %s", watch_path);
+        aodev->rc = rc;
+        goto out;
+    }
+    if (dir) {
+        /* backend path still exists, wait a little longer... */
+        return;
+    }
+
+out:
+    /* We are done, backend path no longer exists */
+    device_hotplug_done(egc, aodev);
+}
+
 static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
@@ -1073,6 +1149,7 @@ static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev)
 {
     /* Clean events and check reentrancy */
     libxl__ev_time_deregister(gc, &aodev->timeout);
+    libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
     assert(!libxl__ev_child_inuse(&aodev->child));
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6ccd826..8ea654d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1906,6 +1906,8 @@ struct libxl__ao_device {
     libxl__ev_devstate backend_ds;
     /* Bodge for Qemu devices, also used for timeout of hotplug execution */
     libxl__ev_time timeout;
+    /* xenstore watch for backend path of driver domains */
+    libxl__ev_xswatch xs_watch;
     /* device hotplug execution */
     const char *what;
     int num_exec;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 04/10] libxl: synchronize device removal when using driver domains

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 04/10] libxl: synchronize device removal when using driver domains"):
> Synchronize the clean up of the backend from the toolstack domain when
> the driver domain has actually finished closing the backend for the
> device.
> 
> This is accomplished by waiting for the driver domain to  remove the
> directory containing the backend keys, then the toolstack domain will
> finish the cleanup by removing the empty folders on the backend path.
...
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index d726f0b..8987434 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -443,6 +443,11 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
>      aodev->num_exec = 0;
>      /* Initialize timer for QEMU Bodge and hotplug execution */
>      libxl__ev_time_init(&aodev->timeout);
> +    /*
> +     * Initialize xs_watch, because it's not used on all possible
> +     * execution paths, but it's unconditionally destroyed when finished.
> +     */
> +    libxl__ev_xswatch_init(&aodev->xs_watch);

FWIW I think this is entirely unremarkable and does not deserve a
comment.  Every field in the newly created aodev should be
initialised.  The fact that it wasn't was a latent bug.  So I would
mention that in the commit message instead.

But I don't feel strongly about this so keep it as it is if you like.
And there are several other similar comments in the same function.

>  static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev);
> @@ -781,12 +794,14 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>          LOG(ERROR, "unable to get info for domain %d", domid);
>          goto out;
>      }
> +
>      if (QEMU_BACKEND(aodev->dev) &&
>          (info.paused || info.dying || info.shutdown)) {
>          /*
>           * TODO: 4.2 Bodge due to QEMU, see comment on top of
>           * libxl__initiate_device_remove in libxl_internal.h
>           */
> +
>          rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
>                                           device_qemu_timeout,
>                                           LIBXL_QEMU_BODGE_TIMEOUT * 1000);

Unintentional whitespace changes ?

Aside from that, this patch all looks good to me.

Thanks,
Ian.

Re: [PATCH v2 04/10] libxl: synchronize device removal when using driver domains

[Roger Pau Monné]

On 11/11/13 18:02, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 04/10] libxl: synchronize device removal when using driver domains"):
>> Synchronize the clean up of the backend from the toolstack domain when
>> the driver domain has actually finished closing the backend for the
>> device.
>>
>> This is accomplished by waiting for the driver domain to  remove the
>> directory containing the backend keys, then the toolstack domain will
>> finish the cleanup by removing the empty folders on the backend path.
> ...
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index d726f0b..8987434 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -443,6 +443,11 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
>>      aodev->num_exec = 0;
>>      /* Initialize timer for QEMU Bodge and hotplug execution */
>>      libxl__ev_time_init(&aodev->timeout);
>> +    /*
>> +     * Initialize xs_watch, because it's not used on all possible
>> +     * execution paths, but it's unconditionally destroyed when finished.
>> +     */
>> +    libxl__ev_xswatch_init(&aodev->xs_watch);
> 
> FWIW I think this is entirely unremarkable and does not deserve a
> comment.  Every field in the newly created aodev should be
> initialised.  The fact that it wasn't was a latent bug.  So I would
> mention that in the commit message instead.
> 
> But I don't feel strongly about this so keep it as it is if you like.
> And there are several other similar comments in the same function.
> 
>>  static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev);
>> @@ -781,12 +794,14 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>>          LOG(ERROR, "unable to get info for domain %d", domid);
>>          goto out;
>>      }
>> +
>>      if (QEMU_BACKEND(aodev->dev) &&
>>          (info.paused || info.dying || info.shutdown)) {
>>          /*
>>           * TODO: 4.2 Bodge due to QEMU, see comment on top of
>>           * libxl__initiate_device_remove in libxl_internal.h
>>           */
>> +
>>          rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
>>                                           device_qemu_timeout,
>>                                           LIBXL_QEMU_BODGE_TIMEOUT * 1000);
> 
> Unintentional whitespace changes ?

Yes, sorry for that.

> Aside from that, this patch all looks good to me.

Great, if you have other comments on the series I will resend it with
the whitespace removed. Should I take the "looks good to me" as an Ack
if the whitespace is removed?

Re: [PATCH v2 04/10] libxl: synchronize device removal when using driver domains

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v2 04/10] libxl: synchronize device removal when using driver domains"):
> On 11/11/13 18:02, Ian Jackson wrote:
> 
> Great, if you have other comments on the series I will resend it with
> the whitespace removed. Should I take the "looks good to me" as an Ack
> if the whitespace is removed?

Yes indeed.

Thanks,
Ian.

[PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices

[Roger Pau Monne]

When Qemu is launched from a driver domain to act as a PV disk
backend we can make sure that Qemu is running before detaching
devices, so there's no need for the bodge there.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since v1:
 * Remove a superfluous return.
---
 tools/libxl/libxl_device.c |   71 ++++++++++++++++++++++++++++++++------------
 1 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8987434..9679bf3 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -785,32 +785,39 @@ void libxl__initiate_device_remove(libxl__egc *egc,
     char *online_path = GCSPRINTF("%s/online", be_path);
     const char *state;
     libxl_dominfo info;
-    uint32_t domid = aodev->dev->domid;
+    uint32_t my_domid, domid = aodev->dev->domid;
     int rc = 0;
 
-    libxl_dominfo_init(&info);
-    rc = libxl_domain_info(CTX, &info, domid);
+    rc = libxl__get_domid(gc, &my_domid);
     if (rc) {
-        LOG(ERROR, "unable to get info for domain %d", domid);
+        LOG(ERROR, "unable to get my domid");
         goto out;
     }
 
-    if (QEMU_BACKEND(aodev->dev) &&
-        (info.paused || info.dying || info.shutdown)) {
-        /*
-         * TODO: 4.2 Bodge due to QEMU, see comment on top of
-         * libxl__initiate_device_remove in libxl_internal.h
-         */
-
-        rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
-                                         device_qemu_timeout,
-                                         LIBXL_QEMU_BODGE_TIMEOUT * 1000);
+    if (my_domid == LIBXL_TOOLSTACK_DOMID) {
+        libxl_dominfo_init(&info);
+        rc = libxl_domain_info(CTX, &info, domid);
         if (rc) {
-            LOG(ERROR, "unable to register timeout for Qemu device %s",
-                       be_path);
+            LOG(ERROR, "unable to get info for domain %d", domid);
             goto out;
         }
-        return;
+        if (QEMU_BACKEND(aodev->dev) &&
+            (info.paused || info.dying || info.shutdown)) {
+            /*
+             * TODO: 4.2 Bodge due to QEMU, see comment on top of
+             * libxl__initiate_device_remove in libxl_internal.h
+             */
+
+            rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
+                                             device_qemu_timeout,
+                                             LIBXL_QEMU_BODGE_TIMEOUT * 1000);
+            if (rc) {
+                LOG(ERROR, "unable to register timeout for Qemu device %s",
+                           be_path);
+                goto out;
+            }
+            return;
+        }
     }
 
     for (;;) {
@@ -879,17 +886,43 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
     STATE_AO_GC(aodev->ao);
     char *be_path = libxl__device_backend_path(gc, aodev->dev);
     char *state_path = GCSPRINTF("%s/state", be_path);
+    const char *xs_state;
+    xs_transaction_t t = 0;
     int rc = 0;
 
     libxl__ev_time_deregister(gc, &aodev->timeout);
 
-    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
-    if (rc) goto out;
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) {
+            LOG(ERROR, "unable to start transaction");
+            goto out;
+        }
+
+        /*
+         * Check that the state path exists and is actually different than
+         * 6 before unconditionally setting it. If Qemu runs on a driver
+         * domain it is possible that the driver domain has already cleaned
+         * the backend path if the device has reached state 6.
+         */
+        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
+        if (rc) goto out;
+
+        if (xs_state && atoi(xs_state) != XenbusStateClosed) {
+            rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
+            if (rc) goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
 
     device_hotplug(egc, aodev);
     return;
 
 out:
+    libxl__xs_transaction_abort(gc, &t);
     aodev->rc = rc;
     device_hotplug_done(egc, aodev);
 }
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices"):
> When Qemu is launched from a driver domain to act as a PV disk
> backend we can make sure that Qemu is running before detaching
> devices, so there's no need for the bodge there.

I'm confused.  I don't see why this change is safe.

You say "we can make sure", but how ?  Do we actually make sure ?
What part of the code is "we" ?

> @@ -879,17 +886,43 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
...

And I don't understand how this next change relates to the above:

> -    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
> -    if (rc) goto out;
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) {
> +            LOG(ERROR, "unable to start transaction");
> +            goto out;
> +        }
> +
> +        /*
> +         * Check that the state path exists and is actually different than
> +         * 6 before unconditionally setting it. If Qemu runs on a driver
> +         * domain it is possible that the driver domain has already cleaned
> +         * the backend path if the device has reached state 6.
> +         */
> +        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
> +        if (rc) goto out;

This is on the "we hope qemu is dying and that this 2.0s wait is
enough" path from the diagram in libxl_internal.h (near line 1989) ?

By "the driver domain" you mean "the driver domain's libxl device
backend daemon" ?

So AFAICT this is an unrelated bugfix, relating to the fact that if
the state is set to 6 the backend daemon will remove the backend path,
and setting it back to 6 is wrong.

And in this case we aren't going to run the hotplug script because
this only happens in the toolstack domain's code when there is a
driver domain ?  Perhaps a more explicit check would be clearer.

Thanks,
Ian.

Re: [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices

[Roger Pau Monné]

On 15/11/13 18:09, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices"):
>> When Qemu is launched from a driver domain to act as a PV disk
>> backend we can make sure that Qemu is running before detaching
>> devices, so there's no need for the bodge there.
> 
> I'm confused.  I don't see why this change is safe.
> 
> You say "we can make sure", but how ?  Do we actually make sure ?
> What part of the code is "we" ?

What I was trying to say here, is that on a normal domain destruction on
Dom0, libxl first signals Qemu to exit and then removes the devices, so
when libxl starts device removal we cannot be sure if Qemu is still
running or not (that's why the bodge is there, to give Qemu enough time
to finish it's pending work and exit).

On the other hand, when running Qemu (Qdisk) on a driver domain, libxl
removes the devices first, and then signals Qemu to exit, so Qemu is (or
should) always be running during the device removal phase.

> 
>> @@ -879,17 +886,43 @@ static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
> ...
> 
> And I don't understand how this next change relates to the above:
> 
>> -    rc = libxl__xs_write_checked(gc, XBT_NULL, state_path, "6");
>> -    if (rc) goto out;
>> +    for (;;) {
>> +        rc = libxl__xs_transaction_start(gc, &t);
>> +        if (rc) {
>> +            LOG(ERROR, "unable to start transaction");
>> +            goto out;
>> +        }
>> +
>> +        /*
>> +         * Check that the state path exists and is actually different than
>> +         * 6 before unconditionally setting it. If Qemu runs on a driver
>> +         * domain it is possible that the driver domain has already cleaned
>> +         * the backend path if the device has reached state 6.
>> +         */
>> +        rc = libxl__xs_read_checked(gc, XBT_NULL, state_path, &xs_state);
>> +        if (rc) goto out;
> 
> This is on the "we hope qemu is dying and that this 2.0s wait is
> enough" path from the diagram in libxl_internal.h (near line 1989) ?

Yes.

> 
> By "the driver domain" you mean "the driver domain's libxl device
> backend daemon" ?

Yes.

> So AFAICT this is an unrelated bugfix, relating to the fact that if
> the state is set to 6 the backend daemon will remove the backend path,
> and setting it back to 6 is wrong.

Yes, but this was not possible before this series, and libxl could set
the backend path state to 6 unconditionally, because Qemu doesn't remove
the backend path on exit, so it's arguably that this was a bug before
this series.

This is not true any more, because we have the libxl driver backend
daemon that cleans the backend path on device removal, so setting state
to 6 from Dom0 after the libxl driver domain daemon has already removed
it is wrong.

> And in this case we aren't going to run the hotplug script because
> this only happens in the toolstack domain's code when there is a
> driver domain ?  Perhaps a more explicit check would be clearer.

No, libxl on Dom0 is not going to run the hotplug scripts, because this
backend is handled by another domain.

Re: [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v2 05/10] libxl: remove the Qemu bodge for driver domain devices"):
> On 15/11/13 18:09, Ian Jackson wrote:
> > I'm confused.  I don't see why this change is safe.
...
> [stuff]

Thanks for the explanation.  I'm convinced.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH v2 06/10] libxl: don't launch Qemu on Dom0 for Qdisk devices on driver domains

[Roger Pau Monne]

In libxl__need_xenpv_qemu check that the backend domain of the Qdisk
device is Dom0 before launching a Qemu instance in the toolstack
domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
Changes since RFC:
 * Fetch and use domid instead of LIBXL_TOOLSTACK_DOMID (altough the
   function is only called on Dom0 right now).
---
 tools/libxl/libxl_dm.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 85a08af..b4798b8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1354,6 +1354,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_disks, libxl_device_disk *disks)
 {
     int i, ret = 0;
+    uint32_t domid;
 
     /*
      * qemu is required in order to support 2 or more consoles. So switch all
@@ -1379,8 +1380,11 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
     }
 
     if (nr_disks > 0) {
+        ret = libxl__get_domid(gc, &domid);
+        if (ret) goto out;
         for (i = 0; i < nr_disks; i++) {
-            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK) {
+            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK &&
+                disks[i].backend_domid == domid) {
                 ret = 1;
                 goto out;
             }
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 06/10] libxl: don't launch Qemu on Dom0 for Qdisk devices on driver domains

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 06/10] libxl: don't launch Qemu on Dom0 for Qdisk devices on driver domains"):
> In libxl__need_xenpv_qemu check that the backend domain of the Qdisk
> device is Dom0 before launching a Qemu instance in the toolstack
> domain.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v2 07/10] libxl: add Qdisk backend launch helper

[Roger Pau Monne]

Current Qemu launch functions in libxl require the usage of data
structures only avaialbe on domain creation. All this information is
not need in order to launch a Qemu instance to serve Qdisk backends,
so introduce a new simplified helper that can be used to launch
Qemu/Qdisk, that will be used to launch Qdisk in driver domains.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dm.c       |  149 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_internal.h |   17 +++++
 2 files changed, 157 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index b4798b8..ad55ced 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1320,18 +1320,123 @@ static void device_model_spawn_outcome(libxl__egc *egc,
     dmss->callback(egc, dmss, rc);
 }
 
-int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+/* callbacks passed in libxl__spawn_spawn for Qdisk attach */
+static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                          const char *xsdata);
+static void qdisk_startup_failed(libxl__egc *egc,
+                                 libxl__spawn_state *spawn);
+static void qdisk_detached(libxl__egc *egc,
+                           libxl__spawn_state *spawn);
+
+void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__spawn_qdisk_state *sqs)
 {
-    char *pid;
-    int ret;
+    STATE_AO_GC(sqs->spawn.ao);
+    flexarray_t *dm_args;
+    char **args;
+    char *logfile = NULL;
+    const char *dm;
+    int logfile_w, null, rc;
+    uint32_t domid = sqs->guest_domid;
+
+    /* Always use qemu-xen as device model */
+    dm = qemu_xen_path(gc);
+
+    dm_args = flexarray_make(gc, 15, 1);
+    flexarray_vappend(dm_args, dm, "-xen-domid",
+                      GCSPRINTF("%d", domid), NULL);
+    flexarray_append(dm_args, "-xen-attach");
+    flexarray_vappend(dm_args, "-name",
+                      GCSPRINTF("domain-%u", domid), NULL);
+    flexarray_append(dm_args, "-nographic");
+    flexarray_vappend(dm_args, "-M", "xenpv", NULL);
+    flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
+    flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
+    flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
+    flexarray_append(dm_args, NULL);
+    args = (char **) flexarray_contents(dm_args);
 
-    pid = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/image/device-model-pid", domid));
-    if (!pid || !atoi(pid)) {
-        LOG(ERROR, "could not find device-model's pid for dom %u", domid);
-        ret = ERROR_FAIL;
+    libxl_create_logfile(CTX, GCSPRINTF("qdisk-%u", domid), &logfile);
+    logfile_w = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
+    if (logfile_w < 0) {
+        free(logfile);
+        rc = logfile_w;
+        goto error;
+    }
+    free(logfile);
+    null = open("/dev/null", O_RDONLY);
+
+    sqs->spawn.what = GCSPRINTF("domain %u Qdisk backend", domid);
+    sqs->spawn.xspath = GCSPRINTF("device-model/%u/state", domid);
+    sqs->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
+    /*
+     * We cannot save Qemu pid anywhere in the xenstore guest dir,
+     * because we will call this from unprivileged driver domains,
+     * so save it in the current domain libxl private dir.
+     */
+    sqs->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    sqs->spawn.midproc_cb = libxl__spawn_record_pid;
+    sqs->spawn.confirm_cb = qdisk_confirm;
+    sqs->spawn.failure_cb = qdisk_startup_failed;
+    sqs->spawn.detached_cb = qdisk_detached;
+    rc = libxl__spawn_spawn(egc, &sqs->spawn);
+    if (rc < 0)
+        goto error;
+    if (!rc) { /* inner child */
+        setsid();
+        libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
+    }
+
+    return;
+
+error:
+    assert(rc);
+    sqs->callback(egc, sqs, rc);
+    return;
+}
+
+static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                          const char *xsdata)
+{
+    STATE_AO_GC(spawn->ao);
+
+    if (!xsdata)
+        return;
+
+    if (strcmp(xsdata, "running"))
+        return;
+
+    libxl__spawn_initiate_detach(gc, spawn);
+}
+
+static void qdisk_startup_failed(libxl__egc *egc,
+                                 libxl__spawn_state *spawn)
+{
+    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
+    sqs->callback(egc, sqs, ERROR_FAIL);
+}
+
+static void qdisk_detached(libxl__egc *egc,
+                           libxl__spawn_state *spawn)
+{
+    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
+    sqs->callback(egc, sqs, 0);
+}
+
+/* Generic function to signal a Qemu instance to exit */
+static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
+{
+    const char *xs_pid;
+    int ret, pid;
+
+    ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
+    if (ret || !xs_pid) {
+        LOG(ERROR, "unable to find device model pid in %s", xs_path_pid);
+        ret = ret ? : ERROR_FAIL;
         goto out;
     }
-    ret = kill(atoi(pid), SIGHUP);
+    pid = atoi(xs_pid);
+
+    ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {
         LOG(ERROR, "Device Model already exited");
         ret = 0;
@@ -1339,7 +1444,7 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
         LOG(DEBUG, "Device Model signaled");
         ret = 0;
     } else {
-        LOGE(ERROR, "failed to kill Device Model [%d]", atoi(pid));
+        LOGE(ERROR, "failed to kill Device Model [%d]", pid);
         ret = ERROR_FAIL;
         goto out;
     }
@@ -1348,6 +1453,32 @@ out:
     return ret;
 }
 
+/* Helper to destroy a Qdisk backend */
+int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
+{
+    char *pid_path;
+    int rc;
+
+    pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+
+    rc = kill_device_model(gc, pid_path);
+    if (rc)
+        goto out;
+
+    libxl__xs_rm_checked(gc, XBT_NULL, pid_path);
+    libxl__xs_rm_checked(gc, XBT_NULL,
+                         GCSPRINTF("device-model/%u", domid));
+
+out:
+    return rc;
+}
+
+int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+{
+    return kill_device_model(gc,
+                GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+}
+
 int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8ea654d..2fdfaef 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2526,6 +2526,23 @@ _hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
 
 _hidden char *libxl__stub_dm_name(libxl__gc *gc, const char * guest_name);
 
+/* Qdisk backend launch helpers */
+
+typedef struct libxl__spawn_qdisk_state libxl__spawn_qdisk_state;
+
+typedef void libxl__qdisk_spawn_cb(libxl__egc *egc, libxl__spawn_qdisk_state*,
+                                   int rc /* if !0, error was logged */);
+
+struct libxl__spawn_qdisk_state {
+    uint32_t guest_domid;
+    libxl__spawn_state spawn;
+    libxl__qdisk_spawn_cb *callback;
+};
+
+_hidden void libxl__spawn_qdisk_backend(libxl__egc *egc,
+                                        libxl__spawn_qdisk_state *sqs);
+_hidden int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid);
+
 /*----- Domain creation -----*/
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 07/10] libxl: add Qdisk backend launch helper

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 07/10] libxl: add Qdisk backend launch helper"):
> Current Qemu launch functions in libxl require the usage of data
> structures only avaialbe on domain creation. All this information is
> not need in order to launch a Qemu instance to serve Qdisk backends,
> so introduce a new simplified helper that can be used to launch
> Qemu/Qdisk, that will be used to launch Qdisk in driver domains.

I think the principle here is good, and the implementation is pretty
good.


Looking for code duplication:

+void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__spawn_qdisk_state *sqs)
...
> +    libxl_create_logfile(CTX, GCSPRINTF("qdisk-%u", domid), &logfile);
> +    logfile_w = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
> +    if (logfile_w < 0) {
> +        free(logfile);
> +        rc = logfile_w;
> +        goto error;
> +    }
> +    free(logfile);
> +    null = open("/dev/null", O_RDONLY);

This is very like similar code in libxl__spawn_local_dm and could
perhaps profitably be factored out into a function.

> +static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
> +                          const char *xsdata)
> +{
> +    STATE_AO_GC(spawn->ao);
> +
> +    if (!xsdata)
> +        return;
> +
> +    if (strcmp(xsdata, "running"))
> +        return;
> +
> +    libxl__spawn_initiate_detach(gc, spawn);
> +}

This is practically identical to device_model_confirm.  Indeed, if you
delete the unneeded line in device_model_confirm for getting the
(unused) dmss, I think it's entirely identical.

> +static void qdisk_startup_failed(libxl__egc *egc,
> +                                 libxl__spawn_state *spawn)
> +{
> +    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
> +    sqs->callback(egc, sqs, ERROR_FAIL);
> +}

These are similar to the callbacks used by libxl__spawn_local_dm.

If you used a libxl__dm_spawn_state (even if mostly full of null
pointers) for the sqs, instead of its own type, you could abolish
these separate callbacks and rely on device_model_spawn_outcome to
DTRT I think.

What do you think ?


Thanks,
Ian.

[PATCH v2 08/10] xl: put daemonize code in it's own function

[Roger Pau Monne]

Move the daemonizer code from create_domain into it's own function
that can be called from other places different than create_domain.
This will be used to daemonize the driver domain backend handler.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   97 ++++++++++++++++++++++++++-------------------
 1 files changed, 56 insertions(+), 41 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b9831bc..1845dd5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -402,6 +402,56 @@ out:
     if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
 }
 
+static int do_daemonize(char *name)
+{
+    char *fullname;
+    pid_t child1, got_child;
+    int nullfd, ret = 0;
+    int status = 0;
+
+    child1 = xl_fork(child_waitdaemon);
+    if (child1) {
+        for (;;) {
+            got_child = xl_waitpid(child_waitdaemon, &status, 0);
+            if (got_child == child1) break;
+            assert(got_child == -1);
+            perror("failed to wait for daemonizing child");
+            ret = ERROR_FAIL;
+            goto out;
+        }
+        if (status) {
+            libxl_report_child_exitstatus(ctx, XTL_ERROR,
+                       "daemonizing child", child1, status);
+            ret = ERROR_FAIL;
+            goto out;
+        }
+        ret = 1;
+        goto out;
+    }
+
+    postfork();
+
+    ret = libxl_create_logfile(ctx, name, &fullname);
+    if (ret) {
+        LOG("failed to open logfile %s: %s",fullname,strerror(errno));
+        exit(-1);
+    }
+
+    CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
+                               0644) )<0);
+    free(fullname);
+
+    CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
+    dup2(nullfd, 0);
+    dup2(logfile, 1);
+    dup2(logfile, 2);
+
+    CHK_ERRNO(daemon(0, 1) < 0);
+
+out:
+    return ret;
+}
+
 static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a)
 {
     int i;
@@ -1901,7 +1951,6 @@ static uint32_t create_domain(struct domain_create *dom_info)
     void *config_data = 0;
     int config_len = 0;
     int restore_fd = -1;
-    int status = 0;
     const libxl_asyncprogress_how *autoconnect_console_how;
     struct save_file_header hdr;
 
@@ -2140,52 +2189,18 @@ start:
         autoconnect_vncviewer(domid, vncautopass);
 
     if (need_daemon) {
-        char *fullname, *name;
-        pid_t child1, got_child;
-        int nullfd;
-
-        child1 = xl_fork(child_waitdaemon);
-        if (child1) {
-            got_child = xl_waitpid(child_waitdaemon, &status, 0);
-            if (got_child != child1) {
-                assert(got_child == -1);
-                perror("failed to wait for daemonizing child");
-                ret = ERROR_FAIL;
-                goto out;
-            }
-            if (status) {
-                libxl_report_child_exitstatus(ctx, XTL_ERROR,
-                           "daemonizing child", child1, status);
-                ret = ERROR_FAIL;
-                goto out;
-            }
-            ret = domid;
-            goto out;
-        }
-
-        postfork();
+        char *name;
 
         if (asprintf(&name, "xl-%s", d_config.c_info.name) < 0) {
             LOG("Failed to allocate memory in asprintf");
             exit(1);
         }
-        rc = libxl_create_logfile(ctx, name, &fullname);
-        if (rc) {
-            LOG("failed to open logfile %s: %s",fullname,strerror(errno));
-            exit(-1);
-        }
-
-        CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
-                                   0644) )<0);
-        free(fullname);
+        ret = do_daemonize(name);
         free(name);
-
-        CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
-        dup2(nullfd, 0);
-        dup2(logfile, 1);
-        dup2(logfile, 2);
-
-        CHK_ERRNO(daemon(0, 1) < 0);
+        if (ret) {
+            ret = (ret == 1) ? domid : ret;
+            goto out;
+        }
         need_daemon = 0;
     }
     LOG("Waiting for domain %s (domid %d) to die [pid %ld]",
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 08/10] xl: put daemonize code in it's own function

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 08/10] xl: put daemonize code in it's own function"):
> Move the daemonizer code from create_domain into it's own function
> that can be called from other places different than create_domain.
> This will be used to daemonize the driver domain backend handler.

I haven't double-checked that the moved code is identical, but on the
basis that you don't intend any functional change:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH v2 09/10] libxl: revert 326a7b74

[Roger Pau Monne]

When running libxl from a driver domain there's no xenstore pid file
(because xenstore is not running on the driver domain). Also, at that
point in libxl initialization there's no way to know wether libxl is
running on a domain different than Dom0, so just revert the change in
order to allow libxl to work on driver domains.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |    7 -------
 tools/libxl/libxl_internal.h |    1 -
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0f0f56c..92a6ef7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -25,7 +25,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags, xentoollog_logger * lg)
 {
     libxl_ctx *ctx = NULL;
-    struct stat stat_buf;
     int rc;
 
     if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
@@ -82,12 +81,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     rc = libxl__poller_init(ctx, &ctx->poller_app);
     if (rc) goto out;
 
-    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
-                     "failed to stat %s", XENSTORE_PID_FILE);
-        rc = ERROR_FAIL; goto out;
-    }
-
     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2fdfaef..77039f4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -378,7 +378,6 @@ typedef struct {
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
 #define AUTO_PHP_SLOT          0x100
-#define XENSTORE_PID_FILE      "/var/run/xenstored.pid"
 
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 09/10] libxl: revert 326a7b74

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 09/10] libxl: revert 326a7b74"):
> When running libxl from a driver domain there's no xenstore pid file
> (because xenstore is not running on the driver domain). Also, at that
> point in libxl initialization there's no way to know wether libxl is
> running on a domain different than Dom0, so just revert the change in
> order to allow libxl to work on driver domains.

Thanks.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

You should have CC'd the patch author for the patch you're reverting,
so that they have the opportunity to comment, and perhaps come up with
a more sophisticated way to address the problem the original patch was
trying to deal with.  I have done that now.

Ian.

Re: [PATCH v2 09/10] libxl: revert 326a7b74

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v2 09/10] libxl: revert 326a7b74"):
> Roger Pau Monne writes ("[PATCH v2 09/10] libxl: revert 326a7b74"):
> > When running libxl from a driver domain there's no xenstore pid file
> > (because xenstore is not running on the driver domain). Also, at that
> > point in libxl initialization there's no way to know wether libxl is
> > running on a domain different than Dom0, so just revert the change in
> > order to allow libxl to work on driver domains.
> 
> Thanks.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> You should have CC'd the patch author for the patch you're reverting,
> so that they have the opportunity to comment, and perhaps come up with
> a more sophisticated way to address the problem the original patch was
> trying to deal with.  I have done that now.

Never mind, that copy bounced.

Ian.

Re: [PATCH v2 09/10] libxl: revert 326a7b74

[Andrew Cooper]

On 11/11/13 12:10, Roger Pau Monne wrote:
> When running libxl from a driver domain there's no xenstore pid file
> (because xenstore is not running on the driver domain). Also, at that
> point in libxl initialization there's no way to know wether libxl is
> running on a domain different than Dom0, so just revert the change in
> order to allow libxl to work on driver domains.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

With this change, I can get rid of my skanky "touch
/var/run/xenstored.pid" hack to get things like "xl info" to work in the
XenServer installer and early boot environments where xenstored is
really not running.

In fact, I nominate this change for backport.

~Andrew

> ---
>  tools/libxl/libxl.c          |    7 -------
>  tools/libxl/libxl_internal.h |    1 -
>  2 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 0f0f56c..92a6ef7 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -25,7 +25,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>                      unsigned flags, xentoollog_logger * lg)
>  {
>      libxl_ctx *ctx = NULL;
> -    struct stat stat_buf;
>      int rc;
>  
>      if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
> @@ -82,12 +81,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>      rc = libxl__poller_init(ctx, &ctx->poller_app);
>      if (rc) goto out;
>  
> -    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
> -                     "failed to stat %s", XENSTORE_PID_FILE);
> -        rc = ERROR_FAIL; goto out;
> -    }
> -
>      ctx->xch = xc_interface_open(lg,lg,0);
>      if (!ctx->xch) {
>          LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 2fdfaef..77039f4 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -378,7 +378,6 @@ typedef struct {
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
>  #define AUTO_PHP_SLOT          0x100
> -#define XENSTORE_PID_FILE      "/var/run/xenstored.pid"
>  
>  #define PROC_PCI_NUM_RESOURCES 7
>  #define PCI_BAR_IO             0x01


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

[PATCH v2 10/10] libxl: add device backend listener in order to launch backends

[Roger Pau Monne]

Add the necessary logic in libxl to allow it to act as a listener for
launching backends in a driver domain, replacing udev (like we already
do on Dom0). This new functionality is acomplished by watching the
domain backend path (/local/domain/<domid>/backend) and reacting to
device creation/destruction.

The way to launch this listener daemon is from xl, using the newly
introduced "devd" command. The command will daemonize by default,
using "xldevd.log" as it's logfile. Optionally the user can force the
execution of the listener in the foreground by passing the "-F"
option to the devd command.

Current backends handled by this daemon include Qdisk, vbd and vif
device types.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
In order to use Qdisk on a driver domain the following patch has to be
applied to Qemu:

http://marc.info/?l=qemu-devel&m=137953390608727&w=2
---
Changes since v1:
 * Use nested aos.
 * Reword some comments.
---
 tools/libxl/libxl.c       |  339 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h       |   12 ++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |   24 +++
 tools/libxl/xl_cmdtable.c |    6 +
 5 files changed, 382 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 92a6ef7..ebeeadc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3528,6 +3528,345 @@ DEFINE_DEVICE_ADD(vtpm)
 
 /******************************************************************************/
 
+/*
+ * Data structures used to track devices handled by driver domains
+ */
+
+/*
+ * Structure that describes a device handled by a driver domain
+ */
+typedef struct libxl__ddomain_device {
+    libxl__device *dev;
+    LIBXL_SLIST_ENTRY(struct libxl__ddomain_device) next;
+} libxl__ddomain_device;
+
+/*
+ * Structure that describes a domain and it's associated devices
+ */
+typedef struct libxl__ddomain_guest {
+    uint32_t domid;
+    int num_vifs, num_vbds, num_qdisks;
+    LIBXL_SLIST_HEAD(, struct libxl__ddomain_device) devices;
+    LIBXL_SLIST_ENTRY(struct libxl__ddomain_guest) next;
+} libxl__ddomain_guest;
+
+/*
+ * Main structure used by a driver domain to keep track of devices
+ * currently in use
+ */
+typedef struct {
+    libxl__ao *ao;
+    libxl__ev_xswatch watch;
+    LIBXL_SLIST_HEAD(, struct libxl__ddomain_guest) guests;
+} libxl__ddomain;
+
+static libxl__ddomain_guest *search_for_guest(libxl__ddomain *ddomain,
+                                               uint32_t domid)
+{
+    libxl__ddomain_guest *dguest;
+
+    LIBXL_SLIST_FOREACH(dguest, &ddomain->guests, next) {
+        if (dguest->domid == domid)
+            return dguest;
+    }
+    return NULL;
+}
+
+static libxl__ddomain_device *search_for_device(libxl__ddomain_guest *dguest,
+                                                libxl__device *dev)
+{
+    libxl__ddomain_device *ddev;
+
+    LIBXL_SLIST_FOREACH(ddev, &dguest->devices, next) {
+#define LIBXL_DEVICE_CMP(dev1, dev2, entry) (dev1->entry == dev2->entry)
+        if (LIBXL_DEVICE_CMP(ddev->dev, dev, backend_devid) &&
+            LIBXL_DEVICE_CMP(ddev->dev, dev, backend_domid) &&
+            LIBXL_DEVICE_CMP(ddev->dev, dev, devid) &&
+            LIBXL_DEVICE_CMP(ddev->dev, dev, domid) &&
+            LIBXL_DEVICE_CMP(ddev->dev, dev, backend_kind) &&
+            LIBXL_DEVICE_CMP(ddev->dev, dev, kind))
+            return ddev;
+#undef LIBXL_DEVICE_CMP
+    }
+
+    return NULL;
+}
+
+static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+
+    if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
+        free(aodev->dev);
+
+    LOG(DEBUG, "device %s %s %s",
+               libxl__device_backend_path(gc, aodev->dev),
+               libxl__device_action_to_string(aodev->action),
+               aodev->rc ? "failed" : "succeed");
+
+    libxl__nested_ao_free(aodev->ao);
+}
+
+static void qdisk_spawn_outcome(libxl__egc *egc, libxl__spawn_qdisk_state *sqs,
+                                int rc)
+{
+    STATE_AO_GC(sqs->spawn.ao);
+
+    LOG(DEBUG, "qdisk backend spawn for domain %u %s", sqs->guest_domid,
+               rc ? "failed" : "succeed");
+
+    libxl__nested_ao_free(sqs->spawn.ao);
+}
+
+/*
+ * The following comment applies to both add_device and remove_device.
+ *
+ * If the return value is greater than 0, it means there's no ao dispatched,
+ * so the free of the nested ao should be done by the parent when it has
+ * finished.
+ */
+static int add_device(libxl__egc *egc, libxl__ao *ao,
+                      libxl__ddomain_guest *dguest,
+                      libxl__ddomain_device *ddev)
+{
+    AO_GC;
+    libxl__device *dev = ddev->dev;
+    libxl__ao_device *aodev;
+    libxl__spawn_qdisk_state *sqs;
+    int rc = 0;
+
+    switch(dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+    case LIBXL__DEVICE_KIND_VIF:
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds++;
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs++;
+
+        GCNEW(aodev);
+        libxl__prepare_ao_device(ao, aodev);
+        aodev->dev = dev;
+        aodev->action = LIBXL__DEVICE_ACTION_ADD;
+        aodev->callback = device_complete;
+        libxl__wait_device_connection(egc, aodev);
+
+        break;
+    case LIBXL__DEVICE_KIND_QDISK:
+        if (dguest->num_qdisks == 0) {
+            GCNEW(sqs);
+            sqs->guest_domid = dev->domid;
+            sqs->spawn.ao = ao;
+            sqs->callback = qdisk_spawn_outcome;
+
+            libxl__spawn_qdisk_backend(egc, sqs);
+        }
+        dguest->num_qdisks++;
+
+        break;
+    default:
+        rc = 1;
+        break;
+    }
+
+    return rc;
+}
+
+static int remove_device(libxl__egc *egc, libxl__ao *ao,
+                         libxl__ddomain_guest *dguest,
+                         libxl__ddomain_device *ddev)
+{
+    AO_GC;
+    libxl__device *dev = ddev->dev;
+    libxl__ao_device *aodev;
+    int rc = 0;
+
+    switch(ddev->dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+    case LIBXL__DEVICE_KIND_VIF:
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VBD) dguest->num_vbds--;
+        if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) dguest->num_vifs--;
+
+        GCNEW(aodev);
+        libxl__prepare_ao_device(ao, aodev);
+        aodev->dev = dev;
+        aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
+        aodev->callback = device_complete;
+        libxl__initiate_device_remove(egc, aodev);
+        break;
+    case LIBXL__DEVICE_KIND_QDISK:
+        if (--dguest->num_qdisks == 0) {
+            rc = libxl__destroy_qdisk_backend(gc, dev->domid);
+            if (rc)
+                goto out;
+        }
+        libxl__device_destroy(gc, dev);
+        free(dev);
+        /* Fall through to return > 0, no ao has been dispatched */
+    default:
+        rc = 1;
+        break;
+    }
+
+out:
+    return rc;
+}
+
+static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
+                                   const char *watch_path,
+                                   const char *event_path)
+{
+    libxl__ddomain *ddomain = CONTAINER_OF(watch, *ddomain, watch);
+    libxl__ao *nested_ao = libxl__nested_ao_create(ddomain->ao);
+    STATE_AO_GC(nested_ao);
+    char *p, *path;
+    const char *sstate;
+    int state, rc, num_devs;
+    libxl__device *dev = NULL;
+    libxl__ddomain_device *ddev = NULL;
+    libxl__ddomain_guest *dguest = NULL;
+    bool free_ao = false;
+
+    /* Check if event_path ends with "state" and truncate it */
+    if (strlen(event_path) < strlen("state"))
+        goto skip;
+
+    path = libxl__strdup(gc, event_path);
+    p = path + strlen(path) - strlen("state") - 1;
+    if (*p != '/')
+        goto skip;
+    *p = '\0';
+    p++;
+    if (strcmp(p, "state") != 0)
+        goto skip;
+
+    /* Check if the state is 1 (XenbusStateInitialising) or greater */
+    rc = libxl__xs_read_checked(gc, XBT_NULL, event_path, &sstate);
+    if (rc || !sstate)
+        goto skip;
+    state = atoi(sstate);
+
+    dev = libxl__zalloc(NOGC, sizeof(*dev));
+    rc = libxl__parse_backend_path(gc, path, dev);
+    if (rc)
+        goto skip;
+
+    dguest = search_for_guest(ddomain, dev->domid);
+    if (dguest == NULL && state == XenbusStateClosed) {
+        /*
+         * Spurious state change, device has already been disconnected
+         * or never attached.
+         */
+        goto skip;
+    }
+    if (dguest == NULL) {
+        /* Create a new guest struct and initialize it */
+        dguest = libxl__zalloc(NOGC, sizeof(*dguest));
+        dguest->domid = dev->domid;
+        LIBXL_SLIST_INIT(&dguest->devices);
+        LIBXL_SLIST_INSERT_HEAD(&ddomain->guests, dguest, next);
+        LOG(DEBUG, "added domain %u to the list of active guests",
+                   dguest->domid);
+    }
+    ddev = search_for_device(dguest, dev);
+    if (ddev == NULL && state == XenbusStateClosed) {
+        /*
+         * Spurious state change, device has already been disconnected
+         * or never attached.
+         */
+        goto skip;
+    } else if (ddev == NULL) {
+        /*
+         * New device addition, allocate a struct to hold it and add it
+         * to the list of active devices for a given guest.
+         */
+        ddev = libxl__zalloc(NOGC, sizeof(*ddev));
+        ddev->dev = dev;
+        LIBXL_SLIST_INSERT_HEAD(&dguest->devices, ddev, next);
+        LOG(DEBUG, "added device %s to the list of active devices", path);
+        rc = add_device(egc, nested_ao, dguest, ddev);
+        if (rc > 0)
+            free_ao = true;
+    } else if (state == XenbusStateClosed) {
+        /*
+         * Removal of an active device, remove it from the list and
+         * free it's data structures if they are no longer needed.
+         *
+         * The free of the associated libxl__device is left to the
+         * helper remove_device function.
+         */
+        LIBXL_SLIST_REMOVE(&dguest->devices, ddev, libxl__ddomain_device,
+                           next);
+        LOG(DEBUG, "removed device %s from the list of active devices", path);
+        rc = remove_device(egc, nested_ao, dguest, ddev);
+        if (rc > 0)
+            free_ao = true;
+
+        free(ddev);
+        /* If this was the last device in the domain, remove it from the list */
+        num_devs = dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks;
+        if (num_devs == 0) {
+            LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
+                               next);
+            LOG(DEBUG, "removed domain %u from the list of active guests",
+                       dguest->domid);
+            /* Clear any leftovers in libxl/<domid> */
+            libxl__xs_rm_checked(gc, XBT_NULL,
+                                 GCSPRINTF("libxl/%u", dguest->domid));
+            free(dguest);
+        }
+    }
+
+    if (free_ao)
+        libxl__nested_ao_free(nested_ao);
+
+    return;
+
+skip:
+    libxl__nested_ao_free(nested_ao);
+    free(dev);
+    free(ddev);
+    free(dguest);
+    return;
+}
+
+/* Handler of events for device driver domains */
+int libxl_device_events_handler(libxl_ctx *ctx,
+                                const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, 0, ao_how);
+    int rc;
+    uint32_t domid;
+    libxl__ddomain ddomain;
+    char *be_path;
+
+    ddomain.ao = ao;
+    LIBXL_SLIST_INIT(&ddomain.guests);
+
+    rc = libxl__get_domid(gc, &domid);
+    if (rc) {
+        LOG(ERROR, "unable to get domain id");
+        goto out;
+    }
+
+    rc = libxl__xs_write_checked(gc, XBT_NULL, DISABLE_UDEV_PATH, "1");
+    if (rc) {
+        LOGE(ERROR, "unable to write %s = 1", DISABLE_UDEV_PATH);
+        goto out;
+    }
+
+    /*
+     * We use absolute paths because we want xswatch to also return
+     * absolute paths that can be parsed by libxl__parse_backend_path.
+     */
+    be_path = GCSPRINTF("/local/domain/%u/backend", domid);
+    rc = libxl__ev_xswatch_register(gc, &ddomain.watch, backend_watch_callback,
+                                    be_path);
+
+out:
+    GC_FREE;
+    return rc ? : AO_INPROGRESS;
+}
+
+/******************************************************************************/
+
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
 {
     GC_INIT(ctx);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9e5651f..5dc3a09 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -923,6 +923,18 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
                                         int *num);
 
 /*
+ * Turns the current process into a backend device service daemon
+ * for a driver domain.
+ *
+ * From a libxl API point of view, this starts a long-running
+ * operation.  That operation consists of "being a driver domain"
+ * and never completes.
+ */
+int libxl_device_events_handler(libxl_ctx *ctx,
+                                const libxl_asyncop_how *ao_how)
+                                LIBXL_EXTERNAL_CALLERS_ONLY;
+
+/*
  * Functions related to making devices assignable -- that is, bound to
  * the pciback driver, ready to be given to a guest via
  * libxl_pci_device_add.
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index e005c39..c876a33 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -105,6 +105,7 @@ int main_getenforce(int argc, char **argv);
 int main_setenforce(int argc, char **argv);
 int main_loadpolicy(int argc, char **argv);
 int main_remus(int argc, char **argv);
+int main_devd(int argc, char **argv);
 
 void help(const char *command);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1845dd5..98bcdce 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7209,6 +7209,30 @@ int main_remus(int argc, char **argv)
     return -ERROR_FAIL;
 }
 
+int main_devd(int argc, char **argv)
+{
+    int ret = 0, opt = 0, daemonize = 1;
+
+    SWITCH_FOREACH_OPT(opt, "F", NULL, "devd", 0) {
+    case 'F':
+        daemonize = 0;
+        break;
+    }
+
+    if (daemonize) {
+        ret = do_daemonize("xldevd");
+        if (ret) {
+            ret = (ret == 1) ? 0 : ret;
+            goto out;
+        }
+    }
+
+    libxl_device_events_handler(ctx, 0);
+
+out:
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 326a660..7709206 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -488,6 +488,12 @@ struct cmd_spec cmd_table[] = {
       "                        of the domain."
 
     },
+    { "devd",
+      &main_devd, 0, 1,
+      "Daemon that listens for devices and launches backends",
+      "[options]",
+      "-F                      Run in the foreground",
+    },
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 10/10] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v2 10/10] libxl: add device backend listener in order to launch backends"):
> Add the necessary logic in libxl to allow it to act as a listener for
> launching backends in a driver domain, replacing udev (like we already
> do on Dom0). This new functionality is acomplished by watching the
> domain backend path (/local/domain/<domid>/backend) and reacting to
> device creation/destruction.
...
> +
> +static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
> +                                   const char *watch_path,
> +                                   const char *event_path)
> +{
> +    libxl__ddomain *ddomain = CONTAINER_OF(watch, *ddomain, watch);
> +    libxl__ao *nested_ao = libxl__nested_ao_create(ddomain->ao);
> +    STATE_AO_GC(nested_ao);
> +    char *p, *path;
> +    const char *sstate;
> +    int state, rc, num_devs;
> +    libxl__device *dev = NULL;
> +    libxl__ddomain_device *ddev = NULL;
> +    libxl__ddomain_guest *dguest = NULL;
> +    bool free_ao = false;
> +
> +    /* Check if event_path ends with "state" and truncate it */
> +    if (strlen(event_path) < strlen("state"))
> +        goto skip;

I think this error handling style leaks the nested_ao sometimes.

I would suggest: rename "skip" to "out".

Would it be possible to abolish the "free_ao" variable, and to change
this:

> +        rc = add_device(egc, nested_ao, dguest, ddev);
> +        if (rc > 0)
> +            free_ao = true;

To this:
           if (!rc)
               /* device callback requires, and will dispose of,
                * nested_ao; ddev and dguest are linked in */
               return;

and always free the ao on ordinary exit ?

Ian.

Re: [PATCH v2 10/10] libxl: add device backend listener in order to launch backends

[Roger Pau Monné]

On 15/11/13 18:54, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 10/10] libxl: add device backend listener in order to launch backends"):
>> Add the necessary logic in libxl to allow it to act as a listener for
>> launching backends in a driver domain, replacing udev (like we already
>> do on Dom0). This new functionality is acomplished by watching the
>> domain backend path (/local/domain/<domid>/backend) and reacting to
>> device creation/destruction.
> ...
>> +
>> +static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
>> +                                   const char *watch_path,
>> +                                   const char *event_path)
>> +{
>> +    libxl__ddomain *ddomain = CONTAINER_OF(watch, *ddomain, watch);
>> +    libxl__ao *nested_ao = libxl__nested_ao_create(ddomain->ao);
>> +    STATE_AO_GC(nested_ao);
>> +    char *p, *path;
>> +    const char *sstate;
>> +    int state, rc, num_devs;
>> +    libxl__device *dev = NULL;
>> +    libxl__ddomain_device *ddev = NULL;
>> +    libxl__ddomain_guest *dguest = NULL;
>> +    bool free_ao = false;
>> +
>> +    /* Check if event_path ends with "state" and truncate it */
>> +    if (strlen(event_path) < strlen("state"))
>> +        goto skip;
> 
> I think this error handling style leaks the nested_ao sometimes.
> 
> I would suggest: rename "skip" to "out".

Ack, I don't mind changing it to out. I don't see any flow in which we
could leak the nested_ao right now.

> 
> Would it be possible to abolish the "free_ao" variable, and to change
> this:
> 
>> +        rc = add_device(egc, nested_ao, dguest, ddev);
>> +        if (rc > 0)
>> +            free_ao = true;
> 
> To this:
>            if (!rc)
>                /* device callback requires, and will dispose of,
>                 * nested_ao; ddev and dguest are linked in */
>                return;
> 
> and always free the ao on ordinary exit ?

I'm not sure I follow, if instead of setting free_ao to true I just
return, who is going to free the ddev and dguest if necessary? (note
that the callback is not using ddev or dguest at all)

Also, doing it in the callback is not safe, because we are no longer
holding the "Big lock", so we would have to add another lock which I
would prefer to avoid.

Re: [PATCH v2 10/10] libxl: add device backend listener in order to launch backends

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v2 10/10] libxl: add device backend listener in order to launch backends"):
> On 15/11/13 18:54, Ian Jackson wrote:
> > I would suggest: rename "skip" to "out".
> 
> Ack, I don't mind changing it to out. I don't see any flow in which we
> could leak the nested_ao right now.

Looking at it again I think I was confused.

> > Would it be possible to abolish the "free_ao" variable, and to change
> > this:
> > 
> >> +        rc = add_device(egc, nested_ao, dguest, ddev);
> >> +        if (rc > 0)
> >> +            free_ao = true;
> > 
> > To this:
> >            if (!rc)
> >                /* device callback requires, and will dispose of,
> >                 * nested_ao; ddev and dguest are linked in */
> >                return;
> > 
> > and always free the ao on ordinary exit ?
> 
> I'm not sure I follow, if instead of setting free_ao to true I just
> return, who is going to free the ddev and dguest if necessary? (note
> that the callback is not using ddev or dguest at all)

Right.  Sorry.

> Also, doing it in the callback is not safe, because we are no longer
> holding the "Big lock", so we would have to add another lock which I
> would prefer to avoid.

That's not actually a problem: all the in-libxl callbacks take place
with the libxl ctx lock held.  But I don't think that's relevant,
actually.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.