[PATCH v2 00/13] xenstore: make it easier to run xenstore in a domain

[PATCH v2 00/13] xenstore: make it easier to run xenstore in a domain

[Juergen Gross]

Xen supports to run xenstore in a dedicated domain. It is, however, a
setup which isn't easy to configure. Today flask is required for full
functionality and the resulting xenstore domain is not configurable in
the same way as the xenstore daemon.

This patch series enables running a xenstore domain without flask. The
tool needed to start that domain is added to the installed binaries
and it is modified to pass arbitrary options to the xenstore domain.

I didn't include a configuration option for starting xenstore as an
own domain instead of the daemon, as this will require some major
tweaking of especially the systemd configuration files. Support for
this will be added later. The main issue is that running xenstore in
a domain requires the xenstore sockets not to be created. I don't
think this is possible with the current scheme of letting systemd
create those sockets.

Changes in V2:
- add new patch 3 to get the xenstore domain id via libxl
- move init-xenstore-domain to new directory tools/helpers in patch 4
  (former patch 3) and modify it to use the standard tools coding style
- add new patch 5 moving xen-init-dom0 to tools/helpers
- split patch 6 (former patch 4) into 2 patches as requested by Ian Campbell
- correct parameter parsing in patch 8 (former patch 5)
- modified commit message of patch 9 (former patch 6) as requested by
  Ian Campbell
- add new patch 11 to split up xen-init-dom0
- modify patch 12 (former patch 8) to create json object and more xenstore
  entries, add error messages as requested by Ian Campbell
- drop former patch 9
- add new patch 13 to avoid stopping xenstore domain on dom0 shutdown

Juergen Gross (13):
  xen: add xenstore domain flag to hypervisor
  libxc: support new xenstore domain flag in libxc
  libxl: provide a function to retrieve the xenstore domain id
  xenstore: move init-xenstore-domain to tools/helpers
  libxl: move xen-init-dom0 to tools/helpers
  xenstore: destroy xenstore domain in case of error after creating it
  xenstore: add error messages to init-xenstore-domain
  xenstore: modify init-xenstore-domain parameter syntax
  xenstore: make use of the "xenstore domain" flag
  xenstore: add init-xenstore-domain parameter to specify cmdline
  tools: split up xen-init-dom0.c
  xenstore: write xenstore domain data to xenstore
  tools: don't stop xenstore domain when stopping dom0

 tools/Makefile                        |   1 +
 tools/helpers/Makefile                |  45 +++++
 tools/helpers/init-dom-json.c         |  59 +++++++
 tools/helpers/init-dom-json.h         |  18 ++
 tools/helpers/init-xenstore-domain.c  | 310 ++++++++++++++++++++++++++++++++++
 tools/helpers/xen-init-dom0.c         |  67 ++++++++
 tools/hotplug/Linux/xendomains.in     |  17 ++
 tools/libxc/include/xenctrl.h         |   2 +-
 tools/libxc/xc_domain.c               |  17 +-
 tools/libxl/Makefile                  |  14 +-
 tools/libxl/libxl.c                   |  24 +++
 tools/libxl/libxl.h                   |  11 ++
 tools/libxl/xen-init-dom0.c           | 120 -------------
 tools/libxl/xl_cmdimpl.c              |  19 ++-
 tools/xenstore/Makefile               |   9 -
 tools/xenstore/init-xenstore-domain.c | 119 -------------
 xen/common/domain.c                   |   6 +
 xen/common/domctl.c                   |  14 +-
 xen/include/public/domctl.h           |   6 +
 xen/include/xen/sched.h               |   5 +
 xen/include/xsm/dummy.h               |   6 +
 xen/include/xsm/xsm.h                 |   1 +
 22 files changed, 613 insertions(+), 277 deletions(-)
 create mode 100644 tools/helpers/Makefile
 create mode 100644 tools/helpers/init-dom-json.c
 create mode 100644 tools/helpers/init-dom-json.h
 create mode 100644 tools/helpers/init-xenstore-domain.c
 create mode 100644 tools/helpers/xen-init-dom0.c
 delete mode 100644 tools/libxl/xen-init-dom0.c
 delete mode 100644 tools/xenstore/init-xenstore-domain.c

-- 
2.6.2

[PATCH v2 01/13] xen: add xenstore domain flag to hypervisor

[Juergen Gross]

In order to be able to have full support of a xenstore domain in Xen
add a "Xenstore-domain" flag to the hypervisor. This flag must be
specified at domain creation time and is returned by
XEN_DOMCTL_getdomaininfo.

It will allow the domain to retrieve domain information by issuing the
XEN_DOMCTL_getdomaininfo itself in order to be able to check for
domains having been destroyed. At the same time this flag will inhibit
the domain to be migrated, as this wouldn't be a very wise thing to do.

In case of a later support of a rebootable Dom0 this flag will allow to
recognize a xenstore domain already being present to connect to.

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/common/domain.c         |  6 ++++++
 xen/common/domctl.c         | 14 +++++++++-----
 xen/include/public/domctl.h |  6 ++++++
 xen/include/xen/sched.h     |  5 +++++
 xen/include/xsm/dummy.h     |  6 ++++++
 xen/include/xsm/xsm.h       |  1 +
 6 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1017efb..2979c1b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         hardware_domain = d;
     }
 
+    if ( domcr_flags & DOMCRF_xs_domain )
+    {
+        d->is_xenstore = 1;
+        d->disable_migrate = 1;
+    }
+
     rangeset_domain_initialise(d);
     init_status |= INIT_rangeset;
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 1fab587..dc1ea06 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -183,10 +183,11 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->cpu_time = cpu_time;
 
     info->flags = (info->nr_online_vcpus ? flags : 0) |
-        ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying    : 0) |
-        (d->is_shut_down                ? XEN_DOMINF_shutdown : 0) |
-        (d->controller_pause_count > 0  ? XEN_DOMINF_paused   : 0) |
-        (d->debugger_attached           ? XEN_DOMINF_debugged : 0) |
+        ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
+        (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
+        (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
+        (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
+        (d->is_xenstore                 ? XEN_DOMINF_xs_domain : 0) |
         d->shutdown_code << XEN_DOMINF_shutdownshift;
 
     switch ( d->guest_type )
@@ -551,7 +552,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                | XEN_DOMCTL_CDF_pvh_guest
                | XEN_DOMCTL_CDF_hap
                | XEN_DOMCTL_CDF_s3_integrity
-               | XEN_DOMCTL_CDF_oos_off)) )
+               | XEN_DOMCTL_CDF_oos_off
+               | XEN_DOMCTL_CDF_xs_domain)) )
             break;
 
         dom = op->domain;
@@ -593,6 +595,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             domcr_flags |= DOMCRF_s3_integrity;
         if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
             domcr_flags |= DOMCRF_oos_off;
+        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
+            domcr_flags |= DOMCRF_xs_domain;
 
         d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7a56b3f..2d8076c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -63,6 +63,9 @@ struct xen_domctl_createdomain {
  /* Is this a PVH guest (as opposed to an HVM or PV guest)? */
 #define _XEN_DOMCTL_CDF_pvh_guest     4
 #define XEN_DOMCTL_CDF_pvh_guest      (1U<<_XEN_DOMCTL_CDF_pvh_guest)
+ /* Is this a xenstore domain? */
+#define _XEN_DOMCTL_CDF_xs_domain     5
+#define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
     uint32_t flags;
     struct xen_arch_domainconfig config;
 };
@@ -97,6 +100,9 @@ struct xen_domctl_getdomaininfo {
 /* domain is PVH */
 #define _XEN_DOMINF_pvh_guest 7
 #define XEN_DOMINF_pvh_guest  (1U<<_XEN_DOMINF_pvh_guest)
+/* domain is a xenstore domain */
+#define _XEN_DOMINF_xs_domain 8
+#define XEN_DOMINF_xs_domain  (1U<<_XEN_DOMINF_xs_domain)
  /* XEN_DOMINF_shutdown guest-supplied code.  */
 #define XEN_DOMINF_shutdownmask 255
 #define XEN_DOMINF_shutdownshift 16
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index fc61fc3..c9a4fc0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -374,6 +374,8 @@ struct domain
     bool_t           auto_node_affinity;
     /* Is this guest fully privileged (aka dom0)? */
     bool_t           is_privileged;
+    /* Is this a xenstore domain (not dom0)? */
+    bool_t           is_xenstore;
     /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
     bool_t           is_pinned;
     /* Non-migratable and non-restoreable? */
@@ -533,6 +535,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
  /* DOMCRF_pvh: Create PV domain in HVM container. */
 #define _DOMCRF_pvh             5
 #define DOMCRF_pvh              (1U<<_DOMCRF_pvh)
+ /* DOMCRF_xs_domain: xenstore domain */
+#define _DOMCRF_xs_domain       6
+#define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 81fba40..9803ff4 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -71,6 +71,10 @@ static always_inline int xsm_default_action(
         if ( src->is_privileged )
             return 0;
         return -EPERM;
+    case XSM_XS_PRIV:
+        if ( src->is_xenstore || src->is_privileged )
+            return 0;
+        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;
@@ -123,6 +127,8 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd)
     case XEN_DOMCTL_bind_pt_irq:
     case XEN_DOMCTL_unbind_pt_irq:
         return xsm_default_action(XSM_DM_PRIV, current->domain, d);
+    case XEN_DOMCTL_getdomaininfo:
+        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
     default:
         return xsm_default_action(XSM_PRIV, current->domain, d);
     }
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 3fc3824..6304eb9 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -35,6 +35,7 @@ enum xsm_default {
     XSM_DM_PRIV,  /* Device model can perform on its target domain */
     XSM_TARGET,   /* Can perform on self or your target domain */
     XSM_PRIV,     /* Privileged - normally restricted to dom0 */
+    XSM_XS_PRIV,  /* Xenstore domain can obtain domain info */
     XSM_OTHER     /* Something more complex */
 };
 typedef enum xsm_default xsm_default_t;
-- 
2.6.2

[PATCH v2 02/13] libxc: support new xenstore domain flag in libxc

[Juergen Gross]

Support the xenstore domain flag for obtaining domain info.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_domain.c       | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 01a6dda..45d8ff6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -456,7 +456,7 @@ typedef struct xc_dominfo {
     uint32_t      ssidref;
     unsigned int  dying:1, crashed:1, shutdown:1,
                   paused:1, blocked:1, running:1,
-                  hvm:1, debugged:1, pvh:1;
+                  hvm:1, debugged:1, pvh:1, xs_domain:1;
     unsigned int  shutdown_reason; /* only meaningful if shutdown==1 */
     unsigned long nr_pages; /* current number, not maximum */
     unsigned long nr_outstanding_pages;
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 96506d5..46bca92 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -366,14 +366,15 @@ int xc_domain_getinfo(xc_interface *xch,
             break;
         info->domid      = (uint16_t)domctl.domain;
 
-        info->dying    = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_dying);
-        info->shutdown = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_shutdown);
-        info->paused   = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_paused);
-        info->blocked  = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_blocked);
-        info->running  = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_running);
-        info->hvm      = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_hvm_guest);
-        info->debugged = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_debugged);
-        info->pvh      = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_pvh_guest);
+        info->dying     = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_dying);
+        info->shutdown  = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_shutdown);
+        info->paused    = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_paused);
+        info->blocked   = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_blocked);
+        info->running   = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_running);
+        info->hvm       = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_hvm_guest);
+        info->debugged  = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_debugged);
+        info->pvh       = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_pvh_guest);
+        info->xs_domain = !!(domctl.u.getdomaininfo.flags&XEN_DOMINF_xs_domain);
 
         info->shutdown_reason =
             (domctl.u.getdomaininfo.flags>>XEN_DOMINF_shutdownshift) &
-- 
2.6.2

[PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

Add libxl_xenstore_domid() to obtain the domain id of the xenstore
domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl.c | 24 ++++++++++++++++++++++++
 tools/libxl/libxl.h | 11 +++++++++++
 2 files changed, 35 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..3bcff59 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -701,6 +701,30 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
     return 0;
 }
 
+int libxl_xenstore_domid(libxl_ctx *ctx, uint32_t *domid)
+{
+    xc_dominfo_t info;
+    uint32_t last_domid;
+    int ret;
+    GC_INIT(ctx);
+
+    for (last_domid = 0;
+         (ret = xc_domain_getinfo(ctx->xch, last_domid, 1, &info)) == 1;
+         last_domid = info.domid + 1)
+    {
+        if (info.xs_domain)
+        {
+            *domid = info.domid;
+            ret = 0;
+            goto out;
+        }
+    }
+    ret = (ret == 0) ? ERROR_DOMAIN_NOTFOUND : ERROR_FAIL;
+out:
+    GC_FREE;
+    return ret;
+}
+
 /* Returns:
  *   0 - success
  *   ERROR_FAIL + errno == ENOENT - no entry found
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 05606a7..41e9d88 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -867,6 +867,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  */
 #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1
 
+/*
+ * LIBXL_HAVE_XENSTORE_DOMID
+ *
+ * If this is defined, then libxl_xenstore_domid is available.
+ */
+#define LIBXL_HAVE_XENSTORE_DOMID
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1306,6 +1313,10 @@ int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
  */
 int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
 
+/* Save domid of a possible Xenstore domain. If no Xenstore domain exists,
+ * ERROR_DOMAIN_NOTFOUND is returned. */
+int libxl_xenstore_domid(libxl_ctx *ctx, uint32_t *domid);
+
 /* May be called with info_r == NULL to check for domain's existence.
  * Returns ERROR_DOMAIN_NOTFOUND if domain does not exist (used to return
  * ERROR_INVAL for this scenario). */
-- 
2.6.2

[PATCH v2 04/13] xenstore: move init-xenstore-domain to tools/helpers

[Juergen Gross]

The program init-xenstore-domain to start a xenstore domain instead
of the xenstored daemon is built, but not installed. Change that.

Move the program to a new tools subdirectory "helpers" to be able to
use libxl in a later patch. Otherwise a dependency loop will be
created.

While moving modify the coding style to the standard style used in
the tools directory.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/Makefile                        |   1 +
 tools/helpers/Makefile                |  34 +++++++++
 tools/helpers/init-xenstore-domain.c  | 128 ++++++++++++++++++++++++++++++++++
 tools/xenstore/Makefile               |   9 ---
 tools/xenstore/init-xenstore-domain.c | 119 -------------------------------
 5 files changed, 163 insertions(+), 128 deletions(-)
 create mode 100644 tools/helpers/Makefile
 create mode 100644 tools/helpers/init-xenstore-domain.c
 delete mode 100644 tools/xenstore/init-xenstore-domain.c

diff --git a/tools/Makefile b/tools/Makefile
index 820ca40..3b15ece 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -29,6 +29,7 @@ endif
 
 SUBDIRS-y += xenpmd
 SUBDIRS-y += libxl
+SUBDIRS-y += helpers
 SUBDIRS-$(CONFIG_X86) += xenpaging
 SUBDIRS-$(CONFIG_X86) += debugger/gdbsx
 SUBDIRS-$(CONFIG_X86) += debugger/kdd
diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
new file mode 100644
index 0000000..52347fd
--- /dev/null
+++ b/tools/helpers/Makefile
@@ -0,0 +1,34 @@
+#
+# tools/helpers/Makefile
+#
+
+XEN_ROOT = $(CURDIR)/../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+ifeq ($(CONFIG_Linux),y)
+PROGS += init-xenstore-domain
+endif
+
+INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o
+$(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenguest)
+$(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+
+.PHONY: all
+all: $(PROGS)
+
+init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(APPEND_LDFLAGS)
+
+.PHONY: install
+install: all
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+ifeq ($(CONFIG_Linux),y)
+	$(INSTALL_PROG) init-xenstore-domain $(DESTDIR)$(LIBEXEC_BIN)
+endif
+
+.PHONY: clean
+clean:
+	$(RM) -f *.o $(PROGS) $(DEPS)
+
+distclean: clean
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
new file mode 100644
index 0000000..3dd1255
--- /dev/null
+++ b/tools/helpers/init-xenstore-domain.c
@@ -0,0 +1,128 @@
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <xenctrl.h>
+#include <xc_dom.h>
+#include <xenstore.h>
+#include <xen/sys/xenbus_dev.h>
+
+static uint32_t domid = -1;
+
+static int build(xc_interface *xch, int argc, char** argv)
+{
+    char cmdline[512];
+    uint32_t ssid;
+    xen_domain_handle_t handle = { 0 };
+    int rv, xs_fd;
+    struct xc_dom_image *dom = NULL;
+    int maxmem = atoi(argv[2]);
+    int limit_kb = (maxmem + 1)*1024;
+
+    xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
+    if (xs_fd == -1) return -1;
+
+    rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid);
+    if (rv) goto err;
+    rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
+    if (rv) goto err;
+    rv = xc_domain_max_vcpus(xch, domid, 1);
+    if (rv) goto err;
+    rv = xc_domain_setmaxmem(xch, domid, limit_kb);
+    if (rv) goto err;
+    rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
+    if (rv) goto err;
+
+    rv = ioctl(xs_fd, IOCTL_XENBUS_BACKEND_SETUP, domid);
+    if (rv < 0) goto err;
+    snprintf(cmdline, 512, "--event %d --internal-db", rv);
+
+    dom = xc_dom_allocate(xch, cmdline, NULL);
+    rv = xc_dom_kernel_file(dom, argv[1]);
+    if (rv) goto err;
+
+    if (argc > 4) {
+        rv = xc_dom_ramdisk_file(dom, argv[4]);
+        if (rv) goto err;
+    }
+
+    rv = xc_dom_boot_xen_init(dom, xch, domid);
+    if (rv) goto err;
+    rv = xc_dom_parse_image(dom);
+    if (rv) goto err;
+    rv = xc_dom_mem_init(dom, maxmem);
+    if (rv) goto err;
+    rv = xc_dom_boot_mem_init(dom);
+    if (rv) goto err;
+    rv = xc_dom_build_image(dom);
+    if (rv) goto err;
+    rv = xc_dom_boot_image(dom);
+    if (rv) goto err;
+
+    xc_dom_release(dom);
+    dom = NULL;
+
+    rv = xc_domain_set_virq_handler(xch, domid, VIRQ_DOM_EXC);
+    if (rv) goto err;
+    rv = xc_domain_unpause(xch, domid);
+    if (rv) goto err;
+
+    return 0;
+
+err:
+    if (dom)
+        xc_dom_release(dom);
+    close(xs_fd);
+    return rv;
+}
+
+int main(int argc, char** argv)
+{
+    xc_interface *xch;
+    struct xs_handle *xsh;
+    char buf[16];
+    int rv, fd;
+
+    if (argc < 4 || argc > 5) {
+        printf("Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n", argv[0]);
+        return 2;
+    }
+
+    xch = xc_interface_open(NULL, NULL, 0);
+    if (!xch) return 1;
+
+    rv = build(xch, argc, argv);
+
+    xc_interface_close(xch);
+
+    if (rv)
+        return 1;
+
+    xsh = xs_open(0);
+    rv = snprintf(buf, 16, "%d", domid);
+    xs_write(xsh, XBT_NULL, "/tool/xenstored/domid", buf, rv);
+    xs_daemon_close(xsh);
+
+    fd = creat("/var/run/xenstored.pid", 0666);
+    if (fd < 0)
+        return 3;
+    rv = snprintf(buf, 16, "domid:%d\n", domid);
+    rv = write(fd, buf, rv);
+    close(fd);
+    if (rv < 0)
+        return 3;
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 1b4a494..404d4cb 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -46,10 +46,6 @@ ifeq ($(XENSTORE_XENSTORED),y)
 ALL_TARGETS += xs_tdb_dump xenstored
 endif
 
-ifeq ($(CONFIG_Linux),y)
-ALL_TARGETS += init-xenstore-domain
-endif
-
 ifdef CONFIG_STUBDOM
 CFLAGS += -DNO_SOCKETS=1
 endif
@@ -72,11 +68,6 @@ xenstored_probes.o: xenstored_solaris.o
 CFLAGS += -DHAVE_DTRACE=1
 endif
 
-init-xenstore-domain.o: CFLAGS += $(CFLAGS_libxenguest)
-
-init-xenstore-domain: init-xenstore-domain.o $(LIBXENSTORE)
-	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) -o $@ $(APPEND_LDFLAGS)
-
 xenstored: $(XENSTORED_OBJS)
 	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenctrl) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
 
diff --git a/tools/xenstore/init-xenstore-domain.c b/tools/xenstore/init-xenstore-domain.c
deleted file mode 100644
index 297afe5..0000000
--- a/tools/xenstore/init-xenstore-domain.c
+++ /dev/null
@@ -1,119 +0,0 @@
-#include <fcntl.h>
-#include <unistd.h>
-#include <stdio.h>
-#include <string.h>
-#include <stdint.h>
-#include <stdlib.h>
-#include <sys/ioctl.h>
-#include <sys/mman.h>
-#include <xenctrl.h>
-#include <xc_dom.h>
-#include <xenstore.h>
-#include <xen/sys/xenbus_dev.h>
-
-static uint32_t domid = -1;
-
-static int build(xc_interface *xch, int argc, char** argv)
-{
-	char cmdline[512];
-	uint32_t ssid;
-	xen_domain_handle_t handle = { 0 };
-	int rv, xs_fd;
-	struct xc_dom_image *dom = NULL;
-	int maxmem = atoi(argv[2]);
-	int limit_kb = (maxmem + 1)*1024;
-
-	xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
-	if (xs_fd == -1) return -1;
-
-	rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid);
-	if (rv) goto err;
-	rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
-	if (rv) goto err;
-	rv = xc_domain_max_vcpus(xch, domid, 1);
-	if (rv) goto err;
-	rv = xc_domain_setmaxmem(xch, domid, limit_kb);
-	if (rv) goto err;
-	rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
-	if (rv) goto err;
-
-	rv = ioctl(xs_fd, IOCTL_XENBUS_BACKEND_SETUP, domid);
-	if (rv < 0) goto err;
-	snprintf(cmdline, 512, "--event %d --internal-db", rv);
-
-	dom = xc_dom_allocate(xch, cmdline, NULL);
-	rv = xc_dom_kernel_file(dom, argv[1]);
-	if (rv) goto err;
-
-	if (argc > 4) {
-		rv = xc_dom_ramdisk_file(dom, argv[4]);
-		if (rv) goto err;
-	}
-
-	rv = xc_dom_boot_xen_init(dom, xch, domid);
-	if (rv) goto err;
-	rv = xc_dom_parse_image(dom);
-	if (rv) goto err;
-	rv = xc_dom_mem_init(dom, maxmem);
-	if (rv) goto err;
-	rv = xc_dom_boot_mem_init(dom);
-	if (rv) goto err;
-	rv = xc_dom_build_image(dom);
-	if (rv) goto err;
-	rv = xc_dom_boot_image(dom);
-	if (rv) goto err;
-
-	xc_dom_release(dom);
-	dom = NULL;
-
-	rv = xc_domain_set_virq_handler(xch, domid, VIRQ_DOM_EXC);
-	if (rv) goto err;
-	rv = xc_domain_unpause(xch, domid);
-	if (rv) goto err;
-
-	return 0;
-
-err:
-	if (dom)
-		xc_dom_release(dom);
-	close(xs_fd);
-	return rv;
-}
-
-int main(int argc, char** argv)
-{
-	xc_interface *xch;
-	struct xs_handle *xsh;
-	char buf[16];
-	int rv, fd;
-
-	if (argc < 4 || argc > 5) {
-		printf("Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n", argv[0]);
-		return 2;
-	}
-
-	xch = xc_interface_open(NULL, NULL, 0);
-	if (!xch) return 1;
-
-	rv = build(xch, argc, argv);
-
-	xc_interface_close(xch);
-
-	if (rv) return 1;
-
-	xsh = xs_open(0);
-	rv = snprintf(buf, 16, "%d", domid);
-	xs_write(xsh, XBT_NULL, "/tool/xenstored/domid", buf, rv);
-	xs_daemon_close(xsh);
-
-	fd = creat("/var/run/xenstored.pid", 0666);
-	if (fd < 0)
-		return 3;
-	rv = snprintf(buf, 16, "domid:%d\n", domid);
-	rv = write(fd, buf, rv);
-	close(fd);
-	if (rv < 0)
-		return 3;
-
-	return 0;
-}
-- 
2.6.2

[PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Juergen Gross]

Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
helper program.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/Makefile                   | 10 ++++++++++
 tools/{libxl => helpers}/xen-init-dom0.c |  0
 tools/libxl/Makefile                     | 14 +++-----------
 3 files changed, 13 insertions(+), 11 deletions(-)
 rename tools/{libxl => helpers}/xen-init-dom0.c (100%)

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 52347fd..92aead4 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -5,10 +5,16 @@
 XEN_ROOT = $(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
+PROGS += xen-init-dom0
 ifeq ($(CONFIG_Linux),y)
 PROGS += init-xenstore-domain
 endif
 
+XEN_INIT_DOM0_OBJS = xen-init-dom0.o
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
+
 INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenguest)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
@@ -17,12 +23,16 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 .PHONY: all
 all: $(PROGS)
 
+xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
 init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(LIBEXEC_BIN)
 ifeq ($(CONFIG_Linux),y)
 	$(INSTALL_PROG) init-xenstore-domain $(DESTDIR)$(LIBEXEC_BIN)
 endif
diff --git a/tools/libxl/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
similarity index 100%
rename from tools/libxl/xen-init-dom0.c
rename to tools/helpers/xen-init-dom0.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 6ff5bee..6a913c8 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -142,7 +142,7 @@ LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
 	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
-CLIENTS = xl testidl libxl-save-helper xen-init-dom0
+CLIENTS = xl testidl libxl-save-helper
 
 CFLAGS_XL += $(CFLAGS_libxenlight)
 CFLAGS_XL += -Wshadow
@@ -153,10 +153,6 @@ $(XL_OBJS) $(TEST_PROG_OBJS) _libxl.api-for-check: \
 $(XL_OBJS): CFLAGS += $(CFLAGS_XL)
 $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h needs it.
 
-XEN_INIT_DOM0_OBJS = xen-init-dom0.o
-$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
-$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
-
 SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o
 $(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
 
@@ -173,7 +169,7 @@ all: $(CLIENTS) $(TEST_PROGS) $(PKG_CONFIG) \
 	$(AUTOSRCS) $(AUTOINCS)
 
 $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS) $(SAVE_HELPER_OBJS) \
-		$(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS) $(XEN_INIT_DOM0_OBJS): \
+		$(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS): \
 	$(AUTOINCS) libxl.api-ok
 
 %.c %.h:: %.y
@@ -214,7 +210,7 @@ libxl_internal_json.h: _libxl_types_internal_json.h
 xl.h: _paths.h
 
 $(LIBXL_OBJS) $(LIBXL_TEST_OBJS) $(LIBXLU_OBJS) \
-	$(XL_OBJS) $(TEST_PROG_OBJS) $(SAVE_HELPER_OBJS) $(XEN_INIT_DOM0_OBJS): libxl.h
+	$(XL_OBJS) $(TEST_PROG_OBJS) $(SAVE_HELPER_OBJS): libxl.h
 $(LIBXL_OBJS) $(LIBXL_TEST_OBJS): libxl_internal.h
 
 _libxl_type%.h _libxl_type%_json.h _libxl_type%_private.h _libxl_type%.c: libxl_type%.idl gentypes.py idl.py
@@ -255,9 +251,6 @@ libxlutil.a: $(LIBXLU_OBJS)
 xl: $(XL_OBJS) libxlutil.so libxenlight.so
 	$(CC) $(LDFLAGS) -o $@ $(XL_OBJS) libxlutil.so $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
 
-xen-init-dom0: $(XEN_INIT_DOM0_OBJS) libxenlight.so
-	$(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
-
 test_%: test_%.o test_common.o libxlutil.so libxenlight_test.so
 	$(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
 
@@ -280,7 +273,6 @@ install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_DIR) $(DESTDIR)$(SHAREDIR)/pkgconfig
 	$(INSTALL_PROG) xl $(DESTDIR)$(sbindir)
-	$(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) libxl-save-helper $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_SHLIB) libxenlight.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)
 	$(SYMLINK_SHLIB) libxenlight.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)/libxenlight.so.$(MAJOR)
-- 
2.6.2

[PATCH v2 06/13] xenstore: destroy xenstore domain in case of error after creating it

[Juergen Gross]

When creating a xenstore domain via init-xenstore-domain destroy it
in case of an error occurred after calling xc_domain_create().

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/helpers/init-xenstore-domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 3dd1255..6dffaef 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -11,7 +11,7 @@
 #include <xenstore.h>
 #include <xen/sys/xenbus_dev.h>
 
-static uint32_t domid = -1;
+static uint32_t domid = ~0;
 
 static int build(xc_interface *xch, int argc, char** argv)
 {
@@ -76,6 +76,8 @@ static int build(xc_interface *xch, int argc, char** argv)
 err:
     if (dom)
         xc_dom_release(dom);
+    if (domid != ~0)
+        xc_domain_destroy(xch, domid);
     close(xs_fd);
     return rv;
 }
-- 
2.6.2

[PATCH v2 07/13] xenstore: add error messages to init-xenstore-domain

[Juergen Gross]

Additional add some diagnostic messages to the program to have an idea
why it failed.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/helpers/init-xenstore-domain.c | 104 ++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 21 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 6dffaef..d3abec0 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -10,6 +10,7 @@
 #include <xc_dom.h>
 #include <xenstore.h>
 #include <xen/sys/xenbus_dev.h>
+#include <xen-xsm/flask/flask.h>
 
 static uint32_t domid = ~0;
 
@@ -24,52 +25,103 @@ static int build(xc_interface *xch, int argc, char** argv)
     int limit_kb = (maxmem + 1)*1024;
 
     xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
-    if (xs_fd == -1) return -1;
+    if (xs_fd == -1) {
+        fprintf(stderr, "Could not open /dev/xen/xenbus_backend\n");
+        return -1;
+    }
 
     rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_flask_context_to_sid failed\n");
+        goto err;
+    }
     rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_create failed\n");
+        goto err;
+    }
     rv = xc_domain_max_vcpus(xch, domid, 1);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_max_vcpus failed\n");
+        goto err;
+    }
     rv = xc_domain_setmaxmem(xch, domid, limit_kb);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_setmaxmem failed\n");
+        goto err;
+    }
     rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_set_memmap_limit failed\n");
+        goto err;
+    }
 
     rv = ioctl(xs_fd, IOCTL_XENBUS_BACKEND_SETUP, domid);
-    if (rv < 0) goto err;
+    if (rv < 0) {
+        fprintf(stderr, "Xenbus setup ioctl failed\n");
+        goto err;
+    }
     snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
     dom = xc_dom_allocate(xch, cmdline, NULL);
     rv = xc_dom_kernel_file(dom, argv[1]);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_kernel_file failed\n");
+        goto err;
+    }
 
     if (argc > 4) {
         rv = xc_dom_ramdisk_file(dom, argv[4]);
-        if (rv) goto err;
+        if (rv) {
+            fprintf(stderr, "xc_dom_ramdisk_file failed\n");
+            goto err;
+        }
     }
 
     rv = xc_dom_boot_xen_init(dom, xch, domid);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_boot_xen_init failed\n");
+        goto err;
+    }
     rv = xc_dom_parse_image(dom);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_parse_image failed\n");
+        goto err;
+    }
     rv = xc_dom_mem_init(dom, maxmem);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_mem_init failed\n");
+        goto err;
+    }
     rv = xc_dom_boot_mem_init(dom);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_boot_mem_init failed\n");
+        goto err;
+    }
     rv = xc_dom_build_image(dom);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_build_image failed\n");
+        goto err;
+    }
     rv = xc_dom_boot_image(dom);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_dom_boot_image failed\n");
+        goto err;
+    }
 
     xc_dom_release(dom);
     dom = NULL;
 
     rv = xc_domain_set_virq_handler(xch, domid, VIRQ_DOM_EXC);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_set_virq_handler failed\n");
+        goto err;
+    }
     rv = xc_domain_unpause(xch, domid);
-    if (rv) goto err;
+    if (rv) {
+        fprintf(stderr, "xc_domain_unpause failed\n");
+        goto err;
+    }
 
     return 0;
 
@@ -90,12 +142,17 @@ int main(int argc, char** argv)
     int rv, fd;
 
     if (argc < 4 || argc > 5) {
-        printf("Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n", argv[0]);
+        fprintf(stderr,
+            "Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n",
+            argv[0]);
         return 2;
     }
 
     xch = xc_interface_open(NULL, NULL, 0);
-    if (!xch) return 1;
+    if (!xch) {
+        fprintf(stderr, "xc_interface_open() failed\n");
+        return 1;
+    }
 
     rv = build(xch, argc, argv);
 
@@ -110,13 +167,18 @@ int main(int argc, char** argv)
     xs_daemon_close(xsh);
 
     fd = creat("/var/run/xenstored.pid", 0666);
-    if (fd < 0)
+    if (fd < 0) {
+        fprintf(stderr, "Creating /var/run/xenstored.pid failed\n");
         return 3;
+    }
     rv = snprintf(buf, 16, "domid:%d\n", domid);
     rv = write(fd, buf, rv);
     close(fd);
-    if (rv < 0)
+    if (rv < 0) {
+        fprintf(stderr,
+            "Writing domid to /var/run/xenstored.pid failed\n");
         return 3;
+    }
 
     return 0;
 }
-- 
2.6.2

[PATCH v2 08/13] xenstore: modify init-xenstore-domain parameter syntax

[Juergen Gross]

init-xenstore-domain takes only positional parameters today. Change
this to a more flexible parameter syntax allowing to specify additional
options or to omit some.

Today the supported usage is:

init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label>
                     [<ramdisk-file>]

Modify this to:

init-xenstore-domain --kernel <xenstore-kernel>
                     --memory <memory_mb>
                     [--flask <flask-label>]
                     [--ramdisk <ramdisk-file>]

The flask label is made optional in order to support xenstore domains
without the need of a flask enabled hypervisor.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/init-xenstore-domain.c | 82 +++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 16 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index d3abec0..17f0151 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -4,6 +4,7 @@
 #include <string.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <getopt.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <xenctrl.h>
@@ -13,16 +14,42 @@
 #include <xen-xsm/flask/flask.h>
 
 static uint32_t domid = ~0;
+static char *kernel;
+static char *ramdisk;
+static char *flask;
+static int memory;
+
+static struct option options[] = {
+    { "kernel", 1, NULL, 'k' },
+    { "memory", 1, NULL, 'm' },
+    { "flask", 1, NULL, 'f' },
+    { "ramdisk", 1, NULL, 'r' },
+    { NULL, 0, NULL, 0 }
+};
+
+static void usage(void)
+{
+    fprintf(stderr,
+"Usage:\n"
+"\n"
+"init-xenstore-domain <options>\n"
+"\n"
+"where options may include:\n"
+"\n"
+"  --kernel <xenstore-kernel> kernel file of the xenstore domain, mandatory\n"
+"  --memory <memory size>     size of the domain in MB, mandatory\n"
+"  --flask <flask-label>      optional flask label of the domain\n"
+"  --ramdisk <ramdisk-file>   optional ramdisk file for the domain\n");
+}
 
-static int build(xc_interface *xch, int argc, char** argv)
+static int build(xc_interface *xch)
 {
     char cmdline[512];
     uint32_t ssid;
     xen_domain_handle_t handle = { 0 };
     int rv, xs_fd;
     struct xc_dom_image *dom = NULL;
-    int maxmem = atoi(argv[2]);
-    int limit_kb = (maxmem + 1)*1024;
+    int limit_kb = (memory + 1)*1024;
 
     xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
     if (xs_fd == -1) {
@@ -30,10 +57,14 @@ static int build(xc_interface *xch, int argc, char** argv)
         return -1;
     }
 
-    rv = xc_flask_context_to_sid(xch, argv[3], strlen(argv[3]), &ssid);
-    if (rv) {
-        fprintf(stderr, "xc_flask_context_to_sid failed\n");
-        goto err;
+    if (flask) {
+        rv = xc_flask_context_to_sid(xch, flask, strlen(flask), &ssid);
+        if (rv) {
+            fprintf(stderr, "xc_flask_context_to_sid failed\n");
+            goto err;
+        }
+    } else {
+        ssid = SECINITSID_DOMU;
     }
     rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
     if (rv) {
@@ -64,14 +95,14 @@ static int build(xc_interface *xch, int argc, char** argv)
     snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
     dom = xc_dom_allocate(xch, cmdline, NULL);
-    rv = xc_dom_kernel_file(dom, argv[1]);
+    rv = xc_dom_kernel_file(dom, kernel);
     if (rv) {
         fprintf(stderr, "xc_dom_kernel_file failed\n");
         goto err;
     }
 
-    if (argc > 4) {
-        rv = xc_dom_ramdisk_file(dom, argv[4]);
+    if (ramdisk) {
+        rv = xc_dom_ramdisk_file(dom, ramdisk);
         if (rv) {
             fprintf(stderr, "xc_dom_ramdisk_file failed\n");
             goto err;
@@ -88,7 +119,7 @@ static int build(xc_interface *xch, int argc, char** argv)
         fprintf(stderr, "xc_dom_parse_image failed\n");
         goto err;
     }
-    rv = xc_dom_mem_init(dom, maxmem);
+    rv = xc_dom_mem_init(dom, memory);
     if (rv) {
         fprintf(stderr, "xc_dom_mem_init failed\n");
         goto err;
@@ -136,15 +167,34 @@ err:
 
 int main(int argc, char** argv)
 {
+    int opt;
     xc_interface *xch;
     struct xs_handle *xsh;
     char buf[16];
     int rv, fd;
 
-    if (argc < 4 || argc > 5) {
-        fprintf(stderr,
-            "Use: %s <xenstore-kernel> <memory_mb> <flask-label> [<ramdisk-file>]\n",
-            argv[0]);
+    while ((opt = getopt_long(argc, argv, "", options, NULL)) != -1) {
+        switch (opt) {
+        case 'k':
+            kernel = optarg;
+            break;
+        case 'm':
+            memory = strtol(optarg, NULL, 10);
+            break;
+        case 'f':
+            flask = optarg;
+            break;
+        case 'r':
+            ramdisk = optarg;
+            break;
+        default:
+            usage();
+            return 2;
+        }
+    }
+
+    if (optind != argc || !kernel || !memory) {
+        usage();
         return 2;
     }
 
@@ -154,7 +204,7 @@ int main(int argc, char** argv)
         return 1;
     }
 
-    rv = build(xch, argc, argv);
+    rv = build(xch);
 
     xc_interface_close(xch);
 
-- 
2.6.2

[PATCH v2 09/13] xenstore: make use of the "xenstore domain" flag

[Juergen Gross]

Create the xenstore domain with the xs_domain flag specified. This
enables us to test whether such a domain is already running before
we create it. As there ought to be only one xenstore in the system
we don't need to start another one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/init-xenstore-domain.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 17f0151..4cd9e0f 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -66,7 +66,8 @@ static int build(xc_interface *xch)
     } else {
         ssid = SECINITSID_DOMU;
     }
-    rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
+    rv = xc_domain_create(xch, ssid, handle, XEN_DOMCTL_CDF_xs_domain,
+                  &domid, NULL);
     if (rv) {
         fprintf(stderr, "xc_domain_create failed\n");
         goto err;
@@ -165,6 +166,21 @@ err:
     return rv;
 }
 
+static int check_domain(xc_interface *xch)
+{
+    xc_dominfo_t info;
+    uint32_t dom;
+
+    dom = 0;
+    while (xc_domain_getinfo(xch, dom, 1, &info) == 1) {
+        if (info.xs_domain)
+            return 1;
+        dom = info.domid + 1;
+    }
+
+    return 0;
+}
+
 int main(int argc, char** argv)
 {
     int opt;
@@ -204,7 +220,12 @@ int main(int argc, char** argv)
         return 1;
     }
 
-    rv = build(xch);
+    rv = check_domain(xch);
+
+    if (!rv)
+        rv = build(xch);
+    else
+        fprintf(stderr, "xenstore domain already present.\n");
 
     xc_interface_close(xch);
 
-- 
2.6.2

[PATCH v2 10/13] xenstore: add init-xenstore-domain parameter to specify cmdline

[Juergen Gross]

Add a parameter to init-xenstore-domain for support of arbitrary
parameters for the xenstore domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/helpers/init-xenstore-domain.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 4cd9e0f..7d875db 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -17,6 +17,7 @@ static uint32_t domid = ~0;
 static char *kernel;
 static char *ramdisk;
 static char *flask;
+static char *param;
 static int memory;
 
 static struct option options[] = {
@@ -24,6 +25,7 @@ static struct option options[] = {
     { "memory", 1, NULL, 'm' },
     { "flask", 1, NULL, 'f' },
     { "ramdisk", 1, NULL, 'r' },
+    { "param", 1, NULL, 'p' },
     { NULL, 0, NULL, 0 }
 };
 
@@ -39,7 +41,8 @@ static void usage(void)
 "  --kernel <xenstore-kernel> kernel file of the xenstore domain, mandatory\n"
 "  --memory <memory size>     size of the domain in MB, mandatory\n"
 "  --flask <flask-label>      optional flask label of the domain\n"
-"  --ramdisk <ramdisk-file>   optional ramdisk file for the domain\n");
+"  --ramdisk <ramdisk-file>   optional ramdisk file for the domain\n"
+"  --param <cmdline>          optional additional parameters for the domain\n");
 }
 
 static int build(xc_interface *xch)
@@ -93,7 +96,12 @@ static int build(xc_interface *xch)
         fprintf(stderr, "Xenbus setup ioctl failed\n");
         goto err;
     }
-    snprintf(cmdline, 512, "--event %d --internal-db", rv);
+
+    if (param)
+        snprintf(cmdline, 512, "--event %d --internal-db %s", rv,
+             param);
+    else
+        snprintf(cmdline, 512, "--event %d --internal-db", rv);
 
     dom = xc_dom_allocate(xch, cmdline, NULL);
     rv = xc_dom_kernel_file(dom, kernel);
@@ -203,6 +211,9 @@ int main(int argc, char** argv)
         case 'r':
             ramdisk = optarg;
             break;
+        case 'p':
+            param = optarg;
+            break;
         default:
             usage();
             return 2;
-- 
2.6.2

[PATCH v2 11/13] tools: split up xen-init-dom0.c

[Juergen Gross]

Split up tools/helpers/xen-init-dom0.c in order to prepare reusing
generation of the json configuration by init-xenstore-domain.c.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/Makefile        |  2 +-
 tools/helpers/init-dom-json.c | 59 ++++++++++++++++++++++++++++++++++
 tools/helpers/init-dom-json.h | 18 +++++++++++
 tools/helpers/xen-init-dom0.c | 73 ++++++-------------------------------------
 4 files changed, 88 insertions(+), 64 deletions(-)
 create mode 100644 tools/helpers/init-dom-json.c
 create mode 100644 tools/helpers/init-dom-json.h

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 92aead4..cfb86f5 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -10,7 +10,7 @@ ifeq ($(CONFIG_Linux),y)
 PROGS += init-xenstore-domain
 endif
 
-XEN_INIT_DOM0_OBJS = xen-init-dom0.o
+XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
 $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
 $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
diff --git a/tools/helpers/init-dom-json.c b/tools/helpers/init-dom-json.c
new file mode 100644
index 0000000..91b1fdf
--- /dev/null
+++ b/tools/helpers/init-dom-json.c
@@ -0,0 +1,59 @@
+#include <err.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <stdio.h>
+
+#include <xenctrl.h>
+#include <libxl.h>
+
+int gen_stub_json_config(uint32_t domid)
+{
+    int rc = 1;
+    xentoollog_logger_stdiostream *logger;
+    libxl_ctx *ctx;
+    libxl_domain_config dom_config;
+    char *json = NULL;
+
+    logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
+    if (!logger)
+        return 1;
+
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
+                        (xentoollog_logger *)logger)) {
+        fprintf(stderr, "cannot init libxl context\n");
+        goto outlog;
+    }
+
+    libxl_domain_config_init(&dom_config);
+
+    /* Generate stub JSON config. */
+    dom_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
+    libxl_domain_build_info_init_type(&dom_config.b_info,
+                                      LIBXL_DOMAIN_TYPE_PV);
+
+    json = libxl_domain_config_to_json(ctx, &dom_config);
+    /* libxl-json format requires the string ends with '\0'. Code
+     * snippet taken from libxl.
+     */
+    rc = libxl_userdata_store(ctx, domid, "libxl-json",
+                              (const uint8_t *)json,
+                              strlen(json) + 1 /* include '\0' */);
+    if (rc)
+        fprintf(stderr, "cannot store stub json config for domain %u\n", domid);
+
+    libxl_domain_config_dispose(&dom_config);
+    free(json);
+    libxl_ctx_free(ctx);
+outlog:
+    xtl_logger_destroy((xentoollog_logger *)logger);
+    return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/helpers/init-dom-json.h b/tools/helpers/init-dom-json.h
new file mode 100644
index 0000000..58c85df
--- /dev/null
+++ b/tools/helpers/init-dom-json.h
@@ -0,0 +1,18 @@
+#ifndef __INIT_DOM_JSON_H
+#define __INIT_DOM_JSON_H
+
+/*
+ * Generate a stub JSON config for a domain with the given domid.
+ */
+int gen_stub_json_config(uint32_t domid);
+
+#endif
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index 7925d64..9ab8468 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -1,65 +1,26 @@
-#include <err.h>
 #include <stdlib.h>
 #include <stdint.h>
 #include <string.h>
 #include <stdio.h>
 
-#include <xenctrl.h>
 #include <xenstore.h>
-#include <libxl.h>
+
+#include "init-dom-json.h"
 
 #define DOMNAME_PATH   "/local/domain/0/name"
 #define DOMID_PATH     "/local/domain/0/domid"
 
-static libxl_ctx *ctx;
-static xentoollog_logger_stdiostream *logger;
-static struct xs_handle *xsh;
-
-static void ctx_alloc(void)
+int main(int argc, char **argv)
 {
-    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
-                        (xentoollog_logger *)logger)) {
-        fprintf(stderr, "cannot init libxl context\n");
-        exit(1);
-    }
+    int rc;
+    struct xs_handle *xsh;
+    char *domname_string = NULL, *domid_string = NULL;
+
     xsh = xs_open(0);
     if (!xsh) {
         fprintf(stderr, "cannot open xenstore connection\n");
         exit(1);
     }
-}
-
-static void ctx_free(void)
-{
-    if (ctx) {
-        libxl_ctx_free(ctx);
-        ctx = NULL;
-    }
-    if (logger) {
-        xtl_logger_destroy((xentoollog_logger *)logger);
-        logger = NULL;
-    }
-    if (xsh) {
-        xs_close(xsh);
-        xsh = NULL;
-    }
-}
-
-int main(int argc, char **argv)
-{
-    int rc;
-    libxl_domain_config dom0_config;
-    char *domname_string = NULL, *domid_string = NULL;
-    char *json = NULL;;
-
-    logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
-    if (!logger) exit(1);
-
-    atexit(ctx_free);
-
-    ctx_alloc();
-
-    libxl_domain_config_init(&dom0_config);
 
     /* Sanity check: this program can only be run once. */
     domid_string = xs_read(xsh, XBT_NULL, DOMID_PATH, NULL);
@@ -70,22 +31,9 @@ int main(int argc, char **argv)
         goto out;
     }
 
-    /* Generate stub JSON config. */
-    dom0_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
-    libxl_domain_build_info_init_type(&dom0_config.b_info,
-                                      LIBXL_DOMAIN_TYPE_PV);
-
-    json = libxl_domain_config_to_json(ctx, &dom0_config);
-    /* libxl-json format requires the string ends with '\0'. Code
-     * snippet taken from libxl.
-     */
-    rc = libxl_userdata_store(ctx, 0, "libxl-json",
-                              (const uint8_t *)json,
-                              strlen(json) + 1 /* include '\0' */);
-    if (rc) {
-        fprintf(stderr, "cannot store stub json config for Dom0\n");
+    rc = gen_stub_json_config(0);
+    if (rc)
         goto out;
-    }
 
     /* Write xenstore entries. */
     if (!xs_write(xsh, XBT_NULL, DOMID_PATH, "0", strlen("0"))) {
@@ -104,10 +52,9 @@ int main(int argc, char **argv)
     printf("Done setting up Dom0\n");
 
 out:
-    libxl_domain_config_dispose(&dom0_config);
     free(domid_string);
     free(domname_string);
-    free(json);
+    xs_close(xsh);
     return rc;
 }
 
-- 
2.6.2

[PATCH v2 12/13] xenstore: write xenstore domain data to xenstore

[Juergen Gross]

After starting the xenstore domain write the basic data (domid, name
and memory values) to the xenstore. This makes the domain appear
correctly in xl list. Create a stub json object in order to make e.g.
xl list -l happy.

Add a new option to init-xenstore-domain to be able to specify the
domain's name.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/helpers/Makefile               |  5 ++--
 tools/helpers/init-xenstore-domain.c | 44 ++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index cfb86f5..e378120 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -15,10 +15,11 @@ $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
 $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
 
-INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o
+INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o init-dom-json.o
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenguest)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+$(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
 
 .PHONY: all
 all: $(PROGS)
@@ -27,7 +28,7 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
 
 init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
-	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: all
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 7d875db..6238b17 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -13,11 +13,14 @@
 #include <xen/sys/xenbus_dev.h>
 #include <xen-xsm/flask/flask.h>
 
+#include "init-dom-json.h"
+
 static uint32_t domid = ~0;
 static char *kernel;
 static char *ramdisk;
 static char *flask;
 static char *param;
+static char *name = "Xenstore";
 static int memory;
 
 static struct option options[] = {
@@ -26,6 +29,7 @@ static struct option options[] = {
     { "flask", 1, NULL, 'f' },
     { "ramdisk", 1, NULL, 'r' },
     { "param", 1, NULL, 'p' },
+    { "name", 1, NULL, 'n' },
     { NULL, 0, NULL, 0 }
 };
 
@@ -42,7 +46,8 @@ static void usage(void)
 "  --memory <memory size>     size of the domain in MB, mandatory\n"
 "  --flask <flask-label>      optional flask label of the domain\n"
 "  --ramdisk <ramdisk-file>   optional ramdisk file for the domain\n"
-"  --param <cmdline>          optional additional parameters for the domain\n");
+"  --param <cmdline>          optional additional parameters for the domain\n"
+"  --name <name>              name of the domain (default: Xenstore)\n");
 }
 
 static int build(xc_interface *xch)
@@ -189,12 +194,27 @@ static int check_domain(xc_interface *xch)
     return 0;
 }
 
+static void do_xs_write(struct xs_handle *xsh, char *path, char *val)
+{
+    if (!xs_write(xsh, XBT_NULL, path, val, strlen(val)))
+        fprintf(stderr, "writing %s to xenstore failed.\n", path);
+}
+
+static void do_xs_write_dom(struct xs_handle *xsh, char *path, char *val)
+{
+    char full_path[64];
+
+    snprintf(full_path, 64, "/local/domain/%d/%s", domid, path);
+    do_xs_write(xsh, full_path, val);
+}
+
 int main(int argc, char** argv)
 {
     int opt;
     xc_interface *xch;
     struct xs_handle *xsh;
     char buf[16];
+    char path[64];
     int rv, fd;
 
     while ((opt = getopt_long(argc, argv, "", options, NULL)) != -1) {
@@ -214,6 +234,9 @@ int main(int argc, char** argv)
         case 'p':
             param = optarg;
             break;
+        case 'n':
+            name = optarg;
+            break;
         default:
             usage();
             return 2;
@@ -243,10 +266,23 @@ int main(int argc, char** argv)
     if (rv)
         return 1;
 
+    rv = gen_stub_json_config(domid);
+    if (rv)
+        return 3;
+
     xsh = xs_open(0);
-    rv = snprintf(buf, 16, "%d", domid);
-    xs_write(xsh, XBT_NULL, "/tool/xenstored/domid", buf, rv);
-    xs_daemon_close(xsh);
+    if (!xsh) {
+        fprintf(stderr, "xs_open() failed.\n");
+        return 3;
+    }
+    snprintf(buf, 16, "%d", domid);
+    do_xs_write(xsh, "/tool/xenstored/domid", buf);
+    do_xs_write_dom(xsh, "domid", buf);
+    do_xs_write_dom(xsh, "name", name);
+    snprintf(buf, 16, "%d", memory * 1024);
+    do_xs_write_dom(xsh, "memory/target", buf);
+    do_xs_write_dom(xsh, "memory/static-max", buf);
+    xs_close(xsh);
 
     fd = creat("/var/run/xenstored.pid", 0666);
     if (fd < 0) {
-- 
2.6.2

[PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Juergen Gross]

When restarting or shutting down dom0 the xendomains script tries to
stop all other domains. Don't do this for the xenstore domain, as it
might survive a dom0 reboot in the future.

The same applies to xl shutdown --all.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
 tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
index dfe0b33..70b7f16 100644
--- a/tools/hotplug/Linux/xendomains.in
+++ b/tools/hotplug/Linux/xendomains.in
@@ -196,6 +196,17 @@ rdnames()
     done
 }
 
+# set xenstore domain id (or 0 if no xenstore domain)
+get_xsdomid()
+{
+    ${bindir}/xenstore-exists /tool/xenstored/domid
+    if test $? -ne 0; then
+        XS_DOMID=0
+    else
+        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
+    fi
+}
+
 LIST_GREP='(domain\|(domid\|(name\|^    {$\|"name":\|"domid":'
 parseln()
 {
@@ -216,12 +227,14 @@ parseln()
 
 is_running()
 {
+    get_xsdomid
     rdname $1
     RC=1
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	case $name in
 	    ($NM)
 		RC=0
@@ -302,10 +315,12 @@ start()
 
 all_zombies()
 {
+    get_xsdomid
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	if test "$state" != "-b---d" -a "$state" != "-----d"; then
 	    return 1;
 	fi
@@ -351,11 +366,13 @@ stop()
     if test "$XENDOMAINS_AUTO_ONLY" = "true"; then
 	rdnames
     fi
+    get_xsdomid
     echo -n "Shutting down Xen domains:"
     name=;id=
     while read LN; do
 	parseln "$LN" || continue
 	if test $id = 0; then continue; fi
+	if test $id = $XS_DOMID; then continue; fi
 	echo -n " $name"
 	if test "$XENDOMAINS_AUTO_ONLY" = "true"; then
 	    eval "
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..bf30030 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4822,6 +4822,7 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
     int opt, i, nb_domain;
     int wait_for_it = 0, all =0;
     int fallback_trigger = 0;
+    uint32_t domid;
     static struct option opts[] = {
         {"all", 0, 0, 'a'},
         {"wait", 0, 0, 'w'},
@@ -4846,32 +4847,42 @@ static int main_shutdown_or_reboot(int do_reboot, int argc, char **argv)
     }
 
     if (all) {
+        int ret;
         libxl_dominfo *dominfo;
         libxl_evgen_domain_death **deathws = NULL;
         if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
             fprintf(stderr, "libxl_list_domain failed.\n");
             return -1;
         }
+        ret = libxl_xenstore_domid(ctx, &domid);
+        if (ret == ERROR_DOMAIN_NOTFOUND) {
+            domid = 0;
+        } else if (ret != 0) {
+            fprintf(stderr, "libxl_xenstore_domid failed.\n");
+            return -1;
+        }
 
         if (wait_for_it)
             deathws = calloc(nb_domain, sizeof(*deathws));
 
+        wait_for_it = 0;
         for (i = 0; i<nb_domain; i++) {
-            if (dominfo[i].domid == 0)
+            if (dominfo[i].domid == 0 || dominfo[i].domid == domid)
                 continue;
             fn(dominfo[i].domid, deathws ? &deathws[i] : NULL, i,
                fallback_trigger);
+            wait_for_it++;
         }
 
-        if (wait_for_it) {
-            wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 0 */);
+        if (deathws) {
+            wait_for_domain_deaths(deathws, wait_for_it);
             free(deathws);
         }
 
         libxl_dominfo_list_free(dominfo, nb_domain);
     } else {
         libxl_evgen_domain_death *deathw = NULL;
-        uint32_t domid = find_domain(argv[optind]);
+        domid = find_domain(argv[optind]);
 
         fn(domid, wait_for_it ? &deathw : NULL, 0, fallback_trigger);
 
-- 
2.6.2

Re: [PATCH v2 01/13] xen: add xenstore domain flag to hypervisor

[Andrew Cooper]

On 18/12/15 13:14, Juergen Gross wrote:
> In order to be able to have full support of a xenstore domain in Xen
> add a "Xenstore-domain" flag to the hypervisor. This flag must be
> specified at domain creation time and is returned by
> XEN_DOMCTL_getdomaininfo.
>
> It will allow the domain to retrieve domain information by issuing the
> XEN_DOMCTL_getdomaininfo itself in order to be able to check for
> domains having been destroyed. At the same time this flag will inhibit
> the domain to be migrated, as this wouldn't be a very wise thing to do.
>
> In case of a later support of a rebootable Dom0 this flag will allow to
> recognize a xenstore domain already being present to connect to.
>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Andrew Cooper]

On 18/12/15 13:14, Juergen Gross wrote:
> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
> domain.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

What are the expected semantics here? Would you expect it to return
domid 0 for a traditional setup, or are you wanting to use it as a "does
a xenstored domain exist" test?

> ---
>  tools/libxl/libxl.c | 24 ++++++++++++++++++++++++
>  tools/libxl/libxl.h | 11 +++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9207621..3bcff59 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -701,6 +701,30 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
>      return 0;
>  }
>  
> +int libxl_xenstore_domid(libxl_ctx *ctx, uint32_t *domid)
> +{
> +    xc_dominfo_t info;
> +    uint32_t last_domid;
> +    int ret;
> +    GC_INIT(ctx);
> +
> +    for (last_domid = 0;
> +         (ret = xc_domain_getinfo(ctx->xch, last_domid, 1, &info)) == 1;
> +         last_domid = info.domid + 1)

Just as a note, this will scale badly with large numbers of domains.  A
lot of other actions in libxl will as well, so I don't think it warrants
changing at this point.

At some point, I need to progress my plans to have stats information
like this exposed via shared memory rather than hypercall.

~Andrew

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

On 18/12/15 14:53, Andrew Cooper wrote:
> On 18/12/15 13:14, Juergen Gross wrote:
>> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
>> domain.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> What are the expected semantics here? Would you expect it to return
> domid 0 for a traditional setup, or are you wanting to use it as a "does
> a xenstored domain exist" test?

The latter. It will be used in patch 13 to decide which domain to
stop via "xl shutdown --all".

> 
>> ---
>>  tools/libxl/libxl.c | 24 ++++++++++++++++++++++++
>>  tools/libxl/libxl.h | 11 +++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 9207621..3bcff59 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -701,6 +701,30 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r,
>>      return 0;
>>  }
>>  
>> +int libxl_xenstore_domid(libxl_ctx *ctx, uint32_t *domid)
>> +{
>> +    xc_dominfo_t info;
>> +    uint32_t last_domid;
>> +    int ret;
>> +    GC_INIT(ctx);
>> +
>> +    for (last_domid = 0;
>> +         (ret = xc_domain_getinfo(ctx->xch, last_domid, 1, &info)) == 1;
>> +         last_domid = info.domid + 1)
> 
> Just as a note, this will scale badly with large numbers of domains.  A
> lot of other actions in libxl will as well, so I don't think it warrants
> changing at this point.

I know. I think it won't really matter, as there are very few scenarios
where this information is needed.

I even noticed that libxl won't work very well with more than 1024
domains, as libxl_list_domain() has 1024 as fixed upper bound of
number of domains (just testing a patch to repair that).


Juergen

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Andrew Cooper]

On 18/12/15 13:14, Juergen Gross wrote:
> When restarting or shutting down dom0 the xendomains script tries to
> stop all other domains. Don't do this for the xenstore domain, as it
> might survive a dom0 reboot in the future.
>
> The same applies to xl shutdown --all.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
>  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
> index dfe0b33..70b7f16 100644
> --- a/tools/hotplug/Linux/xendomains.in
> +++ b/tools/hotplug/Linux/xendomains.in
> @@ -196,6 +196,17 @@ rdnames()
>      done
>  }
>  
> +# set xenstore domain id (or 0 if no xenstore domain)
> +get_xsdomid()

A get/set mismatch.

> +{
> +    ${bindir}/xenstore-exists /tool/xenstored/domid
> +    if test $? -ne 0; then
> +        XS_DOMID=0
> +    else
> +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
> +    fi

This is racy.  Can't you use a failure of xenstore-read as a signal that
the key doesn't exist?

~Andrew

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Juergen Gross]

On 18/12/15 15:42, Andrew Cooper wrote:
> On 18/12/15 13:14, Juergen Gross wrote:
>> When restarting or shutting down dom0 the xendomains script tries to
>> stop all other domains. Don't do this for the xenstore domain, as it
>> might survive a dom0 reboot in the future.
>>
>> The same applies to xl shutdown --all.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
>>  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/hotplug/Linux/xendomains.in b/tools/hotplug/Linux/xendomains.in
>> index dfe0b33..70b7f16 100644
>> --- a/tools/hotplug/Linux/xendomains.in
>> +++ b/tools/hotplug/Linux/xendomains.in
>> @@ -196,6 +196,17 @@ rdnames()
>>      done
>>  }
>>  
>> +# set xenstore domain id (or 0 if no xenstore domain)
>> +get_xsdomid()
> 
> A get/set mismatch.

Hmm, depends.

It is getting the domid of the xenstore domain and is setting
XS_DOMID accordingly. The main semantics are to get the correct
domid.

> 
>> +{
>> +    ${bindir}/xenstore-exists /tool/xenstored/domid
>> +    if test $? -ne 0; then
>> +        XS_DOMID=0
>> +    else
>> +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
>> +    fi
> 
> This is racy.  Can't you use a failure of xenstore-read as a signal that
> the key doesn't exist?

In theory it is racy. OTOH the race would require the xenstore domain to
be started between the call of xenstore-exists and xenstore-read, but
xenstore-exists will block in case no xenstore is available. And no, I
don't have to check that. the whole script will bail out early in this
case as in the beginning xl is tested to work which will be the case
with xenstore available only.

And using xenstore-read alone is ugly as it will barf in case the key
isn't existing.


Juergen

Re: [PATCH v2 01/13] xen: add xenstore domain flag to hypervisor

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 3fc3824..6304eb9 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -35,6 +35,7 @@ enum xsm_default {
>      XSM_DM_PRIV,  /* Device model can perform on its target domain */
>      XSM_TARGET,   /* Can perform on self or your target domain */
>      XSM_PRIV,     /* Privileged - normally restricted to dom0 */
> +    XSM_XS_PRIV,  /* Xenstore domain can obtain domain info */

Nit: This is conflating the semantic meaning of this bit with what the
implementation happens to be in practice right now.

I'd say something more like "Privileged or operations allowed to a xenstore
domain" perhaps.

Ian.

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

Re: [PATCH v2 01/13] xen: add xenstore domain flag to hypervisor

[Juergen Gross]

On 05/01/16 16:46, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index 3fc3824..6304eb9 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -35,6 +35,7 @@ enum xsm_default {
>>      XSM_DM_PRIV,  /* Device model can perform on its target domain */
>>      XSM_TARGET,   /* Can perform on self or your target domain */
>>      XSM_PRIV,     /* Privileged - normally restricted to dom0 */
>> +    XSM_XS_PRIV,  /* Xenstore domain can obtain domain info */
> 
> Nit: This is conflating the semantic meaning of this bit with what the
> implementation happens to be in practice right now.
> 
> I'd say something more like "Privileged or operations allowed to a xenstore
> domain" perhaps.

I'll change it to: "Xenstore domain - can do some privileged operations"


Juergen

Re: [PATCH v2 02/13] libxc: support new xenstore domain flag in libxc

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> Support the xenstore domain flag for obtaining domain info.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libxc/include/xenctrl.h |  2 +-
>  tools/libxc/xc_domain.c       | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h
> b/tools/libxc/include/xenctrl.h
> index 01a6dda..45d8ff6 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -456,7 +456,7 @@ typedef struct xc_dominfo {
>      uint32_t      ssidref;
>      unsigned int  dying:1, crashed:1, shutdown:1,
>                    paused:1, blocked:1, running:1,
> -                  hvm:1, debugged:1, pvh:1;
> +                  hvm:1, debugged:1, pvh:1, xs_domain:1;

All the other flags omit the _domain/_guest since it is implicit in the
context I think.

"xs" would be ok, so would "xenstore" or "xenstored" I think.

You might want to consider similar arguments for DOMINF and the create
flag, or you might want to argue that this field should be xs_domain for
consistency with those.

Apart from what colour to paint the shed to code looks trivially correct.

Ian.

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

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Ian Campbell]

On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
> On 18/12/15 14:53, Andrew Cooper wrote:
> > On 18/12/15 13:14, Juergen Gross wrote:
> > > Add libxl_xenstore_domid() to obtain the domain id of the xenstore
> > > domain.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > What are the expected semantics here? Would you expect it to return
> > domid 0 for a traditional setup, or are you wanting to use it as a
> > "does
> > a xenstored domain exist" test?
> 
> The latter. It will be used in patch 13 to decide which domain to
> stop via "xl shutdown --all".

ITYM "not stop".

libxl already has interfaces for getting info about a
domain, libxl_domain_info libxl_list_domain etc, it seems like this
property should be added to that data rather than introducing a special
purpose API to get it. Especially given that xl shutdown already calls
libxl_list_domain.

I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
to adding a more generic concept such as e.g. permanent domains, which
would be true for the xs domain but also for other special domains in the
future and could even be controlled by the user or toolstack (I'm thinking
you might want to set the flag on a driver domain for example).

Ian.

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

Re: [PATCH v2 04/13] xenstore: move init-xenstore-domain to tools/helpers

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> The program init-xenstore-domain to start a xenstore domain instead
> of the xenstored daemon is built, but not installed. Change that.
> 
> Move the program to a new tools subdirectory "helpers" to be able to
> use libxl in a later patch. Otherwise a dependency loop will be
> created.
> 
> While moving modify the coding style to the standard style used in
> the tools directory.

Please don't combine such things with moving the whole file. Or at the very
least please use git send-email/format-patch -M (ideally both do it
separately and use -M).

So long as the file is self consistent I'm not convinced it is actually
worth changing, plus it doesn't actually seem to be following CODING_STYLE
(which is the default style for tools/* as well as xen/*), since hasn't
gained e.g. spaces inside of the if ( x ).

Ian.

Re: [PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
> helper program.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/helpers/Makefile                   | 10 ++++++++++
>  tools/{libxl => helpers}/xen-init-dom0.c |  0
>  tools/libxl/Makefile                     | 14 +++-----------
>  3 files changed, 13 insertions(+), 11 deletions(-)
>  rename tools/{libxl => helpers}/xen-init-dom0.c (100%)
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 52347fd..92aead4 100644
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -5,10 +5,16 @@
>  XEN_ROOT = $(CURDIR)/../..
>  include $(XEN_ROOT)/tools/Rules.mk
>  
> +PROGS += xen-init-dom0
>  ifeq ($(CONFIG_Linux),y)
>  PROGS += init-xenstore-domain
>  endif
>  
> +XEN_INIT_DOM0_OBJS = xen-init-dom0.o
> +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)

I think the only use of this was for the xtl_* interfaces, which are now in
libxentoollog.h (with a compat include via xenctrl.h). Would you mind
switching the tool over to use xentoollog directly (perhaps in a separate
patch)?

Ian.

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

Re: [PATCH v2 08/13] xenstore: modify init-xenstore-domain parameter syntax

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> init-xenstore-domain takes only positional parameters today. Change
> this to a more flexible parameter syntax allowing to specify additional
> options or to omit some.
> 
> Today the supported usage is:
> 
> init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label>
>                      [<ramdisk-file>]
> 
> Modify this to:
> 
> init-xenstore-domain --kernel <xenstore-kernel>
>                      --memory <memory_mb>
>                      [--flask <flask-label>]
>                      [--ramdisk <ramdisk-file>]
> 
> The flask label is made optional in order to support xenstore domains
> without the need of a flask enabled hypervisor.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I think Daniel and I both acked this patch in v1 (as #5/9). Is this version
different? If so please include an intra-version changelog after the "---"
to help us out.


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

Re: [PATCH v2 09/13] xenstore: make use of the "xenstore domain" flag

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> Create the xenstore domain with the xs_domain flag specified. This
> enables us to test whether such a domain is already running before
> we create it. As there ought to be only one xenstore in the system
> we don't need to start another one.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/helpers/init-xenstore-domain.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-
> xenstore-domain.c
> index 17f0151..4cd9e0f 100644
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -66,7 +66,8 @@ static int build(xc_interface *xch)
>      } else {
>          ssid = SECINITSID_DOMU;
>      }
> -    rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
> +    rv = xc_domain_create(xch, ssid, handle, XEN_DOMCTL_CDF_xs_domain,
> +                  &domid, NULL);
>      if (rv) {
>          fprintf(stderr, "xc_domain_create failed\n");
>          goto err;
> @@ -165,6 +166,21 @@ err:
>      return rv;
>  }
>  
> +static int check_domain(xc_interface *xch)
> +{
> +    xc_dominfo_t info;
> +    uint32_t dom;
> +
> +    dom = 0;

You can pretty safely skip dom0 I think?

> +    while (xc_domain_getinfo(xch, dom, 1, &info) == 1) {

Logging errors (i.e. on return < 0) would be nice I think.

> +        if (info.xs_domain)
> +            return 1;
> +        dom = info.domid + 1;
> +    }
> +
> +    return 0;
> +}
> +
>  int main(int argc, char** argv)
>  {
>      int opt;
> @@ -204,7 +220,12 @@ int main(int argc, char** argv)
>          return 1;
>      }
>  
> -    rv = build(xch);
> +    rv = check_domain(xch);
> +
> +    if (!rv)
> +        rv = build(xch);
> +    else
> +        fprintf(stderr, "xenstore domain already present.\n");
>  
>      xc_interface_close(xch);
>  
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 11/13] tools: split up xen-init-dom0.c

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> Split up tools/helpers/xen-init-dom0.c in order to prepare reusing
> generation of the json configuration by init-xenstore-domain.c.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Wei, was there a reason for using atexit() to free the handles which I'm
not seeing? IOW I think there is a single exit path which could have done
all that today, so Juergen is OK to change it in this way.

Ian.


> ---
>  tools/helpers/Makefile        |  2 +-
>  tools/helpers/init-dom-json.c | 59 ++++++++++++++++++++++++++++++++++
>  tools/helpers/init-dom-json.h | 18 +++++++++++
>  tools/helpers/xen-init-dom0.c | 73 ++++++-------------------------------
> ------
>  4 files changed, 88 insertions(+), 64 deletions(-)
>  create mode 100644 tools/helpers/init-dom-json.c
>  create mode 100644 tools/helpers/init-dom-json.h
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 92aead4..cfb86f5 100644
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -10,7 +10,7 @@ ifeq ($(CONFIG_Linux),y)
>  PROGS += init-xenstore-domain
>  endif
>  
> -XEN_INIT_DOM0_OBJS = xen-init-dom0.o
> +XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
>  $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
>  $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
>  $(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
> diff --git a/tools/helpers/init-dom-json.c b/tools/helpers/init-dom-
> json.c
> new file mode 100644
> index 0000000..91b1fdf
> --- /dev/null
> +++ b/tools/helpers/init-dom-json.c
> @@ -0,0 +1,59 @@
> +#include <err.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include <xenctrl.h>
> +#include <libxl.h>
> +
> +int gen_stub_json_config(uint32_t domid)
> +{
> +    int rc = 1;
> +    xentoollog_logger_stdiostream *logger;
> +    libxl_ctx *ctx;
> +    libxl_domain_config dom_config;
> +    char *json = NULL;
> +
> +    logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
> +    if (!logger)
> +        return 1;
> +
> +    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
> +                        (xentoollog_logger *)logger)) {
> +        fprintf(stderr, "cannot init libxl context\n");
> +        goto outlog;
> +    }
> +
> +    libxl_domain_config_init(&dom_config);
> +
> +    /* Generate stub JSON config. */
> +    dom_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
> +    libxl_domain_build_info_init_type(&dom_config.b_info,
> +                                      LIBXL_DOMAIN_TYPE_PV);
> +
> +    json = libxl_domain_config_to_json(ctx, &dom_config);
> +    /* libxl-json format requires the string ends with '\0'. Code
> +     * snippet taken from libxl.
> +     */
> +    rc = libxl_userdata_store(ctx, domid, "libxl-json",
> +                              (const uint8_t *)json,
> +                              strlen(json) + 1 /* include '\0' */);
> +    if (rc)
> +        fprintf(stderr, "cannot store stub json config for domain %u\n",
> domid);
> +
> +    libxl_domain_config_dispose(&dom_config);
> +    free(json);
> +    libxl_ctx_free(ctx);
> +outlog:
> +    xtl_logger_destroy((xentoollog_logger *)logger);
> +    return rc;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/helpers/init-dom-json.h b/tools/helpers/init-dom-
> json.h
> new file mode 100644
> index 0000000..58c85df
> --- /dev/null
> +++ b/tools/helpers/init-dom-json.h
> @@ -0,0 +1,18 @@
> +#ifndef __INIT_DOM_JSON_H
> +#define __INIT_DOM_JSON_H
> +
> +/*
> + * Generate a stub JSON config for a domain with the given domid.
> + */
> +int gen_stub_json_config(uint32_t domid);
> +
> +#endif
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-
> dom0.c
> index 7925d64..9ab8468 100644
> --- a/tools/helpers/xen-init-dom0.c
> +++ b/tools/helpers/xen-init-dom0.c
> @@ -1,65 +1,26 @@
> -#include <err.h>
>  #include <stdlib.h>
>  #include <stdint.h>
>  #include <string.h>
>  #include <stdio.h>
>  
> -#include <xenctrl.h>
>  #include <xenstore.h>
> -#include <libxl.h>
> +
> +#include "init-dom-json.h"
>  
>  #define DOMNAME_PATH   "/local/domain/0/name"
>  #define DOMID_PATH     "/local/domain/0/domid"
>  
> -static libxl_ctx *ctx;
> -static xentoollog_logger_stdiostream *logger;
> -static struct xs_handle *xsh;
> -
> -static void ctx_alloc(void)
> +int main(int argc, char **argv)
>  {
> -    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
> -                        (xentoollog_logger *)logger)) {
> -        fprintf(stderr, "cannot init libxl context\n");
> -        exit(1);
> -    }
> +    int rc;
> +    struct xs_handle *xsh;
> +    char *domname_string = NULL, *domid_string = NULL;
> +
>      xsh = xs_open(0);
>      if (!xsh) {
>          fprintf(stderr, "cannot open xenstore connection\n");
>          exit(1);
>      }
> -}
> -
> -static void ctx_free(void)
> -{
> -    if (ctx) {
> -        libxl_ctx_free(ctx);
> -        ctx = NULL;
> -    }
> -    if (logger) {
> -        xtl_logger_destroy((xentoollog_logger *)logger);
> -        logger = NULL;
> -    }
> -    if (xsh) {
> -        xs_close(xsh);
> -        xsh = NULL;
> -    }
> -}
> -
> -int main(int argc, char **argv)
> -{
> -    int rc;
> -    libxl_domain_config dom0_config;
> -    char *domname_string = NULL, *domid_string = NULL;
> -    char *json = NULL;;
> -
> -    logger = xtl_createlogger_stdiostream(stderr, XTL_ERROR, 0);
> -    if (!logger) exit(1);
> -
> -    atexit(ctx_free);
> -
> -    ctx_alloc();
> -
> -    libxl_domain_config_init(&dom0_config);
>  
>      /* Sanity check: this program can only be run once. */
>      domid_string = xs_read(xsh, XBT_NULL, DOMID_PATH, NULL);
> @@ -70,22 +31,9 @@ int main(int argc, char **argv)
>          goto out;
>      }
>  
> -    /* Generate stub JSON config. */
> -    dom0_config.c_info.type = LIBXL_DOMAIN_TYPE_PV;
> -    libxl_domain_build_info_init_type(&dom0_config.b_info,
> -                                      LIBXL_DOMAIN_TYPE_PV);
> -
> -    json = libxl_domain_config_to_json(ctx, &dom0_config);
> -    /* libxl-json format requires the string ends with '\0'. Code
> -     * snippet taken from libxl.
> -     */
> -    rc = libxl_userdata_store(ctx, 0, "libxl-json",
> -                              (const uint8_t *)json,
> -                              strlen(json) + 1 /* include '\0' */);
> -    if (rc) {
> -        fprintf(stderr, "cannot store stub json config for Dom0\n");
> +    rc = gen_stub_json_config(0);
> +    if (rc)
>          goto out;
> -    }
>  
>      /* Write xenstore entries. */
>      if (!xs_write(xsh, XBT_NULL, DOMID_PATH, "0", strlen("0"))) {
> @@ -104,10 +52,9 @@ int main(int argc, char **argv)
>      printf("Done setting up Dom0\n");
>  
>  out:
> -    libxl_domain_config_dispose(&dom0_config);
>      free(domid_string);
>      free(domname_string);
> -    free(json);
> +    xs_close(xsh);
>      return rc;
>  }
>  
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 12/13] xenstore: write xenstore domain data to xenstore

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> After starting the xenstore domain write the basic data (domid, name
> and memory values) to the xenstore. This makes the domain appear
> correctly in xl list. Create a stub json object in order to make e.g.
> xl list -l happy.
> 
> Add a new option to init-xenstore-domain to be able to specify the
> domain's name.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

It looks like there is scope for more sharing with init-dom0 too, but this
is OK as it is IMHO:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Ian Campbell]

On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
> helper program.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/helpers/Makefile                   | 10 ++++++++++
>  tools/{libxl => helpers}/xen-init-dom0.c |  0
>  tools/libxl/Makefile                     | 14 +++-----------

Don't forget .gitignore here and when you move the xenstore init tool as
well.


>  3 files changed, 13 insertions(+), 11 deletions(-)
>  rename tools/{libxl => helpers}/xen-init-dom0.c (100%)
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 52347fd..92aead4 100644
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -5,10 +5,16 @@
>  XEN_ROOT = $(CURDIR)/../..
>  include $(XEN_ROOT)/tools/Rules.mk
>  
> +PROGS += xen-init-dom0
>  ifeq ($(CONFIG_Linux),y)
>  PROGS += init-xenstore-domain
>  endif
>  
> +XEN_INIT_DOM0_OBJS = xen-init-dom0.o
> +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
> +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
> +
>  INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o
>  $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenguest)
>  $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> @@ -17,12 +23,16 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS +=
> $(CFLAGS_libxenstore)
>  .PHONY: all
>  all: $(PROGS)
>  
> +xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
> +	$(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS)
> $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl)
> $(APPEND_LDFLAGS)
> +
>  init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
>  	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS)
> $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
> $(APPEND_LDFLAGS)
>  
>  .PHONY: install
>  install: all
>  	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> +	$(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(LIBEXEC_BIN)
>  ifeq ($(CONFIG_Linux),y)
>  	$(INSTALL_PROG) init-xenstore-domain $(DESTDIR)$(LIBEXEC_BIN)
>  endif
> diff --git a/tools/libxl/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
> similarity index 100%
> rename from tools/libxl/xen-init-dom0.c
> rename to tools/helpers/xen-init-dom0.c
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 6ff5bee..6a913c8 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -142,7 +142,7 @@ LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o
> libxlu_cfg.o \
>  	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
>  $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
>  
> -CLIENTS = xl testidl libxl-save-helper xen-init-dom0
> +CLIENTS = xl testidl libxl-save-helper
>  
>  CFLAGS_XL += $(CFLAGS_libxenlight)
>  CFLAGS_XL += -Wshadow
> @@ -153,10 +153,6 @@ $(XL_OBJS) $(TEST_PROG_OBJS) _libxl.api-for-check: \
>  $(XL_OBJS): CFLAGS += $(CFLAGS_XL)
>  $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h
> needs it.
>  
> -XEN_INIT_DOM0_OBJS = xen-init-dom0.o
> -$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> -$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
> -
>  SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o
>  $(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
>  
> @@ -173,7 +169,7 @@ all: $(CLIENTS) $(TEST_PROGS) $(PKG_CONFIG) \
>  	$(AUTOSRCS) $(AUTOINCS)
>  
>  $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS) $(SAVE_HELPER_OBJS) \
> -		$(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS)
> $(XEN_INIT_DOM0_OBJS): \
> +		$(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS): \
>  	$(AUTOINCS) libxl.api-ok
>  
>  %.c %.h:: %.y
> @@ -214,7 +210,7 @@ libxl_internal_json.h: _libxl_types_internal_json.h
>  xl.h: _paths.h
>  
>  $(LIBXL_OBJS) $(LIBXL_TEST_OBJS) $(LIBXLU_OBJS) \
> -	$(XL_OBJS) $(TEST_PROG_OBJS) $(SAVE_HELPER_OBJS)
> $(XEN_INIT_DOM0_OBJS): libxl.h
> +	$(XL_OBJS) $(TEST_PROG_OBJS) $(SAVE_HELPER_OBJS): libxl.h
>  $(LIBXL_OBJS) $(LIBXL_TEST_OBJS): libxl_internal.h
>  
>  _libxl_type%.h _libxl_type%_json.h _libxl_type%_private.h
> _libxl_type%.c: libxl_type%.idl gentypes.py idl.py
> @@ -255,9 +251,6 @@ libxlutil.a: $(LIBXLU_OBJS)
>  xl: $(XL_OBJS) libxlutil.so libxenlight.so
>  	$(CC) $(LDFLAGS) -o $@ $(XL_OBJS) libxlutil.so
> $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
>  
> -xen-init-dom0: $(XEN_INIT_DOM0_OBJS) libxenlight.so
> -	$(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS)
> $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenctrl)
> $(APPEND_LDFLAGS)
> -
>  test_%: test_%.o test_common.o libxlutil.so libxenlight_test.so
>  	$(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so,
> $(LDLIBS_libxenlight)) $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
>  
> @@ -280,7 +273,6 @@ install: all
>  	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>  	$(INSTALL_DIR) $(DESTDIR)$(SHAREDIR)/pkgconfig
>  	$(INSTALL_PROG) xl $(DESTDIR)$(sbindir)
> -	$(INSTALL_PROG) xen-init-dom0 $(DESTDIR)$(LIBEXEC_BIN)
>  	$(INSTALL_PROG) libxl-save-helper $(DESTDIR)$(LIBEXEC_BIN)
>  	$(INSTALL_SHLIB) libxenlight.so.$(MAJOR).$(MINOR)
> $(DESTDIR)$(libdir)
>  	$(SYMLINK_SHLIB) libxenlight.so.$(MAJOR).$(MINOR)
> $(DESTDIR)$(libdir)/libxenlight.so.$(MAJOR)

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

Re: [PATCH v2 11/13] tools: split up xen-init-dom0.c

[Wei Liu]

On Wed, Jan 06, 2016 at 04:26:10PM +0000, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> > Split up tools/helpers/xen-init-dom0.c in order to prepare reusing
> > generation of the json configuration by init-xenstore-domain.c.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Wei, was there a reason for using atexit() to free the handles which I'm
> not seeing? IOW I think there is a single exit path which could have done
> all that today, so Juergen is OK to change it in this way.
> 

No, there isn't particular reason to use atexit. I copied it from xl.
It's OK for Juergen to change it.

Wei.

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Ian Campbell]

On Fri, 2015-12-18 at 15:53 +0100, Juergen Gross wrote:
> On 18/12/15 15:42, Andrew Cooper wrote:
> > On 18/12/15 13:14, Juergen Gross wrote:
> > > When restarting or shutting down dom0 the xendomains script tries to
> > > stop all other domains. Don't do this for the xenstore domain, as it
> > > might survive a dom0 reboot in the future.
> > > 
> > > The same applies to xl shutdown --all.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > >  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
> > >  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
> > >  2 files changed, 32 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/hotplug/Linux/xendomains.in
> > > b/tools/hotplug/Linux/xendomains.in
> > > index dfe0b33..70b7f16 100644
> > > --- a/tools/hotplug/Linux/xendomains.in
> > > +++ b/tools/hotplug/Linux/xendomains.in
> > > @@ -196,6 +196,17 @@ rdnames()
> > >      done
> > >  }
> > >  
> > > +# set xenstore domain id (or 0 if no xenstore domain)
> > > +get_xsdomid()
> > 
> > A get/set mismatch.
> 
> Hmm, depends.
> 
> It is getting the domid of the xenstore domain and is setting
> XS_DOMID accordingly. The main semantics are to get the correct
> domid.
> 
> > 
> > > +{
> > > +    ${bindir}/xenstore-exists /tool/xenstored/domid
> > > +    if test $? -ne 0; then
> > > +        XS_DOMID=0
> > > +    else
> > > +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`

Please update docs/misc/xenstore-paths.markdown with this.

Did you mean /tools?

Earlier in the series there was a patch which looped over xc_dom_info
looking for the xs domain -- if this is in xenstore can't it use that?

> > > +    fi
> > 
> > This is racy.  Can't you use a failure of xenstore-read as a signal
> > that
> > the key doesn't exist?
> 
> In theory it is racy. OTOH the race would require the xenstore domain to
> be started between the call of xenstore-exists and xenstore-read, but
> xenstore-exists will block in case no xenstore is available. And no, I
> don't have to check that. the whole script will bail out early in this
> case as in the beginning xl is tested to work which will be the case
> with xenstore available only.
> 
> And using xenstore-read alone is ugly as it will barf in case the key
> isn't existing.

XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null`

seems like it should work:
root@st40:~# xenstore-read /foo 2>/dev/null; echo $?
1
root@st40:~# xenstore-read /local/domain/0/name 2>/dev/null; echo $?
Domain-0
0

> 
> 
> Juergen

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

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id"):
> On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
> > The latter. It will be used in patch 13 to decide which domain to
> > stop via "xl shutdown --all".
> 
> ITYM "not stop".
> 
> libxl already has interfaces for getting info about a
> domain, libxl_domain_info libxl_list_domain etc, it seems like this
> property should be added to that data rather than introducing a special
> purpose API to get it. Especially given that xl shutdown already calls
> libxl_list_domain.
> 
> I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
> to adding a more generic concept such as e.g. permanent domains, which
> would be true for the xs domain but also for other special domains in the
> future and could even be controlled by the user or toolstack (I'm thinking
> you might want to set the flag on a driver domain for example).

Indeed, I think a more general concept would be sensible.  Stub dm
domains are already handled specially I think.

Ian.

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

On 06/01/16 16:59, Ian Campbell wrote:
> On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
>> On 18/12/15 14:53, Andrew Cooper wrote:
>>> On 18/12/15 13:14, Juergen Gross wrote:
>>>> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
>>>> domain.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> What are the expected semantics here? Would you expect it to return
>>> domid 0 for a traditional setup, or are you wanting to use it as a
>>> "does
>>> a xenstored domain exist" test?
>>
>> The latter. It will be used in patch 13 to decide which domain to
>> stop via "xl shutdown --all".
> 
> ITYM "not stop".

Well, yes, if you select which domains to stop you select which domains
are not stopped, too. :-)

I don't mind either wording. :-)

> libxl already has interfaces for getting info about a
> domain, libxl_domain_info libxl_list_domain etc, it seems like this
> property should be added to that data rather than introducing a special
> purpose API to get it. Especially given that xl shutdown already calls
> libxl_list_domain.

Okay, I can change it that way.

> I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
> to adding a more generic concept such as e.g. permanent domains, which
> would be true for the xs domain but also for other special domains in the
> future and could even be controlled by the user or toolstack (I'm thinking
> you might want to set the flag on a driver domain for example).

The xs domain has to be handled separately, as it is needed to run in
order to be able to stop other domains in a clean way.

In case dom0 reboot will be supported we need two different reboot
modes: one with a hypervisor reboot implying all domains will be
stopped (including the xs domain), and one without hypervisor reboot
implying that all domains not requiring dom0 to be up all time will
still be running after dom0 is up again.

For stopping all domains before doing a hypervisor reboot, driver
domains should be stopped, too, but they should be stopped _after_
all other domains. And even then the xs domain should be still
running when the driver domains are being stopped.

IMO the generic concept you are asking for should be added in a
separate patch handling stopping (and possibly rebooting) driver
domains in a clean way.


Juergen

Re: [PATCH v2 02/13] libxc: support new xenstore domain flag in libxc

[Juergen Gross]

On 06/01/16 16:52, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> Support the xenstore domain flag for obtaining domain info.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/libxc/include/xenctrl.h |  2 +-
>>  tools/libxc/xc_domain.c       | 17 +++++++++--------
>>  2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h
>> b/tools/libxc/include/xenctrl.h
>> index 01a6dda..45d8ff6 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -456,7 +456,7 @@ typedef struct xc_dominfo {
>>      uint32_t      ssidref;
>>      unsigned int  dying:1, crashed:1, shutdown:1,
>>                    paused:1, blocked:1, running:1,
>> -                  hvm:1, debugged:1, pvh:1;
>> +                  hvm:1, debugged:1, pvh:1, xs_domain:1;
> 
> All the other flags omit the _domain/_guest since it is implicit in the
> context I think.
> 
> "xs" would be ok, so would "xenstore" or "xenstored" I think.
> 
> You might want to consider similar arguments for DOMINF and the create
> flag, or you might want to argue that this field should be xs_domain for
> consistency with those.

In case there are no objections from others, I'll go with "xenstore".

> 
> Apart from what colour to paint the shed to code looks trivially correct.

Can I take this as an "Ack"?


Juergen

Re: [PATCH v2 04/13] xenstore: move init-xenstore-domain to tools/helpers

[Juergen Gross]

On 06/01/16 17:03, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> The program init-xenstore-domain to start a xenstore domain instead
>> of the xenstored daemon is built, but not installed. Change that.
>>
>> Move the program to a new tools subdirectory "helpers" to be able to
>> use libxl in a later patch. Otherwise a dependency loop will be
>> created.
>>
>> While moving modify the coding style to the standard style used in
>> the tools directory.
> 
> Please don't combine such things with moving the whole file. Or at the very
> least please use git send-email/format-patch -M (ideally both do it
> separately and use -M).

Okay, I'll do the move in this patch only.

> So long as the file is self consistent I'm not convinced it is actually
> worth changing, plus it doesn't actually seem to be following CODING_STYLE
> (which is the default style for tools/* as well as xen/*), since hasn't
> gained e.g. spaces inside of the if ( x ).

As there are substantial changes to this file in later patches, I'll do
the coding style changes in a separate patch, using the _correct_ style.


Juergen

Re: [PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Juergen Gross]

On 06/01/16 17:12, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
>> helper program.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/helpers/Makefile                   | 10 ++++++++++
>>  tools/{libxl => helpers}/xen-init-dom0.c |  0
>>  tools/libxl/Makefile                     | 14 +++-----------
>>  3 files changed, 13 insertions(+), 11 deletions(-)
>>  rename tools/{libxl => helpers}/xen-init-dom0.c (100%)
>>
>> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
>> index 52347fd..92aead4 100644
>> --- a/tools/helpers/Makefile
>> +++ b/tools/helpers/Makefile
>> @@ -5,10 +5,16 @@
>>  XEN_ROOT = $(CURDIR)/../..
>>  include $(XEN_ROOT)/tools/Rules.mk
>>  
>> +PROGS += xen-init-dom0
>>  ifeq ($(CONFIG_Linux),y)
>>  PROGS += init-xenstore-domain
>>  endif
>>  
>> +XEN_INIT_DOM0_OBJS = xen-init-dom0.o
>> +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> 
> I think the only use of this was for the xtl_* interfaces, which are now in
> libxentoollog.h (with a compat include via xenctrl.h). Would you mind
> switching the tool over to use xentoollog directly (perhaps in a separate
> patch)?

Will do.


Juergen

Re: [PATCH v2 08/13] xenstore: modify init-xenstore-domain parameter syntax

[Juergen Gross]

On 06/01/16 17:21, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> init-xenstore-domain takes only positional parameters today. Change
>> this to a more flexible parameter syntax allowing to specify additional
>> options or to omit some.
>>
>> Today the supported usage is:
>>
>> init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label>
>>                      [<ramdisk-file>]
>>
>> Modify this to:
>>
>> init-xenstore-domain --kernel <xenstore-kernel>
>>                      --memory <memory_mb>
>>                      [--flask <flask-label>]
>>                      [--ramdisk <ramdisk-file>]
>>
>> The flask label is made optional in order to support xenstore domains
>> without the need of a flask enabled hypervisor.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I think Daniel and I both acked this patch in v1 (as #5/9). Is this version
> different? If so please include an intra-version changelog after the "---"
> to help us out.

There was an error in parameter parsing (omitting kernel or memory
wasn't detected). It was mentioned in the changelog in the cover letter.

I'll add a note next time when I drop an Ack.


Juergen

Re: [PATCH v2 09/13] xenstore: make use of the "xenstore domain" flag

[Juergen Gross]

On 06/01/16 17:23, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> Create the xenstore domain with the xs_domain flag specified. This
>> enables us to test whether such a domain is already running before
>> we create it. As there ought to be only one xenstore in the system
>> we don't need to start another one.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/helpers/init-xenstore-domain.c | 25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-
>> xenstore-domain.c
>> index 17f0151..4cd9e0f 100644
>> --- a/tools/helpers/init-xenstore-domain.c
>> +++ b/tools/helpers/init-xenstore-domain.c
>> @@ -66,7 +66,8 @@ static int build(xc_interface *xch)
>>      } else {
>>          ssid = SECINITSID_DOMU;
>>      }
>> -    rv = xc_domain_create(xch, ssid, handle, 0, &domid, NULL);
>> +    rv = xc_domain_create(xch, ssid, handle, XEN_DOMCTL_CDF_xs_domain,
>> +                  &domid, NULL);
>>      if (rv) {
>>          fprintf(stderr, "xc_domain_create failed\n");
>>          goto err;
>> @@ -165,6 +166,21 @@ err:
>>      return rv;
>>  }
>>  
>> +static int check_domain(xc_interface *xch)
>> +{
>> +    xc_dominfo_t info;
>> +    uint32_t dom;
>> +
>> +    dom = 0;
> 
> You can pretty safely skip dom0 I think?

Hmm, yes.

> 
>> +    while (xc_domain_getinfo(xch, dom, 1, &info) == 1) {
> 
> Logging errors (i.e. on return < 0) would be nice I think.

Okay.


Juergen

Re: [PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Juergen Gross]

On 06/01/16 17:28, Ian Campbell wrote:
> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>> Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
>> helper program.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/helpers/Makefile                   | 10 ++++++++++
>>  tools/{libxl => helpers}/xen-init-dom0.c |  0
>>  tools/libxl/Makefile                     | 14 +++-----------
> 
> Don't forget .gitignore here and when you move the xenstore init tool as
> well.

Uuh, yes, of course. Thanks for noticing this.


Juergen

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Juergen Gross]

On 06/01/16 17:33, Ian Campbell wrote:
> On Fri, 2015-12-18 at 15:53 +0100, Juergen Gross wrote:
>> On 18/12/15 15:42, Andrew Cooper wrote:
>>> On 18/12/15 13:14, Juergen Gross wrote:
>>>> When restarting or shutting down dom0 the xendomains script tries to
>>>> stop all other domains. Don't do this for the xenstore domain, as it
>>>> might survive a dom0 reboot in the future.
>>>>
>>>> The same applies to xl shutdown --all.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
>>>>  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
>>>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/hotplug/Linux/xendomains.in
>>>> b/tools/hotplug/Linux/xendomains.in
>>>> index dfe0b33..70b7f16 100644
>>>> --- a/tools/hotplug/Linux/xendomains.in
>>>> +++ b/tools/hotplug/Linux/xendomains.in
>>>> @@ -196,6 +196,17 @@ rdnames()
>>>>      done
>>>>  }
>>>>  
>>>> +# set xenstore domain id (or 0 if no xenstore domain)
>>>> +get_xsdomid()
>>>
>>> A get/set mismatch.
>>
>> Hmm, depends.
>>
>> It is getting the domid of the xenstore domain and is setting
>> XS_DOMID accordingly. The main semantics are to get the correct
>> domid.
>>
>>>
>>>> +{
>>>> +    ${bindir}/xenstore-exists /tool/xenstored/domid
>>>> +    if test $? -ne 0; then
>>>> +        XS_DOMID=0
>>>> +    else
>>>> +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
> 
> Please update docs/misc/xenstore-paths.markdown with this.

Okay.

> 
> Did you mean /tools?

No. The xenstore path is /tool/...

> 
> Earlier in the series there was a patch which looped over xc_dom_info
> looking for the xs domain -- if this is in xenstore can't it use that?

Hen and egg problem. You need to know how to connect to xenstore
(domain or daemon) before being able to read xenstore.

> 
>>>> +    fi
>>>
>>> This is racy.  Can't you use a failure of xenstore-read as a signal
>>> that
>>> the key doesn't exist?
>>
>> In theory it is racy. OTOH the race would require the xenstore domain to
>> be started between the call of xenstore-exists and xenstore-read, but
>> xenstore-exists will block in case no xenstore is available. And no, I
>> don't have to check that. the whole script will bail out early in this
>> case as in the beginning xl is tested to work which will be the case
>> with xenstore available only.
>>
>> And using xenstore-read alone is ugly as it will barf in case the key
>> isn't existing.
> 
> XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid 2>/dev/null`
> 
> seems like it should work:
> root@st40:~# xenstore-read /foo 2>/dev/null; echo $?
> 1
> root@st40:~# xenstore-read /local/domain/0/name 2>/dev/null; echo $?
> Domain-0
> 0

Okay, I'll change it.


Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Ian Campbell]

On Thu, 2016-01-07 at 06:37 +0100, Juergen Gross wrote:
> On 06/01/16 16:59, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
> > > On 18/12/15 14:53, Andrew Cooper wrote:
> > > > On 18/12/15 13:14, Juergen Gross wrote:
> > > > > Add libxl_xenstore_domid() to obtain the domain id of the
> > > > > xenstore
> > > > > domain.
> > > > > 
> > > > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > > 
> > > > What are the expected semantics here? Would you expect it to return
> > > > domid 0 for a traditional setup, or are you wanting to use it as a
> > > > "does
> > > > a xenstored domain exist" test?
> > > 
> > > The latter. It will be used in patch 13 to decide which domain to
> > > stop via "xl shutdown --all".
> > 
> > ITYM "not stop".
> 
> Well, yes, if you select which domains to stop you select which domains
> are not stopped, too. :-)
> 
> I don't mind either wording. :-)

In the sense you meant I think you want "domains".

> > libxl already has interfaces for getting info about a
> > domain, libxl_domain_info libxl_list_domain etc, it seems like this
> > property should be added to that data rather than introducing a special
> > purpose API to get it. Especially given that xl shutdown already calls
> > libxl_list_domain.
> 
> Okay, I can change it that way.

Thanks.

> > I'm unsure if (lib)xl ought to be special casing XS in this way, as
> > opposed
> > to adding a more generic concept such as e.g. permanent domains, which
> > would be true for the xs domain but also for other special domains in
> > the
> > future and could even be controlled by the user or toolstack (I'm
> > thinking
> > you might want to set the flag on a driver domain for example).
> 
> The xs domain has to be handled separately, as it is needed to run in
> order to be able to stop other domains in a clean way.
> 
> In case dom0 reboot will be supported we need two different reboot
> modes: one with a hypervisor reboot implying all domains will be
> stopped (including the xs domain), and one without hypervisor reboot
> implying that all domains not requiring dom0 to be up all time will
> still be running after dom0 is up again.
> 
> For stopping all domains before doing a hypervisor reboot, driver
> domains should be stopped, too, but they should be stopped _after_
> all other domains. And even then the xs domain should be still
> running when the driver domains are being stopped.

So what we have is in effect some sort of reboot ordering or priority and a
threshold depending on what sort of reboot the host as a whole is
undergoing?

> IMO the generic concept you are asking for should be added in a
> separate patch handling stopping (and possibly rebooting) driver
> domains in a clean way.

Since libxl has a stable API once we add something we need to continue
supporting it, so we cannot (easily/cleanly) switch an xs specific scheme
into a generic one later. That argues then for supporting the XS case via
the generic mechanism now, even if we don't implement the other cases.

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

Re: [PATCH v2 02/13] libxc: support new xenstore domain flag in libxc

[Ian Campbell]

On Thu, 2016-01-07 at 07:08 +0100, Juergen Gross wrote:
> On 06/01/16 16:52, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> > > Support the xenstore domain flag for obtaining domain info.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > >  tools/libxc/include/xenctrl.h |  2 +-
> > >  tools/libxc/xc_domain.c       | 17 +++++++++--------
> > >  2 files changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tools/libxc/include/xenctrl.h
> > > b/tools/libxc/include/xenctrl.h
> > > index 01a6dda..45d8ff6 100644
> > > --- a/tools/libxc/include/xenctrl.h
> > > +++ b/tools/libxc/include/xenctrl.h
> > > @@ -456,7 +456,7 @@ typedef struct xc_dominfo {
> > >      uint32_t      ssidref;
> > >      unsigned int  dying:1, crashed:1, shutdown:1,
> > >                    paused:1, blocked:1, running:1,
> > > -                  hvm:1, debugged:1, pvh:1;
> > > +                  hvm:1, debugged:1, pvh:1, xs_domain:1;
> > 
> > All the other flags omit the _domain/_guest since it is implicit in the
> > context I think.
> > 
> > "xs" would be ok, so would "xenstore" or "xenstored" I think.
> > 
> > You might want to consider similar arguments for DOMINF and the create
> > flag, or you might want to argue that this field should be xs_domain for
> > consistency with those.
> 
> In case there are no objections from others, I'll go with "xenstore".

Sounds good to me.

> > 
> > Apart from what colour to paint the shed to code looks trivially
> > correct.
> 
> Can I take this as an "Ack"?

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

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

Re: [PATCH v2 05/13] libxl: move xen-init-dom0 to tools/helpers

[Ian Campbell]

On Thu, 2016-01-07 at 07:15 +0100, Juergen Gross wrote:
> On 06/01/16 17:12, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> > > Move xen-init-dom0 from tools/libxl to tools/helpers, as it is just a
> > > helper program.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > >  tools/helpers/Makefile                   | 10 ++++++++++
> > >  tools/{libxl => helpers}/xen-init-dom0.c |  0
> > >  tools/libxl/Makefile                     | 14 +++-----------
> > >  3 files changed, 13 insertions(+), 11 deletions(-)
> > >  rename tools/{libxl => helpers}/xen-init-dom0.c (100%)
> > > 
> > > diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> > > index 52347fd..92aead4 100644
> > > --- a/tools/helpers/Makefile
> > > +++ b/tools/helpers/Makefile
> > > @@ -5,10 +5,16 @@
> > >  XEN_ROOT = $(CURDIR)/../..
> > >  include $(XEN_ROOT)/tools/Rules.mk
> > >  
> > > +PROGS += xen-init-dom0
> > >  ifeq ($(CONFIG_Linux),y)
> > >  PROGS += init-xenstore-domain
> > >  endif
> > >  
> > > +XEN_INIT_DOM0_OBJS = xen-init-dom0.o
> > > +$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> > 
> > I think the only use of this was for the xtl_* interfaces, which are
> > now in
> > libxentoollog.h (with a compat include via xenctrl.h). Would you mind
> > switching the tool over to use xentoollog directly (perhaps in a
> > separate
> > patch)?
> 
> Will do.

Thanks!

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

Re: [PATCH v2 08/13] xenstore: modify init-xenstore-domain parameter syntax

[Ian Campbell]

On Thu, 2016-01-07 at 07:34 +0100, Juergen Gross wrote:
> On 06/01/16 17:21, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> > > init-xenstore-domain takes only positional parameters today. Change
> > > this to a more flexible parameter syntax allowing to specify
> > > additional
> > > options or to omit some.
> > > 
> > > Today the supported usage is:
> > > 
> > > init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label>
> > >                      [<ramdisk-file>]
> > > 
> > > Modify this to:
> > > 
> > > init-xenstore-domain --kernel <xenstore-kernel>
> > >                      --memory <memory_mb>
> > >                      [--flask <flask-label>]
> > >                      [--ramdisk <ramdisk-file>]
> > > 
> > > The flask label is made optional in order to support xenstore domains
> > > without the need of a flask enabled hypervisor.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > I think Daniel and I both acked this patch in v1 (as #5/9). Is this
> > version
> > different? If so please include an intra-version changelog after the "-
> > --"
> > to help us out.
> 
> There was an error in parameter parsing (omitting kernel or memory
> wasn't detected). It was mentioned in the changelog in the cover letter.

In general we prefer these sorts of details to be mentioned in the patch
themselves, after a "---" marker such that they don't get included in the
actual commit. The cover letter should be more of a summary.

> I'll add a note next time when I drop an Ack.

Thanks.

WRT that change and:

> +    if (optind != argc || !kernel || !memory) {
> +        usage();
>          return 2;
>      }

Under what circumstances can optind != argc? AFAICT it should never happen
because the switch cover all valid options and returns otherwise. If it can
never happen it should be an assert.

Oh, is it to catch things like "--foo bar baz" i.e. additional non-option
arguments (of which there should be none)?

If that is the case: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

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

Re: [PATCH v2 11/13] tools: split up xen-init-dom0.c

[Ian Campbell]

On Wed, 2016-01-06 at 16:33 +0000, Wei Liu wrote:
> On Wed, Jan 06, 2016 at 04:26:10PM +0000, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
> > > Split up tools/helpers/xen-init-dom0.c in order to prepare reusing
> > > generation of the json configuration by init-xenstore-domain.c.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Wei, was there a reason for using atexit() to free the handles which
> > I'm
> > not seeing? IOW I think there is a single exit path which could have
> > done
> > all that today, so Juergen is OK to change it in this way.
> > 
> 
> No, there isn't particular reason to use atexit. I copied it from xl.
> It's OK for Juergen to change it.

In which case: Acked-by: Ian Campbell <ian.campbell@citrix.com>

The mixture of logging via xtl and via (f)printf in this code is a bit odd,
but that's unchanged by this patch. Someone who was feeling particularly
enthusiastic might like to make it use xtl throughout and provide a way to
increase the log level.

Ian.

Re: [PATCH v2 08/13] xenstore: modify init-xenstore-domain parameter syntax

[Juergen Gross]

On 07/01/16 11:23, Ian Campbell wrote:
> On Thu, 2016-01-07 at 07:34 +0100, Juergen Gross wrote:
>> On 06/01/16 17:21, Ian Campbell wrote:
>>> On Fri, 2015-12-18 at 14:14 +0100, Juergen Gross wrote:
>>>> init-xenstore-domain takes only positional parameters today. Change
>>>> this to a more flexible parameter syntax allowing to specify
>>>> additional
>>>> options or to omit some.
>>>>
>>>> Today the supported usage is:
>>>>
>>>> init-xenstore-domain <xenstore-kernel> <memory_mb> <flask-label>
>>>>                      [<ramdisk-file>]
>>>>
>>>> Modify this to:
>>>>
>>>> init-xenstore-domain --kernel <xenstore-kernel>
>>>>                      --memory <memory_mb>
>>>>                      [--flask <flask-label>]
>>>>                      [--ramdisk <ramdisk-file>]
>>>>
>>>> The flask label is made optional in order to support xenstore domains
>>>> without the need of a flask enabled hypervisor.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> I think Daniel and I both acked this patch in v1 (as #5/9). Is this
>>> version
>>> different? If so please include an intra-version changelog after the "-
>>> --"
>>> to help us out.
>>
>> There was an error in parameter parsing (omitting kernel or memory
>> wasn't detected). It was mentioned in the changelog in the cover letter.
> 
> In general we prefer these sorts of details to be mentioned in the patch
> themselves, after a "---" marker such that they don't get included in the
> actual commit. The cover letter should be more of a summary.

Okay, I'll add a patch specific changelog in the future.

> 
>> I'll add a note next time when I drop an Ack.
> 
> Thanks.
> 
> WRT that change and:
> 
>> +    if (optind != argc || !kernel || !memory) {
>> +        usage();
>>          return 2;
>>      }
> 
> Under what circumstances can optind != argc? AFAICT it should never happen
> because the switch cover all valid options and returns otherwise. If it can
> never happen it should be an assert.
> 
> Oh, is it to catch things like "--foo bar baz" i.e. additional non-option
> arguments (of which there should be none)?

Exactly.

> 
> If that is the case: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

Juergen

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Ian Campbell]

On Thu, 2016-01-07 at 07:52 +0100, Juergen Gross wrote:
> On 06/01/16 17:33, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 15:53 +0100, Juergen Gross wrote:
> > > On 18/12/15 15:42, Andrew Cooper wrote:
> > > > On 18/12/15 13:14, Juergen Gross wrote:
> > > > > When restarting or shutting down dom0 the xendomains script tries
> > > > > to
> > > > > stop all other domains. Don't do this for the xenstore domain, as
> > > > > it
> > > > > might survive a dom0 reboot in the future.
> > > > > 
> > > > > The same applies to xl shutdown --all.
> > > > > 
> > > > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > > > ---
> > > > >  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
> > > > >  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
> > > > >  2 files changed, 32 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/tools/hotplug/Linux/xendomains.in
> > > > > b/tools/hotplug/Linux/xendomains.in
> > > > > index dfe0b33..70b7f16 100644
> > > > > --- a/tools/hotplug/Linux/xendomains.in
> > > > > +++ b/tools/hotplug/Linux/xendomains.in
> > > > > @@ -196,6 +196,17 @@ rdnames()
> > > > >      done
> > > > >  }
> > > > >  
> > > > > +# set xenstore domain id (or 0 if no xenstore domain)
> > > > > +get_xsdomid()
> > > > 
> > > > A get/set mismatch.
> > > 
> > > Hmm, depends.
> > > 
> > > It is getting the domid of the xenstore domain and is setting
> > > XS_DOMID accordingly. The main semantics are to get the correct
> > > domid.
> > > 
> > > > 
> > > > > +{
> > > > > +    ${bindir}/xenstore-exists /tool/xenstored/domid
> > > > > +    if test $? -ne 0; then
> > > > > +        XS_DOMID=0
> > > > > +    else
> > > > > +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
> > 
> > Please update docs/misc/xenstore-paths.markdown with this.
> 
> Okay.
> 
> > 
> > Did you mean /tools?
> 
> No. The xenstore path is /tool/...

You mean that are preexisting uses of this path?

/me looks.

Oh, so there is. Undocumented too :-(

> > 
> > Earlier in the series there was a patch which looped over xc_dom_info
> > looking for the xs domain -- if this is in xenstore can't it use that?
> 
> Hen and egg problem. You need to know how to connect to xenstore
> (domain or daemon) before being able to read xenstore.

Oh, of course.

Can you not infer from the presence of absence of the sockets in the local
f/s or do they always exist (i.e. stale from a previous configuration)?

We did once try switching to always using the domain ring, even if the
client and server were co-located in the same domain, but that can result
in uninterruptible sleeps in the kernel IIRC (a bug which might have since
been fixed, not sure). Anyway, that probably rules out the "solution" of
always using the domain.

The daemon would drop a pid file, but I suppose that might also be stale.

I'm mostly just brainstorming here, I don't really have a problem with the
scan in the earlier patch.

(FWIW in English idiom we usually say chicken and egg BTW)

Ian.

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

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

On 07/01/16 11:11, Ian Campbell wrote:
> On Thu, 2016-01-07 at 06:37 +0100, Juergen Gross wrote:
>> On 06/01/16 16:59, Ian Campbell wrote:
>>> On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
>>>> On 18/12/15 14:53, Andrew Cooper wrote:
>>>>> On 18/12/15 13:14, Juergen Gross wrote:
>>>>>> Add libxl_xenstore_domid() to obtain the domain id of the
>>>>>> xenstore
>>>>>> domain.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> What are the expected semantics here? Would you expect it to return
>>>>> domid 0 for a traditional setup, or are you wanting to use it as a
>>>>> "does
>>>>> a xenstored domain exist" test?
>>>>
>>>> The latter. It will be used in patch 13 to decide which domain to
>>>> stop via "xl shutdown --all".
>>>
>>> ITYM "not stop".
>>
>> Well, yes, if you select which domains to stop you select which domains
>> are not stopped, too. :-)
>>
>> I don't mind either wording. :-)
> 
> In the sense you meant I think you want "domains".

Indeed.

>>> I'm unsure if (lib)xl ought to be special casing XS in this way, as
>>> opposed
>>> to adding a more generic concept such as e.g. permanent domains, which
>>> would be true for the xs domain but also for other special domains in
>>> the
>>> future and could even be controlled by the user or toolstack (I'm
>>> thinking
>>> you might want to set the flag on a driver domain for example).
>>
>> The xs domain has to be handled separately, as it is needed to run in
>> order to be able to stop other domains in a clean way.
>>
>> In case dom0 reboot will be supported we need two different reboot
>> modes: one with a hypervisor reboot implying all domains will be
>> stopped (including the xs domain), and one without hypervisor reboot
>> implying that all domains not requiring dom0 to be up all time will
>> still be running after dom0 is up again.
>>
>> For stopping all domains before doing a hypervisor reboot, driver
>> domains should be stopped, too, but they should be stopped _after_
>> all other domains. And even then the xs domain should be still
>> running when the driver domains are being stopped.
> 
> So what we have is in effect some sort of reboot ordering or priority and a
> threshold depending on what sort of reboot the host as a whole is
> undergoing?

I think so, yes.

>> IMO the generic concept you are asking for should be added in a
>> separate patch handling stopping (and possibly rebooting) driver
>> domains in a clean way.
> 
> Since libxl has a stable API once we add something we need to continue
> supporting it, so we cannot (easily/cleanly) switch an xs specific scheme
> into a generic one later. That argues then for supporting the XS case via
> the generic mechanism now, even if we don't implement the other cases.

I can't see a scenario where the xenstore domain would have to be
stopped by dom0. Once you do it you'll never be able to connect to
it again without changing the xenbus driver interface, too. It is
the same reason why xenstored can't be restarted.

Driver domains are different and I think the interface to query a
domain whether it is a driver domain or whether it might survive a
dom0 reboot should be based on xenstore.

So a xenstore domain would always need special handling.


Juergen

Re: [PATCH v2 13/13] tools: don't stop xenstore domain when stopping dom0

[Juergen Gross]

On 07/01/16 11:34, Ian Campbell wrote:
> On Thu, 2016-01-07 at 07:52 +0100, Juergen Gross wrote:
>> On 06/01/16 17:33, Ian Campbell wrote:
>>> On Fri, 2015-12-18 at 15:53 +0100, Juergen Gross wrote:
>>>> On 18/12/15 15:42, Andrew Cooper wrote:
>>>>> On 18/12/15 13:14, Juergen Gross wrote:
>>>>>> When restarting or shutting down dom0 the xendomains script tries
>>>>>> to
>>>>>> stop all other domains. Don't do this for the xenstore domain, as
>>>>>> it
>>>>>> might survive a dom0 reboot in the future.
>>>>>>
>>>>>> The same applies to xl shutdown --all.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>> ---
>>>>>>  tools/hotplug/Linux/xendomains.in | 17 +++++++++++++++++
>>>>>>  tools/libxl/xl_cmdimpl.c          | 19 +++++++++++++++----
>>>>>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/hotplug/Linux/xendomains.in
>>>>>> b/tools/hotplug/Linux/xendomains.in
>>>>>> index dfe0b33..70b7f16 100644
>>>>>> --- a/tools/hotplug/Linux/xendomains.in
>>>>>> +++ b/tools/hotplug/Linux/xendomains.in
>>>>>> @@ -196,6 +196,17 @@ rdnames()
>>>>>>      done
>>>>>>  }
>>>>>>  
>>>>>> +# set xenstore domain id (or 0 if no xenstore domain)
>>>>>> +get_xsdomid()
>>>>>
>>>>> A get/set mismatch.
>>>>
>>>> Hmm, depends.
>>>>
>>>> It is getting the domid of the xenstore domain and is setting
>>>> XS_DOMID accordingly. The main semantics are to get the correct
>>>> domid.
>>>>
>>>>>
>>>>>> +{
>>>>>> +    ${bindir}/xenstore-exists /tool/xenstored/domid
>>>>>> +    if test $? -ne 0; then
>>>>>> +        XS_DOMID=0
>>>>>> +    else
>>>>>> +        XS_DOMID=`${bindir}/xenstore-read /tool/xenstored/domid`
>>>
>>> Please update docs/misc/xenstore-paths.markdown with this.
>>
>> Okay.
>>
>>>
>>> Did you mean /tools?
>>
>> No. The xenstore path is /tool/...
> 
> You mean that are preexisting uses of this path?
> 
> /me looks.
> 
> Oh, so there is. Undocumented too :-(
> 
>>>
>>> Earlier in the series there was a patch which looped over xc_dom_info
>>> looking for the xs domain -- if this is in xenstore can't it use that?
>>
>> Hen and egg problem. You need to know how to connect to xenstore
>> (domain or daemon) before being able to read xenstore.
> 
> Oh, of course.
> 
> Can you not infer from the presence of absence of the sockets in the local
> f/s or do they always exist (i.e. stale from a previous configuration)?

No. Sockets will be created later even in the xenstored case. And still
you need to know the domain id of the xenstore domain in order to be
able to connect to it. This is available in the hypervisor and the
xenstore domain only at this time.

> We did once try switching to always using the domain ring, even if the
> client and server were co-located in the same domain, but that can result
> in uninterruptible sleeps in the kernel IIRC (a bug which might have since
> been fixed, not sure). Anyway, that probably rules out the "solution" of
> always using the domain.
> 
> The daemon would drop a pid file, but I suppose that might also be stale.
> 
> I'm mostly just brainstorming here, I don't really have a problem with the
> scan in the earlier patch.
> 
> (FWIW in English idiom we usually say chicken and egg BTW)

Aah, thanks.


Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Ian Campbell]

On Thu, 2016-01-07 at 11:44 +0100, Juergen Gross wrote:
> > > IMO the generic concept you are asking for should be added in a
> > > separate patch handling stopping (and possibly rebooting) driver
> > > domains in a clean way.
> > 
> > Since libxl has a stable API once we add something we need to continue
> > supporting it, so we cannot (easily/cleanly) switch an xs specific
> > scheme
> > into a generic one later. That argues then for supporting the XS case
> > via
> > the generic mechanism now, even if we don't implement the other cases.
> 
> I can't see a scenario where the xenstore domain would have to be
> stopped by dom0. Once you do it you'll never be able to connect to
> it again without changing the xenbus driver interface, too. It is
> the same reason why xenstored can't be restarted.
> 
> Driver domains are different and I think the interface to query a
> domain whether it is a driver domain or whether it might survive a
> dom0 reboot should be based on xenstore.
> 
> So a xenstore domain would always need special handling.

If there is really _never_ any reason to stop the xs domain then I think at
the libxl API level a class of "never stop" domains would be better than
special casing the xs, even if it turns out the only member of the set is
xs at least we've given ourselves wriggle room if something else comes up
in the future.

Ian.

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

On 07/01/16 11:55, Ian Campbell wrote:
> On Thu, 2016-01-07 at 11:44 +0100, Juergen Gross wrote:
>>>> IMO the generic concept you are asking for should be added in a
>>>> separate patch handling stopping (and possibly rebooting) driver
>>>> domains in a clean way.
>>>
>>> Since libxl has a stable API once we add something we need to continue
>>> supporting it, so we cannot (easily/cleanly) switch an xs specific
>>> scheme
>>> into a generic one later. That argues then for supporting the XS case
>>> via
>>> the generic mechanism now, even if we don't implement the other cases.
>>
>> I can't see a scenario where the xenstore domain would have to be
>> stopped by dom0. Once you do it you'll never be able to connect to
>> it again without changing the xenbus driver interface, too. It is
>> the same reason why xenstored can't be restarted.
>>
>> Driver domains are different and I think the interface to query a
>> domain whether it is a driver domain or whether it might survive a
>> dom0 reboot should be based on xenstore.
>>
>> So a xenstore domain would always need special handling.
> 
> If there is really _never_ any reason to stop the xs domain then I think at
> the libxl API level a class of "never stop" domains would be better than
> special casing the xs, even if it turns out the only member of the set is
> xs at least we've given ourselves wriggle room if something else comes up
> in the future.

Okay, so this would translate to either:

- add a "never stop" flag to libxl_dominfo (can I do this without
  breaking the API?)
- add a new call interface to either check a single domain to be of
  the "never stop" class or to return a list of those domains.

Preferences?


Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Ian Campbell]

On Thu, 2016-01-07 at 12:21 +0100, Juergen Gross wrote:
> On 07/01/16 11:55, Ian Campbell wrote:
> > On Thu, 2016-01-07 at 11:44 +0100, Juergen Gross wrote:
> > > > > IMO the generic concept you are asking for should be added in a
> > > > > separate patch handling stopping (and possibly rebooting) driver
> > > > > domains in a clean way.
> > > > 
> > > > Since libxl has a stable API once we add something we need to
> > > > continue
> > > > supporting it, so we cannot (easily/cleanly) switch an xs specific
> > > > scheme
> > > > into a generic one later. That argues then for supporting the XS
> > > > case
> > > > via
> > > > the generic mechanism now, even if we don't implement the other
> > > > cases.
> > > 
> > > I can't see a scenario where the xenstore domain would have to be
> > > stopped by dom0. Once you do it you'll never be able to connect to
> > > it again without changing the xenbus driver interface, too. It is
> > > the same reason why xenstored can't be restarted.
> > > 
> > > Driver domains are different and I think the interface to query a
> > > domain whether it is a driver domain or whether it might survive a
> > > dom0 reboot should be based on xenstore.
> > > 
> > > So a xenstore domain would always need special handling.
> > 
> > If there is really _never_ any reason to stop the xs domain then I
> > think at
> > the libxl API level a class of "never stop" domains would be better
> > than
> > special casing the xs, even if it turns out the only member of the set
> > is
> > xs at least we've given ourselves wriggle room if something else comes
> > up
> > in the future.
> 
> Okay, so this would translate to either:
> 
> - add a "never stop" flag to libxl_dominfo (can I do this without
>   breaking the API?)
> - add a new call interface to either check a single domain to be of
>   the "never stop" class or to return a list of those domains.
> 
> Preferences?

Definitely the former, with a LIBXL_HAVE_ #define in libxl.h so consumers
know they can use it.

Wei, Ian, do you agree with this approach?

Ian.

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

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Wei Liu]

On Thu, Jan 07, 2016 at 06:37:34AM +0100, Juergen Gross wrote:
> On 06/01/16 16:59, Ian Campbell wrote:
> > On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
> >> On 18/12/15 14:53, Andrew Cooper wrote:
> >>> On 18/12/15 13:14, Juergen Gross wrote:
> >>>> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
> >>>> domain.
> >>>>
> >>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>
> >>> What are the expected semantics here? Would you expect it to return
> >>> domid 0 for a traditional setup, or are you wanting to use it as a
> >>> "does
> >>> a xenstored domain exist" test?
> >>
> >> The latter. It will be used in patch 13 to decide which domain to
> >> stop via "xl shutdown --all".
> > 
> > ITYM "not stop".
> 
> Well, yes, if you select which domains to stop you select which domains
> are not stopped, too. :-)
> 
> I don't mind either wording. :-)
> 
> > libxl already has interfaces for getting info about a
> > domain, libxl_domain_info libxl_list_domain etc, it seems like this
> > property should be added to that data rather than introducing a special
> > purpose API to get it. Especially given that xl shutdown already calls
> > libxl_list_domain.
> 
> Okay, I can change it that way.
> 
> > I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
> > to adding a more generic concept such as e.g. permanent domains, which
> > would be true for the xs domain but also for other special domains in the
> > future and could even be controlled by the user or toolstack (I'm thinking
> > you might want to set the flag on a driver domain for example).
> 
> The xs domain has to be handled separately, as it is needed to run in
> order to be able to stop other domains in a clean way.
> 
> In case dom0 reboot will be supported we need two different reboot
> modes: one with a hypervisor reboot implying all domains will be
> stopped (including the xs domain), and one without hypervisor reboot
> implying that all domains not requiring dom0 to be up all time will
> still be running after dom0 is up again.
> 

For so long we've lumped together so many things in Dom0, so it would be
better there is clear definition what you would expect from rebooting
Dom0.

As far as I can tell, currently in a typical setup Dom0 serves at least
several purposes:

1. Toosltack domain for managing VMs
2. Driver domain for physical devices
3. Running emulators
4. Provide some services to DomU (I know there are people who do that)

Do we need provision for adding more flags or groups? One flag (as
suggested in subthread) doesn't seem enough.

Wei.

> For stopping all domains before doing a hypervisor reboot, driver
> domains should be stopped, too, but they should be stopped _after_
> all other domains. And even then the xs domain should be still
> running when the driver domains are being stopped.
> 
> IMO the generic concept you are asking for should be added in a
> separate patch handling stopping (and possibly rebooting) driver
> domains in a clean way.
> 
> 
> Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Juergen Gross]

On 07/01/16 13:40, Wei Liu wrote:
> On Thu, Jan 07, 2016 at 06:37:34AM +0100, Juergen Gross wrote:
>> On 06/01/16 16:59, Ian Campbell wrote:
>>> On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
>>>> On 18/12/15 14:53, Andrew Cooper wrote:
>>>>> On 18/12/15 13:14, Juergen Gross wrote:
>>>>>> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
>>>>>> domain.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> What are the expected semantics here? Would you expect it to return
>>>>> domid 0 for a traditional setup, or are you wanting to use it as a
>>>>> "does
>>>>> a xenstored domain exist" test?
>>>>
>>>> The latter. It will be used in patch 13 to decide which domain to
>>>> stop via "xl shutdown --all".
>>>
>>> ITYM "not stop".
>>
>> Well, yes, if you select which domains to stop you select which domains
>> are not stopped, too. :-)
>>
>> I don't mind either wording. :-)
>>
>>> libxl already has interfaces for getting info about a
>>> domain, libxl_domain_info libxl_list_domain etc, it seems like this
>>> property should be added to that data rather than introducing a special
>>> purpose API to get it. Especially given that xl shutdown already calls
>>> libxl_list_domain.
>>
>> Okay, I can change it that way.
>>
>>> I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
>>> to adding a more generic concept such as e.g. permanent domains, which
>>> would be true for the xs domain but also for other special domains in the
>>> future and could even be controlled by the user or toolstack (I'm thinking
>>> you might want to set the flag on a driver domain for example).
>>
>> The xs domain has to be handled separately, as it is needed to run in
>> order to be able to stop other domains in a clean way.
>>
>> In case dom0 reboot will be supported we need two different reboot
>> modes: one with a hypervisor reboot implying all domains will be
>> stopped (including the xs domain), and one without hypervisor reboot
>> implying that all domains not requiring dom0 to be up all time will
>> still be running after dom0 is up again.
>>
> 
> For so long we've lumped together so many things in Dom0, so it would be
> better there is clear definition what you would expect from rebooting
> Dom0.
> 
> As far as I can tell, currently in a typical setup Dom0 serves at least
> several purposes:
> 
> 1. Toosltack domain for managing VMs
> 2. Driver domain for physical devices
> 3. Running emulators
> 4. Provide some services to DomU (I know there are people who do that)
> 
> Do we need provision for adding more flags or groups? One flag (as
> suggested in subthread) doesn't seem enough.

Which information do we really need in dominfo? I suspect all but the
xenstore flag would be better retrieved from xenstore via a different
interface. I don't think we want that information in the hypervisor
as well, so xenstore is the only alternative surviving a potential
dom0 reboot. And libxl_list_domain() isn't reading xenstore today
and it shouldn't do so in future.


Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Wei Liu]

On Thu, Jan 07, 2016 at 01:57:44PM +0100, Juergen Gross wrote:
> On 07/01/16 13:40, Wei Liu wrote:
> > On Thu, Jan 07, 2016 at 06:37:34AM +0100, Juergen Gross wrote:
> >> On 06/01/16 16:59, Ian Campbell wrote:
> >>> On Fri, 2015-12-18 at 15:10 +0100, Juergen Gross wrote:
> >>>> On 18/12/15 14:53, Andrew Cooper wrote:
> >>>>> On 18/12/15 13:14, Juergen Gross wrote:
> >>>>>> Add libxl_xenstore_domid() to obtain the domain id of the xenstore
> >>>>>> domain.
> >>>>>>
> >>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>>>
> >>>>> What are the expected semantics here? Would you expect it to return
> >>>>> domid 0 for a traditional setup, or are you wanting to use it as a
> >>>>> "does
> >>>>> a xenstored domain exist" test?
> >>>>
> >>>> The latter. It will be used in patch 13 to decide which domain to
> >>>> stop via "xl shutdown --all".
> >>>
> >>> ITYM "not stop".
> >>
> >> Well, yes, if you select which domains to stop you select which domains
> >> are not stopped, too. :-)
> >>
> >> I don't mind either wording. :-)
> >>
> >>> libxl already has interfaces for getting info about a
> >>> domain, libxl_domain_info libxl_list_domain etc, it seems like this
> >>> property should be added to that data rather than introducing a special
> >>> purpose API to get it. Especially given that xl shutdown already calls
> >>> libxl_list_domain.
> >>
> >> Okay, I can change it that way.
> >>
> >>> I'm unsure if (lib)xl ought to be special casing XS in this way, as opposed
> >>> to adding a more generic concept such as e.g. permanent domains, which
> >>> would be true for the xs domain but also for other special domains in the
> >>> future and could even be controlled by the user or toolstack (I'm thinking
> >>> you might want to set the flag on a driver domain for example).
> >>
> >> The xs domain has to be handled separately, as it is needed to run in
> >> order to be able to stop other domains in a clean way.
> >>
> >> In case dom0 reboot will be supported we need two different reboot
> >> modes: one with a hypervisor reboot implying all domains will be
> >> stopped (including the xs domain), and one without hypervisor reboot
> >> implying that all domains not requiring dom0 to be up all time will
> >> still be running after dom0 is up again.
> >>
> > 
> > For so long we've lumped together so many things in Dom0, so it would be
> > better there is clear definition what you would expect from rebooting
> > Dom0.
> > 
> > As far as I can tell, currently in a typical setup Dom0 serves at least
> > several purposes:
> > 
> > 1. Toosltack domain for managing VMs
> > 2. Driver domain for physical devices
> > 3. Running emulators
> > 4. Provide some services to DomU (I know there are people who do that)
> > 
> > Do we need provision for adding more flags or groups? One flag (as
> > suggested in subthread) doesn't seem enough.
> 
> Which information do we really need in dominfo? I suspect all but the
> xenstore flag would be better retrieved from xenstore via a different
> interface. I don't think we want that information in the hypervisor
> as well, so xenstore is the only alternative surviving a potential

Using Xenstore to store domain type would work, too. That would be one
way of "provision" to me. I was thinking about having more flags in HV
but in the end I deemed it a bad idea myself so I just asked you about
your thought.

So now the absolute bare minimum setup for Xen system is the hypervisor
plus xenstore domain. I think that's fine as long as it is clearly
communicated and agreed upon. :-)

Wei.

> dom0 reboot. And libxl_list_domain() isn't reading xenstore today
> and it shouldn't do so in future.
> 
> 
> Juergen

Re: [PATCH v2 03/13] libxl: provide a function to retrieve the xenstore domain id

[Wei Liu]

On Thu, Jan 07, 2016 at 11:36:08AM +0000, Ian Campbell wrote:
> On Thu, 2016-01-07 at 12:21 +0100, Juergen Gross wrote:
> > On 07/01/16 11:55, Ian Campbell wrote:
> > > On Thu, 2016-01-07 at 11:44 +0100, Juergen Gross wrote:
> > > > > > IMO the generic concept you are asking for should be added in a
> > > > > > separate patch handling stopping (and possibly rebooting) driver
> > > > > > domains in a clean way.
> > > > > 
> > > > > Since libxl has a stable API once we add something we need to
> > > > > continue
> > > > > supporting it, so we cannot (easily/cleanly) switch an xs specific
> > > > > scheme
> > > > > into a generic one later. That argues then for supporting the XS
> > > > > case
> > > > > via
> > > > > the generic mechanism now, even if we don't implement the other
> > > > > cases.
> > > > 
> > > > I can't see a scenario where the xenstore domain would have to be
> > > > stopped by dom0. Once you do it you'll never be able to connect to
> > > > it again without changing the xenbus driver interface, too. It is
> > > > the same reason why xenstored can't be restarted.
> > > > 
> > > > Driver domains are different and I think the interface to query a
> > > > domain whether it is a driver domain or whether it might survive a
> > > > dom0 reboot should be based on xenstore.
> > > > 
> > > > So a xenstore domain would always need special handling.
> > > 
> > > If there is really _never_ any reason to stop the xs domain then I
> > > think at
> > > the libxl API level a class of "never stop" domains would be better
> > > than
> > > special casing the xs, even if it turns out the only member of the set
> > > is
> > > xs at least we've given ourselves wriggle room if something else comes
> > > up
> > > in the future.
> > 
> > Okay, so this would translate to either:
> > 
> > - add a "never stop" flag to libxl_dominfo (can I do this without
> >   breaking the API?)
> > - add a new call interface to either check a single domain to be of
> >   the "never stop" class or to return a list of those domains.
> > 
> > Preferences?
> 
> Definitely the former, with a LIBXL_HAVE_ #define in libxl.h so consumers
> know they can use it.
> 
> Wei, Ian, do you agree with this approach?
> 

My concern in other sub-thread is addressed so this approach is fine by
me.

Wei.

> Ian.