Xen devel - Nov 2009 - [PATCH 1/3] libxenlight: Clean up logging arrangements

osdeps.[hc] previously mistakenly declared and defined [v]asprintf.
These functions are available in the libc on most platforms.  Also,
osdeps.h is used by xc.c but xc.c is not part of the library, so
osdeps.h is part of the public interface and should have a better name.

So now, instead:

 * osdeps.h is libxl_osdeps.h.

 * _GNU_SOURCE is #defined in libxl_osdeps.h so that we get the system
   [v]asprintf (and various other functions)

 * libxl_osdeps.h is included first in every libxl*.c file (it needs to
   be before any system headers so that _GNU_SOURCE) takes effect.

 * osdeps.[hc] only provide their own reimplementation of [v]asprintf
   if NEED_OWN_ASPRINTF is defined.  Currently it is not ever defined
   but this is provided for any platform which needs it.

 * While I was editing the #includes in each .c file, I put them all
   into the same order: "libxl_osdeps.h", then system headers,
   then local headers.

 * xs.h is included in libxl.h.  This is needed for "bool"; it has to
   not be typedefed in libxl.h because otherwise we get a duplicate
   definition when including xs.h.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/Makefile         |    3 +--
 tools/libxl/libxl.c          |    3 +++
 tools/libxl/libxl.h          |    5 ++---
 tools/libxl/libxl_device.c   |    7 +++++--
 tools/libxl/libxl_dom.c      |   16 ++++++++++------
 tools/libxl/libxl_exec.c     |    3 +++
 tools/libxl/libxl_internal.c |   10 +++++++---
 tools/libxl/libxl_osdeps.h   |   33 +++++++++++++++++++++++++++++++++
 tools/libxl/libxl_utils.c    |    6 ++++--
 tools/libxl/libxl_xshelp.c   |    7 +++++--
 tools/libxl/osdeps.c         |    4 ++++
 tools/libxl/osdeps.h         |   26 --------------------------
 tools/libxl/xl.c             |    7 +++++--
 13 files changed, 82 insertions(+), 48 deletions(-)
 create mode 100644 tools/libxl/libxl_osdeps.h
 delete mode 100644 tools/libxl/osdeps.h

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 492d6d9..0358cea 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -23,8 +23,7 @@ LIBCONFIG_URL ?= http://www.hyperrealm.com/libconfig
 LIBCONFIG_SOURCE = libconfig-1.3.2
 LIBCONFIG_OUTPUT = $(LIBCONFIG_SOURCE)/.libs
 
-LIBXL_OBJS-y -LIBXL_OBJS-$(CONFIG_Linux) += osdeps.o
+LIBXL_OBJS-y = osdeps.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_dom.o libxl_exec.o libxl_xshelp.o
libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 CLIENTS = xl
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5e2f23b..421628c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -14,6 +14,8 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -25,6 +27,7 @@
 #include <unistd.h> /* for write, unlink and close */
 #include <stdint.h>
 #include <inttypes.h>
+
 #include "libxl.h"
 #include "libxl_utils.h"
 #include "libxl_internal.h"
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0c6f168..ac4c79e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -15,14 +15,13 @@
 #ifndef LIBXL_H
 #define LIBXL_H
 
-#include "osdeps.h"
 #include <stdint.h>
 #include <stdarg.h>
 #include <netinet/in.h>
 #include <xenctrl.h>
-#include "xen_uuid.h"
+#include <xs.h>
 
-typedef int bool;
+#include "xen_uuid.h"
 
 typedef void (*libxl_log_callback)(void *userdata, int loglevel, const char
*file,
                                    int line, const char *func, char *s);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 82a2a30..451233f 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -14,15 +14,18 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <string.h>
 #include <stdio.h>
-#include "libxl.h"
-#include "libxl_internal.h"
 #include <sys/time.h> /* for struct timeval */
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "libxl.h"
+#include "libxl_internal.h"
+
 char *string_of_kinds[] = {
     [DEVICE_VIF] = "vif",
     [DEVICE_VBD] = "vbd",
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index eaea6f3..177c16c 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -13,17 +13,21 @@
  * GNU Lesser General Public License for more details.
  */
 
-#include "libxl.h"
-#include "libxl_internal.h"
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <inttypes.h>
-#include <xenguest.h>
-#include <xenctrl.h>
-#include <xc_dom.h>
 #include <string.h>
 #include <sys/time.h> /* for struct timeval */
 #include <unistd.h> /* for sleep(2) */
 
+#include <xenctrl.h>
+#include <xc_dom.h>
+#include <xenguest.h>
+
+#include "libxl.h"
+#include "libxl_internal.h"
+
 int is_hvm(struct libxl_ctx *ctx, uint32_t domid)
 {
     xc_domaininfo_t info;
@@ -110,7 +114,7 @@ int build_pv(struct libxl_ctx *ctx, uint32_t domid,
 
     dom = xc_dom_allocate(info->u.pv.cmdline, info->u.pv.features);
     if (!dom) {
-        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, dom, "xc_dom_allocate
failed");
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "xc_dom_allocate failed");
         return -1;
     }
     if ((ret = xc_dom_linux_build(ctx->xch, dom, domid, info->max_memkb /
1024,
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 8a589b6..5186ac8 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -15,9 +15,12 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <unistd.h>
 #include <stdlib.h>
+
 #include "libxl.h"
 #include "libxl_internal.h"
 
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 7ba0c97..a04ac8d 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -12,13 +12,17 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU Lesser General Public License for more details.
  */
-#include "libxl.h"
-#include "libxl_internal.h"
-#include "libxl_utils.h"
+
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <stdarg.h>
 #include <string.h>
 
+#include "libxl.h"
+#include "libxl_internal.h"
+#include "libxl_utils.h"
+
 int libxl_error_set(struct libxl_ctx *ctx, int code)
 {
     return 0;
diff --git a/tools/libxl/libxl_osdeps.h b/tools/libxl/libxl_osdeps.h
new file mode 100644
index 0000000..fc453c3
--- /dev/null
+++ b/tools/libxl/libxl_osdeps.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2009      Citrix Ltd.
+ * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+/*
+ * This header must be included first, before any system headers,
+ * so that _GNU_SOURCE takes effect properly.
+ */
+
+#ifndef LIBXL_OSDEP
+#define LIBXL_OSDEP
+
+#define _GNU_SOURCE
+
+#ifdef NEED_OWN_ASPRINTF
+#include <stdarg.h>
+
+int asprintf(char **buffer, char *fmt, ...);
+int vasprintf(char **buffer, const char *fmt, va_list ap);
+#endif /*NEED_OWN_ASPRINTF*/
+
+#endif
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 5048fd0..c174d0f 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -13,8 +13,8 @@
  * GNU Lesser General Public License for more details.
  */
 
-#include "libxl_utils.h"
-#include "libxl_internal.h"
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -24,6 +24,8 @@
 #include <ctype.h>
 #include <errno.h>
 
+#include "libxl_utils.h"
+#include "libxl_internal.h"
 
 
 unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb,
unsigned int smp_cpus)
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index f59eee7..87c494a 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -13,13 +13,16 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <string.h>
 #include <stddef.h>
-#include "libxl.h"
-#include "libxl_internal.h"
 #include <stdio.h>
 #include <stdarg.h>
 
+#include "libxl.h"
+#include "libxl_internal.h"
+
 char **libxl_xs_kvs_of_flexarray(struct libxl_ctx *ctx, flexarray_t *array, int
length)
 {
     char **kvs;
diff --git a/tools/libxl/osdeps.c b/tools/libxl/osdeps.c
index 81175aa..ad96480 100644
--- a/tools/libxl/osdeps.c
+++ b/tools/libxl/osdeps.c
@@ -19,6 +19,8 @@
 #include <sys/time.h>
 #include <stdlib.h>
 
+#ifdef NEED_OWN_ASPRINTF
+
 int vasprintf(char **buffer, const char *fmt, va_list ap)
 {
     int size = 0;
@@ -60,3 +62,5 @@ int asprintf(char **buffer, char *fmt, ...)
     va_end (ap);
     return status;
 }
+
+#endif
diff --git a/tools/libxl/osdeps.h b/tools/libxl/osdeps.h
deleted file mode 100644
index 5391727..0000000
--- a/tools/libxl/osdeps.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * Copyright (C) 2009      Citrix Ltd.
- * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- */
-
-#ifndef LIBXL_OSDEP
-#define LIBXL_OSDEP
-
-#include <stdarg.h>
-
-#if defined(__linux__)
-int asprintf(char **buffer, char *fmt, ...);
-int vasprintf(char **buffer, const char *fmt, va_list ap);
-#endif
-
-#endif
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 3b735ec..727fe4a 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -14,8 +14,8 @@
  * GNU Lesser General Public License for more details.
  */
 
-#include "libxl.h"
-#include "libxl_utils.h"
+#include "libxl_osdeps.h"
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -29,6 +29,9 @@
 #include <arpa/inet.h>
 #include <xenctrl.h>
 
+#include "libxl.h"
+#include "libxl_utils.h"
+
 void log_callback(void *userdata, int loglevel, const char *file, int line,
const char *func, char *s)
 {
     fprintf(stderr, "[%d] %s:%d:%s: %s\n", loglevel, file, line,
func, s);
-- 
1.5.6.5


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

* Introduce new variants of the logging functions which include
  errno values (converted using strerror) in the messages passed to the
  application''s logging callback.

* Use the new errno-including logging functions everywhere where
  appropriate.  In general, xc_... functions return errno values or 0;
  xs_... functions return 0 or -1 (or some such) setting errno.

* When libxl_xs_get_dompath fails, do not treat it as an allocation
  error.  It isn''t: it usually means xenstored failed.

* Remove many spurious \n''s from log messages.  (The applications log
  callback is expected to add a \n if it wants to do that, so libxl''s
  logging functions should be passes strings without \n.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |   91 +++++++++++++++++++++++------------------
 tools/libxl/libxl_device.c   |   27 +++++++++----
 tools/libxl/libxl_dom.c      |   11 +++--
 tools/libxl/libxl_exec.c     |    2 +-
 tools/libxl/libxl_internal.c |   39 +++++++++++++++---
 tools/libxl/libxl_internal.h |   11 ++++-
 tools/libxl/libxl_xshelp.c   |    5 ++
 7 files changed, 124 insertions(+), 62 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 69690e4..5e2f23b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -106,14 +106,17 @@ int libxl_domain_make(struct libxl_ctx *ctx,
libxl_domain_create_info *info,
 
     ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags,
domid);
     if (ret < 0) {
-        XL_LOG(ctx, XL_LOG_ERROR, "domain creation fail: %d", ret);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, ret, "domain creation
fail");
         return ERROR_FAIL;
     }
 
     dom_path = libxl_xs_get_dompath(ctx, *domid);
+    if (!dom_path)
+        return ERROR_FAIL;
+
     vm_path = libxl_sprintf(ctx, "/vm/%s", uuid_string);
     vss_path = libxl_sprintf(ctx, "/vss/%s", uuid_string);
-    if (!dom_path || !vm_path || !vss_path) {
+    if (!vm_path || !vss_path) {
         XL_LOG(ctx, XL_LOG_ERROR, "cannot allocate create paths");
         return ERROR_FAIL;
     }
@@ -326,6 +329,9 @@ int libxl_domain_shutdown(struct libxl_ctx *ctx, uint32_t
domid, int req)
         return ERROR_INVAL;
 
     dom_path = libxl_xs_get_dompath(ctx, domid);
+    if (!dom_path)
+        return ERROR_FAIL;
+
     shutdown_path = libxl_sprintf(ctx, "%s/control/shutdown",
dom_path);
 
     xs_write(ctx->xsh, XBT_NULL, shutdown_path, req_table[req],
strlen(req_table[req]));
@@ -347,20 +353,21 @@ static int libxl_destroy_device_model(struct libxl_ctx
*ctx, uint32_t domid)
 
     pid = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/%d/image/device-model-pid", domid));
     if (!pid) {
-        XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t find device
model''s pid\n");
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t find device
model''s pid");
         return -1;
     }
     xs_rm(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"/local/domain/0/device-model/%d", domid));
 
     ret = kill(atoi(pid), SIGHUP);
     if (ret < 0 && errno == ESRCH) {
-        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model already exited\n");
+        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model already exited");
         ret = 0;
     } else if (ret == 0) {
-        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model signaled\n");
+        XL_LOG(ctx, XL_LOG_DEBUG, "Device Model signaled");
         ret = 0;
     } else {
-        XL_LOG(ctx, XL_LOG_ERROR, "kill %d returned %d errno=%d\n",
atoi(pid), ret, errno);
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to kill Device Model
[%d]",
+                     atoi(pid));
     }
     return ret;
 }
@@ -369,38 +376,40 @@ int libxl_domain_destroy(struct libxl_ctx *ctx, uint32_t
domid, int force)
 {
     char *dom_path, vm_path[41];
     xen_uuid_t *uuid;
+    int rc;
 
     dom_path = libxl_xs_get_dompath(ctx, domid);
-    if (!dom_path) {
-        XL_LOG(ctx, XL_LOG_ERROR, "dompath doesn''t exist for
%d\n", domid);
+    if (!dom_path)
         return -1;
-    }
+
     if (libxl_domid_to_uuid(ctx, &uuid, domid) < 0) {
-        XL_LOG(ctx, XL_LOG_ERROR, "failed ot get uuid for %d\n",
domid);
+        XL_LOG(ctx, XL_LOG_ERROR, "failed ot get uuid for %d",
domid);
         return -1;
     }
     if (libxl_device_pci_shutdown(ctx, domid) < 0)
-        XL_LOG(ctx, XL_LOG_ERROR, "pci shutdown failed for domid
%d\n", domid);
+        XL_LOG(ctx, XL_LOG_ERROR, "pci shutdown failed for domid %d",
domid);
     xs_write(ctx->xsh, XBT_NULL,
              libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/command", domid),
              "shutdown", strlen("shutdown"));
-    if (xc_domain_pause(ctx->xch, domid) < 0) {
-        XL_LOG(ctx, XL_LOG_ERROR, "xc_domain_pause failed for %d\n",
domid);
+    rc = xc_domain_pause(ctx->xch, domid);
+    if (rc < 0) {
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_domain_pause failed for
%d", domid);
         return -1;
     }
-    if (xc_domain_destroy(ctx->xch, domid) < 0) {
-        XL_LOG(ctx, XL_LOG_ERROR, "xc_domain_destroy failed for
%d\n", domid);
+    rc = xc_domain_destroy(ctx->xch, domid);
+    if (rc < 0) {
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_domain_destroy failed
for %d", domid);
         return -1;
     }
     if (libxl_devices_destroy(ctx, domid, force) < 0)
-        XL_LOG(ctx, XL_LOG_ERROR, "libxl_destroy_devices failed for
%d\n", domid);
+        XL_LOG(ctx, XL_LOG_ERROR, "libxl_destroy_devices failed for
%d", domid);
     if (libxl_destroy_device_model(ctx, domid) < 0)
-        XL_LOG(ctx, XL_LOG_ERROR, "libxl_destroy_device_model failed for
%d\n", domid);
+        XL_LOG(ctx, XL_LOG_ERROR, "libxl_destroy_device_model failed for
%d", domid);
     if (!xs_rm(ctx->xsh, XBT_NULL, dom_path))
-        XL_LOG(ctx, XL_LOG_ERROR, "xs_rm failed for %s\n", dom_path);
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "xs_rm failed for %s",
dom_path);
     snprintf(vm_path, sizeof(vm_path), "/vm/%s",
libxl_uuid_to_string(ctx, uuid));
     if (!xs_rm(ctx->xsh, XBT_NULL, vm_path))
-        XL_LOG(ctx, XL_LOG_ERROR, "xs_rm failed for %s\n", vm_path);
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "xs_rm failed for %s",
vm_path);
     return 0;
 }
 
@@ -524,6 +533,8 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
         return ERROR_FAIL;
 
     dom_path = libxl_xs_get_dompath(ctx, info->domid);
+    if (!dom_path)
+        return ERROR_FAIL;
 
     path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d",
info->domid);
     xs_mkdir(ctx->xsh, XBT_NULL, path);
@@ -1017,7 +1028,7 @@ static int libxl_create_pci_backend(struct libxl_ctx *ctx,
uint32_t domid, libxl
     if (!back)
         return ERROR_NOMEM;
 
-    XL_LOG(ctx, XL_LOG_DEBUG, "Creating pci backend\n");
+    XL_LOG(ctx, XL_LOG_DEBUG, "Creating pci backend");
 
     /* add pci device */
     device.backend_devid = 0;
@@ -1088,7 +1099,7 @@ static int libxl_device_pci_add_xenstore(struct libxl_ctx
*ctx, uint32_t domid,
     if (!back)
         return ERROR_NOMEM;
 
-    XL_LOG(ctx, XL_LOG_DEBUG, "Adding new pci device to xenstore\n");
+    XL_LOG(ctx, XL_LOG_DEBUG, "Adding new pci device to xenstore");
     num = atoi(num_devs);
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "key-%d",
num));
     flexarray_set(back, boffset++, libxl_sprintf(ctx, PCI_BDF,
pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func));
@@ -1140,7 +1151,7 @@ static int libxl_device_pci_remove_xenstore(struct
libxl_ctx *ctx, uint32_t domi
 
     if (!is_hvm(ctx, domid)) {
         if (libxl_wait_for_backend(ctx, be_path, "4") < 0) {
-            XL_LOG(ctx, XL_LOG_DEBUG, "pci backend at %s is not
ready\n");
+            XL_LOG(ctx, XL_LOG_DEBUG, "pci backend at %s is not
ready");
             return -1;
         }
     }
@@ -1154,7 +1165,7 @@ static int libxl_device_pci_remove_xenstore(struct
libxl_ctx *ctx, uint32_t domi
         }
     }
     if (i == num) {
-        XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t find the device on
xenstore\n");
+        XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t find the device on
xenstore");
         return -1;
     }
 
@@ -1196,7 +1207,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
         if (libxl_wait_for_device_model(ctx, domid, "pci-inserted")
< 0)
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time\n");
+            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/parameter", domid);
         vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
         sscanf(vdevfn + 2, "%x", &pcidev->vdevfn);
@@ -1211,22 +1222,22 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
         int i;
 
         if (f == NULL) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t open %s\n",
sysfs_path);
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t open
%s", sysfs_path);
             return -1;
         }
         for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
-            fscanf(f, "0x%x 0x%x 0x%x\n", &start, &end,
&flags);
+            fscanf(f, "0x%x 0x%x 0x%x", &start, &end,
&flags);
             size = end - start + 1;
             if (start) {
                 if (flags & PCI_BAR_IO) {
                     rc = xc_domain_ioport_permission(ctx->xch, domid, start,
size, 1);
                     if (rc < 0)
-                        XL_LOG(ctx, XL_LOG_ERROR, "Error:
xc_domain_ioport_permission error 0x%x/0x%x:  %d\n", start, size, rc);
+                        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_ioport_permission error 0x%x/0x%x", start, size);
                 } else {
                     rc = xc_domain_iomem_permission(ctx->xch, domid,
start>>XC_PAGE_SHIFT,
                                                    
(size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 1);
                     if (rc < 0)
-                        XL_LOG(ctx, XL_LOG_ERROR, "Error:
xc_domain_iomem_permission error 0x%x/0x%x:  %d\n", start, size, rc);
+                        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_iomem_permission error 0x%x/0x%x", start, size);
                 }
             }
         }
@@ -1235,25 +1246,25 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
                                    pcidev->bus, pcidev->dev,
pcidev->func);
         f = fopen(sysfs_path, "r");
         if (f == NULL) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t open %s\n",
sysfs_path);
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t open
%s", sysfs_path);
             goto out;
         }
         fscanf(f, "%u", &irq);
         if (irq) {
             rc = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
             if (rc < 0) {
-                XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_physdev_map_pirq
irq=%d: %d\n", irq, rc);
+                XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_physdev_map_pirq irq=%d", irq);
             }
             rc = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
             if (rc < 0) {
-                XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_domain_irq_permission
irq=%d: %d\n", irq, rc);
+                XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_irq_permission irq=%d", irq);
             }
         }
         fclose(f);
     }
 out:
     if ((rc = xc_assign_device(ctx->xch, domid, pcidev->value)) < 0)
-        XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_assign_device error
%d\n", rc);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_assign_device
failed");
 
     libxl_device_pci_add_xenstore(ctx, domid, pcidev);
     return 0;
@@ -1280,7 +1291,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
         if (libxl_wait_for_device_model(ctx, domid, "pci-removed")
< 0) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time\n");
+            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1294,7 +1305,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
         int i;
 
         if (f == NULL) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t open %s\n",
sysfs_path);
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t open
%s", sysfs_path);
             goto skip1;
         }
         for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
@@ -1304,12 +1315,12 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
                 if (flags & PCI_BAR_IO) {
                     rc = xc_domain_ioport_permission(ctx->xch, domid, start,
size, 0);
                     if (rc < 0)
-                        XL_LOG(ctx, XL_LOG_ERROR, "Error:
xc_domain_ioport_permission error 0x%x/0x%x:  %d\n", start, size, rc);
+                        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"xc_domain_ioport_permission error 0x%x/0x%x", start, size);
                 } else {
                     rc = xc_domain_iomem_permission(ctx->xch, domid,
start>>XC_PAGE_SHIFT,
                                                    
(size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 0);
                     if (rc < 0)
-                        XL_LOG(ctx, XL_LOG_ERROR, "Error:
xc_domain_iomem_permission error 0x%x/0x%x:  %d\n", start, size, rc);
+                        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"xc_domain_iomem_permission error 0x%x/0x%x", start, size);
                 }
             }
         }
@@ -1319,18 +1330,18 @@ skip1:
                                    pcidev->bus, pcidev->dev,
pcidev->func);
         f = fopen(sysfs_path, "r");
         if (f == NULL) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t open %s\n",
sysfs_path);
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t open
%s", sysfs_path);
             goto out;
         }
         fscanf(f, "%u", &irq);
         if (irq) {
             rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
             if (rc < 0) {
-                XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_physdev_map_pirq
irq=%d: %d\n", irq, rc);
+                XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"xc_physdev_map_pirq irq=%d", irq);
             }
             rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
             if (rc < 0) {
-                XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_domain_irq_permission
irq=%d: %d\n", irq, rc);
+                XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"xc_domain_irq_permission irq=%d", irq);
             }
         }
         fclose(f);
@@ -1341,7 +1352,7 @@ out:
     libxl_device_pci_flr(ctx, pcidev->domain, pcidev->bus,
pcidev->dev, pcidev->func);
 
     if ((rc = xc_deassign_device(ctx->xch, domid, pcidev->value)) < 0)
-        XL_LOG(ctx, XL_LOG_ERROR, "Error: xc_deassign_device error
%d\n", rc);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_deassign_device
failed");
     return 0;
 }
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eee0a40..82a2a30 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -92,7 +92,7 @@ retry_transaction:
         if (errno == EAGAIN)
             goto retry_transaction;
         else
-            XL_LOG(ctx, XL_LOG_ERROR, "xs transaction failed
errno=%d\n", errno);
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "xs transaction failed");
     }
     return 0;
 }
@@ -213,7 +213,7 @@ int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t
domid, int force)
     path = libxl_sprintf(ctx, "/local/domain/%d/device", domid);
     l1 = libxl_xs_directory(ctx, XBT_NULL, path, &num1);
     if (!l1) {
-        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty\n", path);
+        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty", path);
         return -1;
     }
     for (i = 0; i < num1; i++) {
@@ -248,7 +248,7 @@ int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t
domid, int force)
                     if (!state || atoi(state) == 6) {
                         xs_unwatch(ctx->xsh, l1[0], l1[1]);
                         xs_rm(ctx->xsh, XBT_NULL, l1[1]);
-                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device
backend at %s\n", l1[1]);
+                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device
backend at %s", l1[1]);
                         n_watches--;
                     }
                 }
@@ -267,15 +267,20 @@ int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t
domid, int force)
 int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned int domain, unsigned
int bus,
                          unsigned int dev, unsigned int func)
 {
+    char *do_flr= "/sys/bus/pci/drivers/pciback/do_flr";
     FILE *fd;
 
-    fd = fopen("/sys/bus/pci/drivers/pciback/do_flr", "w");
+    fd = fopen(do_flr, "w");
     if (fd != NULL) {
         fprintf(fd, PCI_BDF, domain, bus, dev, func);
         fclose(fd);
         return 0;
     }
-    XL_LOG(ctx, XL_LOG_ERROR, "Pciback doesn''t support do_flr,
cannot flr the device\n");
+    if (errno == ENOENT) {
+        XL_LOG(ctx, XL_LOG_ERROR, "Pciback doesn''t support
do_flr, cannot flr the device");
+    } else {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access pciback path
%s", do_flr);
+    }
     return -1;
 }
 
@@ -303,7 +308,7 @@ int libxl_wait_for_device_model(struct libxl_ctx *ctx,
uint32_t domid, char *sta
             }
         }
     }
-    XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready\n");
+    XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready");
     return -1;
 }
 
@@ -317,7 +322,13 @@ int libxl_wait_for_backend(struct libxl_ctx *ctx, char
*be_path, char *state)
     while (watchdog > 0) {
         p = xs_read(ctx->xsh, XBT_NULL, path, &len);
         if (p == NULL) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Backend %s does not exist\n",
be_path);
+            if (errno == ENOENT) {
+                XL_LOG(ctx, XL_LOG_ERROR, "Backend %s does not
exist",
+                       be_path);
+            } else {
+                XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access backend
%s",
+                       be_path);
+            }
             return -1;
         } else {
             if (!strcmp(p, state)) {
@@ -328,7 +339,7 @@ int libxl_wait_for_backend(struct libxl_ctx *ctx, char
*be_path, char *state)
             }
         }
     }
-    XL_LOG(ctx, XL_LOG_ERROR, "Backend %s not ready\n", be_path);
+    XL_LOG(ctx, XL_LOG_ERROR, "Backend %s not ready", be_path);
     return -1;
 }
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a98daee..eaea6f3 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -83,6 +83,9 @@ int build_post(struct libxl_ctx *ctx, uint32_t domid,
     ents[9] = libxl_sprintf(ctx, "%lu", state->store_mfn);
 
     dom_path = libxl_xs_get_dompath(ctx, domid);
+    if (!dom_path)
+        return ERROR_FAIL;
+    
     vm_path = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(ctx,
"%s/vm", dom_path), NULL);
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
@@ -107,7 +110,7 @@ int build_pv(struct libxl_ctx *ctx, uint32_t domid,
 
     dom = xc_dom_allocate(info->u.pv.cmdline, info->u.pv.features);
     if (!dom) {
-        XL_LOG(ctx, XL_LOG_ERROR, "xc_dom_allocate failed: %d", dom);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, dom, "xc_dom_allocate
failed");
         return -1;
     }
     if ((ret = xc_dom_linux_build(ctx->xch, dom, domid, info->max_memkb /
1024,
@@ -115,7 +118,7 @@ int build_pv(struct libxl_ctx *ctx, uint32_t domid,
                                   state->store_port,
&state->store_mfn,
                                   state->console_port,
&state->console_mfn)) != 0) {
         xc_dom_release(dom);
-        XL_LOG(ctx, XL_LOG_ERROR, "xc_dom_linux_build failed: %d",
ret);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, ret, "xc_dom_linux_build
failed");
         return -2;
     }
     xc_dom_release(dom);
@@ -129,7 +132,7 @@ int build_hvm(struct libxl_ctx *ctx, uint32_t domid,
 
     ret = xc_hvm_build(ctx->xch, domid, info->max_memkb / 1024,
info->kernel);
     if (ret) {
-        XL_LOG(ctx, XL_LOG_ERROR, "hvm building failed: %d", ret);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, ret, "hvm building
failed");
         return ERROR_FAIL;
     }
     ret = hvm_build_set_params(ctx->xch, domid, info->u.hvm.apic,
info->u.hvm.acpi,
@@ -137,7 +140,7 @@ int build_hvm(struct libxl_ctx *ctx, uint32_t domid,
                                info->max_vcpus,
                                state->store_port, &state->store_mfn);
     if (ret) {
-        XL_LOG(ctx, XL_LOG_ERROR, "hvm build set params failed: %d",
ret);
+        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, ret, "hvm build set params
failed");
         return ERROR_FAIL;
     }
     xc_cpuid_apply_policy(ctx->xch, domid);
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 8d7928b..8a589b6 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -28,7 +28,7 @@ int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int
stdoutfd, int stderrfd,
 
     pid = fork();
     if (pid == -1) {
-        XL_LOG(ctx, XL_LOG_ERROR, "fork failed");
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "fork failed");
         return -1;
     }
     if (pid == 0) {
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 407cf22..7ba0c97 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -146,14 +146,41 @@ char *libxl_dirname(struct libxl_ctx *ctx, const char *s)
     return ptr;
 }
 
-void xl_log(struct libxl_ctx *ctx, int loglevel, const char *file, int line,
const char *func, char *fmt, ...)
+void xl_logv(struct libxl_ctx *ctx, int loglevel, int errnoval,
+             const char *file, int line, const char *func,
+             char *fmt, va_list ap)
 {
-    va_list ap;
+    char *enomem = "[out of memory formatting log message]";
     char *s;
-    va_start(ap, fmt);
-    vasprintf(&s, fmt, ap);
-    va_end(ap);
+    int rc;
+
+    rc = vasprintf(&s, fmt, ap);
+    if (rc<0) { s = enomem; goto x; }
+
+    if (errnoval >= 0) {
+        char *errstr, *snew;
+        errstr = strerror(errnoval);
+        if (errstr)
+            rc = asprintf(&snew, "%s: %s", s, errstr);
+        else
+            rc = asprintf(&snew, "%s: unknown error number %d",
s, errnoval);
+        free(s);
+        if (rc<0) { s = enomem; goto x; }
+        s = snew;
+    }
 
+ x:
     ctx->log_callback(ctx->log_userdata, loglevel, file, line, func, s);
-    free(s);
+    if (s != enomem)
+        free(s);
+}
+
+void xl_log(struct libxl_ctx *ctx, int loglevel, int errnoval,
+            const char *file, int line,
+            const char *func, char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    xl_logv(ctx, loglevel, errnoval, file, line, func, fmt, ap);
+    va_end(ap);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d6835be..046e51d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -37,9 +37,13 @@
 #define XL_LOGGING_ENABLED
 
 #ifdef XL_LOGGING_ENABLED
-#define XL_LOG(ctx, loglevel, _f, _a...)   xl_log(ctx, loglevel, __FILE__,
__LINE__, __func__, _f, ##_a)
+#define XL_LOG(ctx, loglevel, _f, _a...)   xl_log(ctx, loglevel, -1, __FILE__,
__LINE__, __func__, _f, ##_a)
+#define XL_LOG_ERRNO(ctx, loglevel, _f, _a...)   xl_log(ctx, loglevel, errno,
__FILE__, __LINE__, __func__, _f, ##_a)
+#define XL_LOG_ERRNOVAL(ctx, errnoval, loglevel, _f, _a...)   xl_log(ctx,
loglevel, errnoval, __FILE__, __LINE__, __func__, _f, ##_a)
 #else
 #define XL_LOG(ctx, loglevel, _f, _a...)
+#define XL_LOG_ERRNO(ctx, loglevel, _f, _a...)
+#define XL_LOG_ERRNOVAL(ctx, loglevel, errnoval, _f, _a...)
 #endif
 
 #define XL_LOG_DEBUG 3
@@ -47,7 +51,8 @@
 #define XL_LOG_WARNING 1
 #define XL_LOG_ERROR 0
 
-void xl_log(struct libxl_ctx *ctx, int loglevel, const char *file, int line,
const char *func, char *fmt, ...);
+void xl_logv(struct libxl_ctx *ctx, int errnoval, int loglevel, const char
*file, int line, const char *func, char *fmt, va_list al);
+void xl_log(struct libxl_ctx *ctx, int errnoval, int loglevel, const char
*file, int line, const char *func, char *fmt, ...);
 
 struct libxl_domain_build_state_ {
     uint32_t store_port;
@@ -97,7 +102,7 @@ int libxl_xs_writev(struct libxl_ctx *ctx, xs_transaction_t
t,
                     char *dir, char **kvs);
 int libxl_xs_write(struct libxl_ctx *ctx, xs_transaction_t t,
                    char *path, char *fmt, ...);
-char *libxl_xs_get_dompath(struct libxl_ctx *ctx, uint32_t domid);
+char *libxl_xs_get_dompath(struct libxl_ctx *ctx, uint32_t domid); // logs errs
 char *libxl_xs_read(struct libxl_ctx *ctx, xs_transaction_t t, char *path);
 char **libxl_xs_directory(struct libxl_ctx *ctx, xs_transaction_t t, char
*path, unsigned int *nb);
 
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 6286d02..f59eee7 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -95,6 +95,11 @@ char * libxl_xs_read(struct libxl_ctx *ctx, xs_transaction_t
t, char *path)
 char *libxl_xs_get_dompath(struct libxl_ctx *ctx, uint32_t domid)
 {
     char *s = xs_get_domain_path(ctx->xsh, domid);
+    if (!s) {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "failed to get dompath for
%lu",
+                     domid);
+        return NULL;
+    }
     libxl_ptr_add(ctx, s);
     return s;
 }
-- 
1.5.6.5


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

This patch makes xl create check whether qemu-dm has started
correctly, and causes it to fail immediately with appropriate errors
if not.  There are other bugfixes too.

More specifically:

 * libxl_create_device_model forks twice rather than once so that the
   process which calls libxl does not end up being the actual parent
   of qemu.  That avoids the need for the qemu-dm process to be reaped
   at some indefinite time in the future.

 * The first fork generates an intermediate process which is
   responsible for writing the qemu-dm pid to xenstore and then merely
   waits to collect and report on qemu-dm''s exit status during
   startup.  New arguments to libxl_create_device_model allow the
   preservation of its pid so that a later call can check whether the
   startup is successful.

 * The core of this functionality (the double fork, waitpid, signal
   handling and so forth) is abstracted away into a new facility
   libxl_spawn_... in libxl_exec.c.

Consequential changes:

 * libxl_wait_for_device_model now takes a callback function parameter
   which is called repeatedly in the loop iteration and allows the
   caller to abort the wait.

 * libxl_exec no longer calls fork; there is a new libxl_fork.

 * There is a hook to override waitpid, which will be necessary for
   some callers.

Remaining problems and other issues I noticed or we found:

 * The error handling is rather inconsistent still and lacking in
   places.
 * destroy_device_model can kill random dom0 processes (!)

Changes since v2 of this patch:
 * Various other changes split out into earlier patches in this series
 * xl.c''s create_domain checking for errors in its libxl calls
   deferred for a future patch to avoid rebase churn.
 * New libxl_spawn_... abstraction.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |   94 +++++++++++++++++----
 tools/libxl/libxl.h          |   20 ++++-
 tools/libxl/libxl_device.c   |   11 ++-
 tools/libxl/libxl_exec.c     |  189 ++++++++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_internal.h |   49 ++++++++++-
 tools/libxl/osdeps.c         |    2 +
 tools/libxl/xl.c             |   19 ++++-
 7 files changed, 344 insertions(+), 40 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 421628c..29143cf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -23,10 +23,12 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <sys/select.h>
+#include <sys/wait.h>
 #include <signal.h>
 #include <unistd.h> /* for write, unlink and close */
 #include <stdint.h>
 #include <inttypes.h>
+#include <assert.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"
@@ -43,6 +45,8 @@ int libxl_ctx_init(struct libxl_ctx *ctx)
 
     ctx->xch = xc_interface_open();
     ctx->xsh = xs_daemon_open();
+
+    ctx->waitpid_instead= libxl_waitpid_instead_default;
     return 0;
 }
 
@@ -520,16 +524,41 @@ static char ** libxl_build_device_model_args(struct
libxl_ctx *ctx,
     return (char **) flexarray_contents(dm_args);
 }
 
+struct libxl_device_model_starting {
+    struct libxl_spawn_starting for_spawn; /* first! */
+    char *dom_path;
+    int domid;
+};
+
+void dm_xenstore_record_pid(struct libxl_ctx *ctx, void *for_spawn,
+                            pid_t innerchild) {
+    struct libxl_device_model_starting *starting = for_spawn;
+    struct libxl_ctx clone;
+    char *kvs[3];
+    
+    clone = *ctx;
+    clone.xsh = xs_daemon_open();
+    /* we mustn''t use the parent''s handle in the child */
+
+    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
+    kvs[1] = libxl_sprintf(ctx, "%d", innerchild);
+    kvs[2] = NULL;
+    libxl_xs_writev(ctx, XBT_NULL, starting->dom_path, kvs);
+}
+
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs)
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r)
 {
     char *dom_path, *path, *logfile, *logfile_new;
-    char *kvs[3];
     struct stat stat_buf;
-    int logfile_w, null, pid;
-    int i;
+    int logfile_w, null;
+    int i, rc;
     char **args;
+    struct libxl_spawn_starting buf_spawn, *for_spawn;
+
+    *starting_r= 0;
 
     args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
     if (!args)
@@ -559,18 +588,50 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
     logfile = libxl_sprintf(ctx, "/var/log/xen/qemu-dm-%s.log",
info->dom_name);
     logfile_w = open(logfile, O_WRONLY|O_CREAT, 0644);
     null = open("/dev/null", O_RDONLY);
-    pid = libxl_exec(ctx, null, logfile_w, logfile_w, info->device_model,
args);
+
+    if (starting_r) {
+        *starting_r= libxl_calloc(ctx, sizeof(**starting_r), 1);
+        if (!*starting_r) return ERROR_NOMEM;
+        (*starting_r)->domid= info->domid;
+        for_spawn= &(*starting_r)->for_spawn;
+    } else {
+        for_spawn= &buf_spawn;
+    }
+    rc = libxl_spawn_spawn(ctx, for_spawn, "device model",
+                           dm_xenstore_record_pid);
+    if (rc < 0) goto xit;
+    if (!rc) { /* inner child */
+        libxl_exec(ctx, null, logfile_w, logfile_w,
+                   info->device_model, args);
+    }
+
+    rc = 0;
+ xit:
     close(null);
     close(logfile_w);
 
-    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
-    kvs[1] = libxl_sprintf(ctx, "%d", pid);
-    kvs[2] = NULL;
-    libxl_xs_writev(ctx, XBT_NULL, dom_path, kvs);
+    return rc;
+}
 
-    return 0;
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting) {
+    int rc;
+    rc = libxl_spawn_detach(ctx, &starting->for_spawn);
+    libxl_free(ctx, starting);
+    return rc;
+}
+
+
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                                       libxl_device_model_starting *starting) {
+    int problem = libxl_wait_for_device_model(ctx, starting->domid,
"running",
+                                              libxl_spawn_check,
+                                              &starting->for_spawn);
+    int detach = libxl_detach_device_model(ctx, starting);
+    return problem ? problem : detach;
 }
 
+
 /******************************************************************************/
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
@@ -917,12 +978,13 @@ static int libxl_build_xenpv_qemu_args(struct libxl_ctx
*ctx,
 }
 
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console)
+                            int num_console, libxl_device_console *console,
+                            struct libxl_device_model_starting **starting_r)
 {
     libxl_device_model_info info;
 
     libxl_build_xenpv_qemu_args(ctx, vfb, num_console, console, &info);
-    libxl_create_device_model(ctx, &info, NULL, 0);
+    libxl_create_device_model(ctx, &info, NULL, 0, starting_r);
     return 0;
 }
 
@@ -1195,7 +1257,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1209,7 +1271,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
                            pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted")
< 0)
+        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted",
0,0) < 0)
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/parameter", domid);
         vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
@@ -1283,7 +1345,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1293,7 +1355,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
                        pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed")
< 0) {
+        if (libxl_wait_for_device_model(ctx, domid, "pci-removed",
0,0) < 0) {
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
             return -1;
         }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ac4c79e..29ffde2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -41,6 +41,11 @@ struct libxl_ctx {
     /* mini-GC */
     int alloc_maxsize;
     void **alloc_ptrs;
+
+    /* for callers who reap children willy-nilly; caller must only
+     * set this after libxl_init and before any other call - or
+     * may leave them untouched */
+    int (*waitpid_instead)(pid_t pid, int *status, int flags);
 };
 
 typedef struct {
@@ -235,11 +240,22 @@ int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t
domid);
 struct libxl_dominfo * libxl_domain_list(struct libxl_ctx *ctx, int
*nb_domain);
 xc_dominfo_t * libxl_domain_infolist(struct libxl_ctx *ctx, int *nb_domain);
 
+typedef struct libxl_device_model_starting libxl_device_model_starting;
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs);
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r);
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console);
+                            int num_console, libxl_device_console *console,
+                            libxl_device_model_starting **starting_r);
+  /* Caller must either: pass starting_r==0, or on successful
+   * return pass *starting_r (which will be non-0) to
+   * libxl_confirm_device_model or libxl_detach_device_model. */
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+  /* DM is detached even if error is returned */
 
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
 int libxl_device_disk_clean_shutdown(struct libxl_ctx *ctx, uint32_t domid);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 451233f..39932a6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -287,12 +287,17 @@ int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned
int domain, unsigned in
     return -1;
 }
 
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state)
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata)
 {
     char path[50];
     char *p;
     int watchdog = 100;
     unsigned int len;
+    int rc;
 
     snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
     while (watchdog > 0) {
@@ -310,6 +315,10 @@ int libxl_wait_for_device_model(struct libxl_ctx *ctx,
uint32_t domid, char *sta
                 watchdog--;
             }
         }
+        if (check_callback) {
+            rc = check_callback(ctx, check_callback_userdata);
+            if (rc) return rc;
+        }
     }
     XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready");
     return -1;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 5186ac8..6a3373e 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -18,34 +18,191 @@
 #include "libxl_osdeps.h"
 
 #include <stdio.h>
+#include <string.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <unistd.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include "libxl.h"
 #include "libxl_internal.h"
 
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args)
+pid_t libxl_fork(struct libxl_ctx *ctx)
 {
-    int pid, i;
+    pid_t pid;
 
     pid = fork();
     if (pid == -1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "fork failed");
         return -1;
     }
-    if (pid == 0) {
-        /* child */
-        if (stdinfd != -1)
-            dup2(stdinfd, STDIN_FILENO);
-        if (stdoutfd != -1)
-            dup2(stdoutfd, STDOUT_FILENO);
-        if (stderrfd != -1)
-            dup2(stderrfd, STDERR_FILENO);
-        for (i = 4; i < 256; i++)
-            close(i);
-        execv(arg0, args);
-        exit(256);
-    }
+    
     return pid;
 }
+
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args)
+     /* call this in the child */
+{
+    int i;
+
+    if (stdinfd != -1)
+        dup2(stdinfd, STDIN_FILENO);
+    if (stdoutfd != -1)
+        dup2(stdoutfd, STDOUT_FILENO);
+    if (stderrfd != -1)
+        dup2(stderrfd, STDERR_FILENO);
+    for (i = 4; i < 256; i++)
+        close(i);
+    execv(arg0, args);
+    XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "exec %s failed", arg0);
+    _exit(-1);
+}
+
+void libxl_report_child_exitstatus(struct libxl_ctx *ctx,
+                                   const char *what, pid_t pid, int status) {
+    /* treats all exit statuses as errors; if that''s not what you
want,
+     * check status yourself first */
+    
+    if (WIFEXITED(status)) {
+        int st= WEXITSTATUS(status);
+        if (st)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] exited"
+                   " with error status %d", what, (unsigned long)pid,
st);
+        else
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] unexpectedly"
+                   " exited status zero", what, (unsigned long)pid);
+    } else if (WIFSIGNALED(status)) {
+        int sig= WTERMSIG(status);
+        const char *str= strsignal(sig);
+        const char *coredump= WCOREDUMP(status) ? " (core dumped)" :
"";
+        if (str)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to"
+                   " fatal signal %s%s", what, (unsigned long)pid,
+                   str, coredump);
+        else 
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to unknown"
+                   " fatal signal number %d%s", what, (unsigned
long)pid,
+                   sig, coredump);
+    } else {
+        XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] gave unknown"
+               " wait status 0x%x", what, (unsigned long)pid,
status);
+    }
+}
+
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags) {
+    return waitpid(pid,status,flags);
+}
+
+
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild)) {
+    pid_t child, got;
+    int status;
+    pid_t intermediate;
+
+    if (for_spawn) {
+        for_spawn->what= strdup(what);
+        if (!for_spawn->what) return ERROR_NOMEM;
+    }
+
+    intermediate = libxl_fork(ctx);
+    if (intermediate==-1) {
+        if (for_spawn) free(for_spawn->what);
+        return ERROR_FAIL;
+    }
+    if (intermediate) {
+        /* parent */
+        if (for_spawn) for_spawn->intermediate= intermediate;
+        return 1;
+    }
+    
+    /* we are now the intermediate process */
+
+    child = libxl_fork(ctx);
+    if (!child) return 0; /* caller runs child code */
+    if (child<0) exit(255);
+
+    intermediate_hook(ctx, for_spawn, child);
+
+    if (!for_spawn) _exit(0); /* just detach then */
+
+    got = ctx->waitpid_instead(child, &status, 0);
+    assert(got == child);
+
+    libxl_report_child_exitstatus(ctx, what, child, status);
+    _exit(WIFEXITED(status) ? WEXITSTATUS(status) :
+          WIFSIGNALED(status) && WTERMSIG(status)<127
+          ? WTERMSIG(status)+128 : -1);
+}
+
+static void report_spawn_intermediate_status(struct libxl_ctx *ctx,
+                                 struct libxl_spawn_starting *for_spawn,
+                                 int status) {
+    if (!WIFEXITED(status)) {
+        /* intermediate process did the logging itself if it exited */
+        char *intermediate_what+            libxl_sprintf(ctx,
+                          "%s intermediate process (startup
monitor)",
+                          for_spawn->what);
+        libxl_report_child_exitstatus(ctx, intermediate_what,
+                                      for_spawn->intermediate, status);
+    }
+}
+
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn) {
+    int r, status;
+    pid_t got;
+    int rc = 0;
+
+    if (!for_spawn) return 0;
+    
+    if (for_spawn->intermediate) {
+        r = kill(for_spawn->intermediate, SIGKILL);
+        if (r) {
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
+                         "could not kill %s intermediate process
[%ld]",
+                         for_spawn->what,
+                         (unsigned long)for_spawn->intermediate);
+            abort(); /* things are very wrong */
+        }
+        got = ctx->waitpid_instead(for_spawn->intermediate, &status,
0);
+        assert(got == for_spawn->intermediate);
+        if (!(WIFSIGNALED(status) && WTERMSIG(status)==SIGKILL)) {
+            report_spawn_intermediate_status(ctx, for_spawn, status);
+            rc = ERROR_FAIL;
+        }
+        for_spawn->intermediate = 0;
+    }
+
+    free(for_spawn->what);
+    for_spawn->what = 0;
+    
+    return rc;
+}
+
+int libxl_spawn_check(struct libxl_ctx *ctx, void *for_spawn_void) {
+    struct libxl_spawn_starting *for_spawn = for_spawn_void;
+    pid_t got;
+    int status;
+
+    if (!for_spawn) return 0;
+    
+    assert(for_spawn->intermediate);
+    got = ctx->waitpid_instead(for_spawn->intermediate, &status,
WNOHANG);
+    if (!got) return 0;
+
+    assert(got == for_spawn->intermediate);
+    report_spawn_intermediate_status(ctx, for_spawn, status);
+    
+    for_spawn->intermediate= 0;
+    return ERROR_FAIL;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 046e51d..957bd9e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -135,7 +135,11 @@ int libxl_device_generic_add(struct libxl_ctx *ctx,
libxl_device *device,
                              char **bents, char **fents);
 int libxl_device_destroy(struct libxl_ctx *ctx, char *be_path, int force);
 int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t domid, int force);
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state);
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata);
 int libxl_wait_for_backend(struct libxl_ctx *ctx, char *be_path, char *state);
 int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned int domain, unsigned
int bus,
                          unsigned int dev, unsigned int func);
@@ -146,8 +150,47 @@ int hvm_build_set_params(int handle, uint32_t domid,
                          int vcpus, int store_evtchn, unsigned long
*store_mfn);
 
 /* xl_exec */
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args);
+
+ /* higher-level double-fork and separate detach eg as for device models */
+
+struct libxl_spawn_starting {
+    /* put this in your own stateu structure as returned to application */
+    /* all fields are private to libxl_spawn_... */
+    pid_t intermediate;
+    char *what; /* malloc''d in spawn_spawn */
+};
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild));
+  /* Logs errors.  A copy of "what" is taken.  Return values:
+   *  < 0   error, for_spawn need not be detached
+   *   +1   caller is now the inner child, should probably call libxl_exec
+   *    0   caller is the parent, must call detach on *for_spawn eventually
+   * Caller, may pass 0 for for_spawn, in which case no need to detach.
+   */
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn);
+  /* Logs errors.  Idempotent, but only permitted after successful
+   * call to libxl_spawn_spawn, and no point calling it again if it fails. */
+int libxl_spawn_check(struct libxl_ctx *ctx,
+                      void *for_spawn);
+  /* Logs errors but also returns them.
+   * for_spawn must actually be a  struct libxl_spawn_starting*  but
+   * we take void* so you can pass this function directly to
+   * libxl_wait_for_device_model.  Caller must still call detach. */
+
+ /* low-level stuff, for synchronous subprocesses etc. */
+
+pid_t libxl_fork(struct libxl_ctx *ctx); // logs errors
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args); // logs errors, never returns
+void libxl_log_child_exitstatus(struct libxl_ctx *ctx,
+                                const char *what, pid_t pid, int status);
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags);
 
 #endif
 
diff --git a/tools/libxl/osdeps.c b/tools/libxl/osdeps.c
index ad96480..b146a9d 100644
--- a/tools/libxl/osdeps.c
+++ b/tools/libxl/osdeps.c
@@ -13,6 +13,8 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <unistd.h>
 #include <stdarg.h>
 #include <stdio.h>
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 727fe4a..6d43f8b 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -699,6 +699,15 @@ skip_pci:
     config_destroy(&config);
 }
 
+#define MUST( call ) ({                                                 \
+        int must_rc = (call);                                           \
+        if (must_rc) {                                                  \
+            fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n",    
\
+                    __FILE__,__LINE__, must_rc, #call);                 \
+            exit(-must_rc);                                             \
+        }                                                               \
+    })
+
 static void create_domain(int debug, const char *filename)
 {
     struct libxl_ctx ctx;
@@ -715,6 +724,7 @@ static void create_domain(int debug, const char *filename)
     libxl_device_console console;
     int num_disks = 0, num_vifs = 0, num_pcidevs = 0, num_vfbs = 0, num_vkbs =
0;
     int i;
+    libxl_device_model_starting *dm_starting = 0;
 
     printf("Parsing config file %s\n", filename);
     parse_config_file(filename, &info1, &info2, &disks,
&num_disks, &vifs, &num_vifs, &pcidevs, &num_pcidevs,
&vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
@@ -736,7 +746,8 @@ static void create_domain(int debug, const char *filename)
     }
     if (info1.hvm) {
         device_model_info_domid_fixup(&dm_info, domid);
-        libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs);
+        MUST( libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs,
+                                        &dm_starting) );
     } else {
         for (i = 0; i < num_vfbs; i++) {
             vfb_info_domid_fixup(vfbs + i, domid);
@@ -750,10 +761,14 @@ static void create_domain(int debug, const char *filename)
             console.constype = CONSTYPE_IOEMU;
         libxl_device_console_add(&ctx, domid, &console);
         if (num_vfbs)
-            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console);
+            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console,
&dm_starting);
     }
+
     for (i = 0; i < num_pcidevs; i++)
         libxl_device_pci_add(&ctx, domid, &pcidevs[i]);
+    if (dm_starting)
+        MUST( libxl_confirm_device_model_startup(&ctx, dm_starting) );
+    
     libxl_domain_unpause(&ctx, domid);
 
 }
-- 
1.5.6.5


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

Ian Jackson writes ("[PATCH 1/3] libxenlight: Clean up logging
arrangements"):
> * Introduce new variants of the logging functions which include
>   errno values (converted using strerror) in the messages passed to the
>   application''s logging callback.
These three patches should replace my earlier patches.  I repost them
all firstly because of a silly error (transposed arguments) in the
logging patch, and secondly to make the dependency order clear.

Ian.

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

This patch doesn''t correctly write the qemu pid on xenstore.
Could you please fix and resend it?

On Wed, 18 Nov 2009, Ian Jackson wrote:
> This patch makes xl create check whether qemu-dm has started
> correctly, and causes it to fail immediately with appropriate errors
> if not.  There are other bugfixes too.
> 
> More specifically:
> 
>  * libxl_create_device_model forks twice rather than once so that the
>    process which calls libxl does not end up being the actual parent
>    of qemu.  That avoids the need for the qemu-dm process to be reaped
>    at some indefinite time in the future.
> 
>  * The first fork generates an intermediate process which is
>    responsible for writing the qemu-dm pid to xenstore and then merely
>    waits to collect and report on qemu-dm''s exit status during
>    startup.  New arguments to libxl_create_device_model allow the
>    preservation of its pid so that a later call can check whether the
>    startup is successful.
> 
>  * The core of this functionality (the double fork, waitpid, signal
>    handling and so forth) is abstracted away into a new facility
>    libxl_spawn_... in libxl_exec.c.
> 
> Consequential changes:
> 
>  * libxl_wait_for_device_model now takes a callback function parameter
>    which is called repeatedly in the loop iteration and allows the
>    caller to abort the wait.
> 
>  * libxl_exec no longer calls fork; there is a new libxl_fork.
> 
>  * There is a hook to override waitpid, which will be necessary for
>    some callers.
> 
> Remaining problems and other issues I noticed or we found:
> 
>  * The error handling is rather inconsistent still and lacking in
>    places.
>  * destroy_device_model can kill random dom0 processes (!)
> 
> Changes since v2 of this patch:
>  * Various other changes split out into earlier patches in this series
>  * xl.c''s create_domain checking for errors in its libxl calls
>    deferred for a future patch to avoid rebase churn.
>  * New libxl_spawn_... abstraction.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |   94 +++++++++++++++++----
>  tools/libxl/libxl.h          |   20 ++++-
>  tools/libxl/libxl_device.c   |   11 ++-
>  tools/libxl/libxl_exec.c     |  189
++++++++++++++++++++++++++++++++++++++----
>  tools/libxl/libxl_internal.h |   49 ++++++++++-
>  tools/libxl/osdeps.c         |    2 +
>  tools/libxl/xl.c             |   19 ++++-
>  7 files changed, 344 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 421628c..29143cf 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -23,10 +23,12 @@
>  #include <sys/types.h>
>  #include <fcntl.h>
>  #include <sys/select.h>
> +#include <sys/wait.h>
>  #include <signal.h>
>  #include <unistd.h> /* for write, unlink and close */
>  #include <stdint.h>
>  #include <inttypes.h>
> +#include <assert.h>
> 
>  #include "libxl.h"
>  #include "libxl_utils.h"
> @@ -43,6 +45,8 @@ int libxl_ctx_init(struct libxl_ctx *ctx)
> 
>      ctx->xch = xc_interface_open();
>      ctx->xsh = xs_daemon_open();
> +
> +    ctx->waitpid_instead= libxl_waitpid_instead_default;
>      return 0;
>  }
> 
> @@ -520,16 +524,41 @@ static char ** libxl_build_device_model_args(struct
libxl_ctx *ctx,
>      return (char **) flexarray_contents(dm_args);
>  }
> 
> +struct libxl_device_model_starting {
> +    struct libxl_spawn_starting for_spawn; /* first! */
> +    char *dom_path;
> +    int domid;
> +};
> +
> +void dm_xenstore_record_pid(struct libxl_ctx *ctx, void *for_spawn,
> +                            pid_t innerchild) {
> +    struct libxl_device_model_starting *starting = for_spawn;
> +    struct libxl_ctx clone;
> +    char *kvs[3];
> +
> +    clone = *ctx;
> +    clone.xsh = xs_daemon_open();
> +    /* we mustn''t use the parent''s handle in the child
*/
> +
> +    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
> +    kvs[1] = libxl_sprintf(ctx, "%d", innerchild);
> +    kvs[2] = NULL;
> +    libxl_xs_writev(ctx, XBT_NULL, starting->dom_path, kvs);
> +}
> +
>  int libxl_create_device_model(struct libxl_ctx *ctx,
>                                libxl_device_model_info *info,
> -                              libxl_device_nic *vifs, int num_vifs)
> +                              libxl_device_nic *vifs, int num_vifs,
> +                              libxl_device_model_starting **starting_r)
>  {
>      char *dom_path, *path, *logfile, *logfile_new;
> -    char *kvs[3];
>      struct stat stat_buf;
> -    int logfile_w, null, pid;
> -    int i;
> +    int logfile_w, null;
> +    int i, rc;
>      char **args;
> +    struct libxl_spawn_starting buf_spawn, *for_spawn;
> +
> +    *starting_r= 0;
> 
>      args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
>      if (!args)
> @@ -559,18 +588,50 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
>      logfile = libxl_sprintf(ctx, "/var/log/xen/qemu-dm-%s.log",
info->dom_name);
>      logfile_w = open(logfile, O_WRONLY|O_CREAT, 0644);
>      null = open("/dev/null", O_RDONLY);
> -    pid = libxl_exec(ctx, null, logfile_w, logfile_w,
info->device_model, args);
> +
> +    if (starting_r) {
> +        *starting_r= libxl_calloc(ctx, sizeof(**starting_r), 1);
> +        if (!*starting_r) return ERROR_NOMEM;
> +        (*starting_r)->domid= info->domid;
> +        for_spawn= &(*starting_r)->for_spawn;
> +    } else {
> +        for_spawn= &buf_spawn;
> +    }
> +    rc = libxl_spawn_spawn(ctx, for_spawn, "device model",
> +                           dm_xenstore_record_pid);
> +    if (rc < 0) goto xit;
> +    if (!rc) { /* inner child */
> +        libxl_exec(ctx, null, logfile_w, logfile_w,
> +                   info->device_model, args);
> +    }
> +
> +    rc = 0;
> + xit:
>      close(null);
>      close(logfile_w);
> 
> -    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
> -    kvs[1] = libxl_sprintf(ctx, "%d", pid);
> -    kvs[2] = NULL;
> -    libxl_xs_writev(ctx, XBT_NULL, dom_path, kvs);
> +    return rc;
> +}
> 
> -    return 0;
> +int libxl_detach_device_model(struct libxl_ctx *ctx,
> +                              libxl_device_model_starting *starting) {
> +    int rc;
> +    rc = libxl_spawn_detach(ctx, &starting->for_spawn);
> +    libxl_free(ctx, starting);
> +    return rc;
> +}
> +
> +
> +int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
> +                                       libxl_device_model_starting
*starting) {
> +    int problem = libxl_wait_for_device_model(ctx, starting->domid,
"running",
> +                                              libxl_spawn_check,
> +                                             
&starting->for_spawn);
> +    int detach = libxl_detach_device_model(ctx, starting);
> +    return problem ? problem : detach;
>  }
> 
> +
> 
/******************************************************************************/
>  int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
>  {
> @@ -917,12 +978,13 @@ static int libxl_build_xenpv_qemu_args(struct
libxl_ctx *ctx,
>  }
> 
>  int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
> -                            int num_console, libxl_device_console
*console)
> +                            int num_console, libxl_device_console
*console,
> +                            struct libxl_device_model_starting
**starting_r)
>  {
>      libxl_device_model_info info;
> 
>      libxl_build_xenpv_qemu_args(ctx, vfb, num_console, console,
&info);
> -    libxl_create_device_model(ctx, &info, NULL, 0);
> +    libxl_create_device_model(ctx, &info, NULL, 0, starting_r);
>      return 0;
>  }
> 
> @@ -1195,7 +1257,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_pci
> 
>      hvm = is_hvm(ctx, domid);
>      if (hvm) {
> -        if (libxl_wait_for_device_model(ctx, domid, "running")
< 0) {
> +        if (libxl_wait_for_device_model(ctx, domid, "running",
0,0) < 0) {
>              return -1;
>          }
>          snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
> @@ -1209,7 +1271,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_pci
>                             pcidev->bus, pcidev->dev,
pcidev->func);
>          snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
>          xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
> -        if (libxl_wait_for_device_model(ctx, domid,
"pci-inserted") < 0)
> +        if (libxl_wait_for_device_model(ctx, domid,
"pci-inserted", 0,0) < 0)
>              XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
>          snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/parameter", domid);
>          vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
> @@ -1283,7 +1345,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
> 
>      hvm = is_hvm(ctx, domid);
>      if (hvm) {
> -        if (libxl_wait_for_device_model(ctx, domid, "running")
< 0) {
> +        if (libxl_wait_for_device_model(ctx, domid, "running",
0,0) < 0) {
>              return -1;
>          }
>          snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
> @@ -1293,7 +1355,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
>                         pcidev->bus, pcidev->dev, pcidev->func);
>          snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
>          xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
> -        if (libxl_wait_for_device_model(ctx, domid,
"pci-removed") < 0) {
> +        if (libxl_wait_for_device_model(ctx, domid,
"pci-removed", 0,0) < 0) {
>              XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
>              return -1;
>          }
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index ac4c79e..29ffde2 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -41,6 +41,11 @@ struct libxl_ctx {
>      /* mini-GC */
>      int alloc_maxsize;
>      void **alloc_ptrs;
> +
> +    /* for callers who reap children willy-nilly; caller must only
> +     * set this after libxl_init and before any other call - or
> +     * may leave them untouched */
> +    int (*waitpid_instead)(pid_t pid, int *status, int flags);
>  };
> 
>  typedef struct {
> @@ -235,11 +240,22 @@ int libxl_domain_unpause(struct libxl_ctx *ctx,
uint32_t domid);
>  struct libxl_dominfo * libxl_domain_list(struct libxl_ctx *ctx, int
*nb_domain);
>  xc_dominfo_t * libxl_domain_infolist(struct libxl_ctx *ctx, int
*nb_domain);
> 
> +typedef struct libxl_device_model_starting libxl_device_model_starting;
>  int libxl_create_device_model(struct libxl_ctx *ctx,
>                                libxl_device_model_info *info,
> -                              libxl_device_nic *vifs, int num_vifs);
> +                              libxl_device_nic *vifs, int num_vifs,
> +                              libxl_device_model_starting **starting_r);
>  int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
> -                            int num_console, libxl_device_console
*console);
> +                            int num_console, libxl_device_console
*console,
> +                            libxl_device_model_starting **starting_r);
> +  /* Caller must either: pass starting_r==0, or on successful
> +   * return pass *starting_r (which will be non-0) to
> +   * libxl_confirm_device_model or libxl_detach_device_model. */
> +int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
> +                              libxl_device_model_starting *starting);
> +int libxl_detach_device_model(struct libxl_ctx *ctx,
> +                              libxl_device_model_starting *starting);
> +  /* DM is detached even if error is returned */
> 
>  int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
>  int libxl_device_disk_clean_shutdown(struct libxl_ctx *ctx, uint32_t
domid);
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 451233f..39932a6 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -287,12 +287,17 @@ int libxl_device_pci_flr(struct libxl_ctx *ctx,
unsigned int domain, unsigned in
>      return -1;
>  }
> 
> -int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid,
char *state)
> +int libxl_wait_for_device_model(struct libxl_ctx *ctx,
> +                                uint32_t domid, char *state,
> +                                int (*check_callback)(struct libxl_ctx
*ctx,
> +                                                      void *userdata),
> +                                void *check_callback_userdata)
>  {
>      char path[50];
>      char *p;
>      int watchdog = 100;
>      unsigned int len;
> +    int rc;
> 
>      snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
>      while (watchdog > 0) {
> @@ -310,6 +315,10 @@ int libxl_wait_for_device_model(struct libxl_ctx *ctx,
uint32_t domid, char *sta
>                  watchdog--;
>              }
>          }
> +        if (check_callback) {
> +            rc = check_callback(ctx, check_callback_userdata);
> +            if (rc) return rc;
> +        }
>      }
>      XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready");
>      return -1;
> diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
> index 5186ac8..6a3373e 100644
> --- a/tools/libxl/libxl_exec.c
> +++ b/tools/libxl/libxl_exec.c
> @@ -18,34 +18,191 @@
>  #include "libxl_osdeps.h"
> 
>  #include <stdio.h>
> +#include <string.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <unistd.h>
> +#include <assert.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> 
>  #include "libxl.h"
>  #include "libxl_internal.h"
> 
> -int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int
stderrfd,
> -               char *arg0, char **args)
> +pid_t libxl_fork(struct libxl_ctx *ctx)
>  {
> -    int pid, i;
> +    pid_t pid;
> 
>      pid = fork();
>      if (pid == -1) {
>          XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "fork failed");
>          return -1;
>      }
> -    if (pid == 0) {
> -        /* child */
> -        if (stdinfd != -1)
> -            dup2(stdinfd, STDIN_FILENO);
> -        if (stdoutfd != -1)
> -            dup2(stdoutfd, STDOUT_FILENO);
> -        if (stderrfd != -1)
> -            dup2(stderrfd, STDERR_FILENO);
> -        for (i = 4; i < 256; i++)
> -            close(i);
> -        execv(arg0, args);
> -        exit(256);
> -    }
> +
>      return pid;
>  }
> +
> +void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int
stderrfd,
> +                char *arg0, char **args)
> +     /* call this in the child */
> +{
> +    int i;
> +
> +    if (stdinfd != -1)
> +        dup2(stdinfd, STDIN_FILENO);
> +    if (stdoutfd != -1)
> +        dup2(stdoutfd, STDOUT_FILENO);
> +    if (stderrfd != -1)
> +        dup2(stderrfd, STDERR_FILENO);
> +    for (i = 4; i < 256; i++)
> +        close(i);
> +    execv(arg0, args);
> +    XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "exec %s failed", arg0);
> +    _exit(-1);
> +}
> +
> +void libxl_report_child_exitstatus(struct libxl_ctx *ctx,
> +                                   const char *what, pid_t pid, int
status) {
> +    /* treats all exit statuses as errors; if that''s not what you
want,
> +     * check status yourself first */
> +
> +    if (WIFEXITED(status)) {
> +        int st= WEXITSTATUS(status);
> +        if (st)
> +            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] exited"
> +                   " with error status %d", what, (unsigned
long)pid, st);
> +        else
> +            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] unexpectedly"
> +                   " exited status zero", what, (unsigned
long)pid);
> +    } else if (WIFSIGNALED(status)) {
> +        int sig= WTERMSIG(status);
> +        const char *str= strsignal(sig);
> +        const char *coredump= WCOREDUMP(status) ? " (core
dumped)" : "";
> +        if (str)
> +            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to"
> +                   " fatal signal %s%s", what, (unsigned
long)pid,
> +                   str, coredump);
> +        else
> +            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to
unknown"
> +                   " fatal signal number %d%s", what, (unsigned
long)pid,
> +                   sig, coredump);
> +    } else {
> +        XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] gave unknown"
> +               " wait status 0x%x", what, (unsigned long)pid,
status);
> +    }
> +}
> +
> +pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags) {
> +    return waitpid(pid,status,flags);
> +}
> +
> +
> +
> +int libxl_spawn_spawn(struct libxl_ctx *ctx,
> +                      struct libxl_spawn_starting *for_spawn,
> +                      const char *what,
> +                      void (*intermediate_hook)(struct libxl_ctx *ctx,
> +                                                void *for_spawn,
> +                                                pid_t innerchild)) {
> +    pid_t child, got;
> +    int status;
> +    pid_t intermediate;
> +
> +    if (for_spawn) {
> +        for_spawn->what= strdup(what);
> +        if (!for_spawn->what) return ERROR_NOMEM;
> +    }
> +
> +    intermediate = libxl_fork(ctx);
> +    if (intermediate==-1) {
> +        if (for_spawn) free(for_spawn->what);
> +        return ERROR_FAIL;
> +    }
> +    if (intermediate) {
> +        /* parent */
> +        if (for_spawn) for_spawn->intermediate= intermediate;
> +        return 1;
> +    }
> +
> +    /* we are now the intermediate process */
> +
> +    child = libxl_fork(ctx);
> +    if (!child) return 0; /* caller runs child code */
> +    if (child<0) exit(255);
> +
> +    intermediate_hook(ctx, for_spawn, child);
> +
> +    if (!for_spawn) _exit(0); /* just detach then */
> +
> +    got = ctx->waitpid_instead(child, &status, 0);
> +    assert(got == child);
> +
> +    libxl_report_child_exitstatus(ctx, what, child, status);
> +    _exit(WIFEXITED(status) ? WEXITSTATUS(status) :
> +          WIFSIGNALED(status) && WTERMSIG(status)<127
> +          ? WTERMSIG(status)+128 : -1);
> +}
> +
> +static void report_spawn_intermediate_status(struct libxl_ctx *ctx,
> +                                 struct libxl_spawn_starting *for_spawn,
> +                                 int status) {
> +    if (!WIFEXITED(status)) {
> +        /* intermediate process did the logging itself if it exited */
> +        char *intermediate_what> +            libxl_sprintf(ctx,
> +                          "%s intermediate process (startup
monitor)",
> +                          for_spawn->what);
> +        libxl_report_child_exitstatus(ctx, intermediate_what,
> +                                      for_spawn->intermediate, status);
> +    }
> +}
> +
> +int libxl_spawn_detach(struct libxl_ctx *ctx,
> +                       struct libxl_spawn_starting *for_spawn) {
> +    int r, status;
> +    pid_t got;
> +    int rc = 0;
> +
> +    if (!for_spawn) return 0;
> +
> +    if (for_spawn->intermediate) {
> +        r = kill(for_spawn->intermediate, SIGKILL);
> +        if (r) {
> +            XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
> +                         "could not kill %s intermediate process
[%ld]",
> +                         for_spawn->what,
> +                         (unsigned long)for_spawn->intermediate);
> +            abort(); /* things are very wrong */
> +        }
> +        got = ctx->waitpid_instead(for_spawn->intermediate,
&status, 0);
> +        assert(got == for_spawn->intermediate);
> +        if (!(WIFSIGNALED(status) && WTERMSIG(status)==SIGKILL)) {
> +            report_spawn_intermediate_status(ctx, for_spawn, status);
> +            rc = ERROR_FAIL;
> +        }
> +        for_spawn->intermediate = 0;
> +    }
> +
> +    free(for_spawn->what);
> +    for_spawn->what = 0;
> +
> +    return rc;
> +}
> +
> +int libxl_spawn_check(struct libxl_ctx *ctx, void *for_spawn_void) {
> +    struct libxl_spawn_starting *for_spawn = for_spawn_void;
> +    pid_t got;
> +    int status;
> +
> +    if (!for_spawn) return 0;
> +
> +    assert(for_spawn->intermediate);
> +    got = ctx->waitpid_instead(for_spawn->intermediate, &status,
WNOHANG);
> +    if (!got) return 0;
> +
> +    assert(got == for_spawn->intermediate);
> +    report_spawn_intermediate_status(ctx, for_spawn, status);
> +
> +    for_spawn->intermediate= 0;
> +    return ERROR_FAIL;
> +}
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 046e51d..957bd9e 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -135,7 +135,11 @@ int libxl_device_generic_add(struct libxl_ctx *ctx,
libxl_device *device,
>                               char **bents, char **fents);
>  int libxl_device_destroy(struct libxl_ctx *ctx, char *be_path, int force);
>  int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t domid, int
force);
> -int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid,
char *state);
> +int libxl_wait_for_device_model(struct libxl_ctx *ctx,
> +                                uint32_t domid, char *state,
> +                                int (*check_callback)(struct libxl_ctx
*ctx,
> +                                                      void *userdata),
> +                                void *check_callback_userdata);
>  int libxl_wait_for_backend(struct libxl_ctx *ctx, char *be_path, char
*state);
>  int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned int domain,
unsigned int bus,
>                           unsigned int dev, unsigned int func);
> @@ -146,8 +150,47 @@ int hvm_build_set_params(int handle, uint32_t domid,
>                           int vcpus, int store_evtchn, unsigned long
*store_mfn);
> 
>  /* xl_exec */
> -int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int
stderrfd,
> -               char *arg0, char **args);
> +
> + /* higher-level double-fork and separate detach eg as for device models
*/
> +
> +struct libxl_spawn_starting {
> +    /* put this in your own stateu structure as returned to application */
> +    /* all fields are private to libxl_spawn_... */
> +    pid_t intermediate;
> +    char *what; /* malloc''d in spawn_spawn */
> +};
> +
> +int libxl_spawn_spawn(struct libxl_ctx *ctx,
> +                      struct libxl_spawn_starting *for_spawn,
> +                      const char *what,
> +                      void (*intermediate_hook)(struct libxl_ctx *ctx,
> +                                                void *for_spawn,
> +                                                pid_t innerchild));
> +  /* Logs errors.  A copy of "what" is taken.  Return values:
> +   *  < 0   error, for_spawn need not be detached
> +   *   +1   caller is now the inner child, should probably call libxl_exec
> +   *    0   caller is the parent, must call detach on *for_spawn
eventually
> +   * Caller, may pass 0 for for_spawn, in which case no need to detach.
> +   */
> +int libxl_spawn_detach(struct libxl_ctx *ctx,
> +                       struct libxl_spawn_starting *for_spawn);
> +  /* Logs errors.  Idempotent, but only permitted after successful
> +   * call to libxl_spawn_spawn, and no point calling it again if it fails.
*/
> +int libxl_spawn_check(struct libxl_ctx *ctx,
> +                      void *for_spawn);
> +  /* Logs errors but also returns them.
> +   * for_spawn must actually be a  struct libxl_spawn_starting*  but
> +   * we take void* so you can pass this function directly to
> +   * libxl_wait_for_device_model.  Caller must still call detach. */
> +
> + /* low-level stuff, for synchronous subprocesses etc. */
> +
> +pid_t libxl_fork(struct libxl_ctx *ctx); // logs errors
> +void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int
stderrfd,
> +                char *arg0, char **args); // logs errors, never returns
> +void libxl_log_child_exitstatus(struct libxl_ctx *ctx,
> +                                const char *what, pid_t pid, int status);
> +pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags);
> 
>  #endif
> 
> diff --git a/tools/libxl/osdeps.c b/tools/libxl/osdeps.c
> index ad96480..b146a9d 100644
> --- a/tools/libxl/osdeps.c
> +++ b/tools/libxl/osdeps.c
> @@ -13,6 +13,8 @@
>   * GNU Lesser General Public License for more details.
>   */
> 
> +#include "libxl_osdeps.h"
> +
>  #include <unistd.h>
>  #include <stdarg.h>
>  #include <stdio.h>
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 727fe4a..6d43f8b 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -699,6 +699,15 @@ skip_pci:
>      config_destroy(&config);
>  }
> 
> +#define MUST( call ) ({                                                 \
> +        int must_rc = (call);                                           \
> +        if (must_rc) {                                                  \
> +            fprintf(stderr,"xl: fatal error: %s:%d, rc=%d:
%s\n",       \
> +                    __FILE__,__LINE__, must_rc, #call);                 \
> +            exit(-must_rc);                                             \
> +        }                                                               \
> +    })
> +
>  static void create_domain(int debug, const char *filename)
>  {
>      struct libxl_ctx ctx;
> @@ -715,6 +724,7 @@ static void create_domain(int debug, const char
*filename)
>      libxl_device_console console;
>      int num_disks = 0, num_vifs = 0, num_pcidevs = 0, num_vfbs = 0,
num_vkbs = 0;
>      int i;
> +    libxl_device_model_starting *dm_starting = 0;
> 
>      printf("Parsing config file %s\n", filename);
>      parse_config_file(filename, &info1, &info2, &disks,
&num_disks, &vifs, &num_vifs, &pcidevs, &num_pcidevs,
&vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
> @@ -736,7 +746,8 @@ static void create_domain(int debug, const char
*filename)
>      }
>      if (info1.hvm) {
>          device_model_info_domid_fixup(&dm_info, domid);
> -        libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs);
> +        MUST( libxl_create_device_model(&ctx, &dm_info, vifs,
num_vifs,
> +                                        &dm_starting) );
>      } else {
>          for (i = 0; i < num_vfbs; i++) {
>              vfb_info_domid_fixup(vfbs + i, domid);
> @@ -750,10 +761,14 @@ static void create_domain(int debug, const char
*filename)
>              console.constype = CONSTYPE_IOEMU;
>          libxl_device_console_add(&ctx, domid, &console);
>          if (num_vfbs)
> -            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console);
> +            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console,
&dm_starting);
>      }
> +
>      for (i = 0; i < num_pcidevs; i++)
>          libxl_device_pci_add(&ctx, domid, &pcidevs[i]);
> +    if (dm_starting)
> +        MUST( libxl_confirm_device_model_startup(&ctx, dm_starting) );
> +
>      libxl_domain_unpause(&ctx, domid);
> 
>  }
> --
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/3] libxenlight: check
for early failures of qemu-dm (v3)"):
> This patch doesn''t correctly write the qemu pid on xenstore.
> Could you please fix and resend it?
Sorry about that.  Thanks for pointing me to a 2.6.27 kernel for
testing.  I''ve fixed this and tested it and now I can create and
destroy domains.


commit 6704f959a0ea543f6bbf07e0a58e8b84cdef2236
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Wed Nov 18 15:24:17 2009 +0000

libxenlight: check for early failures of qemu-dm (v4)

This patch makes xl create check whether qemu-dm has started
correctly, and causes it to fail immediately with appropriate errors
if not.  There are other bugfixes too.

More specifically:

 * libxl_create_device_model forks twice rather than once so that the
   process which calls libxl does not end up being the actual parent
   of qemu.  That avoids the need for the qemu-dm process to be reaped
   at some indefinite time in the future.

 * The first fork generates an intermediate process which is
   responsible for writing the qemu-dm pid to xenstore and then merely
   waits to collect and report on qemu-dm''s exit status during
   startup.  New arguments to libxl_create_device_model allow the
   preservation of its pid so that a later call can check whether the
   startup is successful.

 * The core of this functionality (the double fork, waitpid, signal
   handling and so forth) is abstracted away into a new facility
   libxl_spawn_... in libxl_exec.c.

Consequential changes:

 * libxl_wait_for_device_model now takes a callback function parameter
   which is called repeatedly in the loop iteration and allows the
   caller to abort the wait.

 * libxl_exec no longer calls fork; there is a new libxl_fork.

 * There is a hook to override waitpid, which will be necessary for
   some callers.

Remaining problems and other issues I noticed or we found:

 * The error handling is rather inconsistent still and lacking in
   places.
 * destroy_device_model can kill random dom0 processes (!)

Changes since v3 of this patch:
 * Fixed bug which prevented qemu-dm from being properly killed on domain
   shutdown.  (Failure to set dom_path in dm starting state.)
 * Slightly tidy some memory allocation and error handling in
   dm_xenstore_record_pid.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>


diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 421628c..29143cf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -23,10 +23,12 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <sys/select.h>
+#include <sys/wait.h>
 #include <signal.h>
 #include <unistd.h> /* for write, unlink and close */
 #include <stdint.h>
 #include <inttypes.h>
+#include <assert.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"
@@ -43,6 +45,8 @@ int libxl_ctx_init(struct libxl_ctx *ctx)
 
     ctx->xch = xc_interface_open();
     ctx->xsh = xs_daemon_open();
+
+    ctx->waitpid_instead= libxl_waitpid_instead_default;
     return 0;
 }
 
@@ -520,16 +524,41 @@ static char ** libxl_build_device_model_args(struct
libxl_ctx *ctx,
     return (char **) flexarray_contents(dm_args);
 }
 
+struct libxl_device_model_starting {
+    struct libxl_spawn_starting for_spawn; /* first! */
+    char *dom_path;
+    int domid;
+};
+
+void dm_xenstore_record_pid(struct libxl_ctx *ctx, void *for_spawn,
+                            pid_t innerchild) {
+    struct libxl_device_model_starting *starting = for_spawn;
+    struct libxl_ctx clone;
+    char *kvs[3];
+    
+    clone = *ctx;
+    clone.xsh = xs_daemon_open();
+    /* we mustn''t use the parent''s handle in the child */
+
+    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
+    kvs[1] = libxl_sprintf(ctx, "%d", innerchild);
+    kvs[2] = NULL;
+    libxl_xs_writev(ctx, XBT_NULL, starting->dom_path, kvs);
+}
+
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs)
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r)
 {
     char *dom_path, *path, *logfile, *logfile_new;
-    char *kvs[3];
     struct stat stat_buf;
-    int logfile_w, null, pid;
-    int i;
+    int logfile_w, null;
+    int i, rc;
     char **args;
+    struct libxl_spawn_starting buf_spawn, *for_spawn;
+
+    *starting_r= 0;
 
     args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
     if (!args)
@@ -559,18 +588,50 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
     logfile = libxl_sprintf(ctx, "/var/log/xen/qemu-dm-%s.log",
info->dom_name);
     logfile_w = open(logfile, O_WRONLY|O_CREAT, 0644);
     null = open("/dev/null", O_RDONLY);
-    pid = libxl_exec(ctx, null, logfile_w, logfile_w, info->device_model,
args);
+
+    if (starting_r) {
+        *starting_r= libxl_calloc(ctx, sizeof(**starting_r), 1);
+        if (!*starting_r) return ERROR_NOMEM;
+        (*starting_r)->domid= info->domid;
+        for_spawn= &(*starting_r)->for_spawn;
+    } else {
+        for_spawn= &buf_spawn;
+    }
+    rc = libxl_spawn_spawn(ctx, for_spawn, "device model",
+                           dm_xenstore_record_pid);
+    if (rc < 0) goto xit;
+    if (!rc) { /* inner child */
+        libxl_exec(ctx, null, logfile_w, logfile_w,
+                   info->device_model, args);
+    }
+
+    rc = 0;
+ xit:
     close(null);
     close(logfile_w);
 
-    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
-    kvs[1] = libxl_sprintf(ctx, "%d", pid);
-    kvs[2] = NULL;
-    libxl_xs_writev(ctx, XBT_NULL, dom_path, kvs);
+    return rc;
+}
 
-    return 0;
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting) {
+    int rc;
+    rc = libxl_spawn_detach(ctx, &starting->for_spawn);
+    libxl_free(ctx, starting);
+    return rc;
+}
+
+
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                                       libxl_device_model_starting *starting) {
+    int problem = libxl_wait_for_device_model(ctx, starting->domid,
"running",
+                                              libxl_spawn_check,
+                                              &starting->for_spawn);
+    int detach = libxl_detach_device_model(ctx, starting);
+    return problem ? problem : detach;
 }
 
+
 /******************************************************************************/
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
@@ -917,12 +978,13 @@ static int libxl_build_xenpv_qemu_args(struct libxl_ctx
*ctx,
 }
 
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console)
+                            int num_console, libxl_device_console *console,
+                            struct libxl_device_model_starting **starting_r)
 {
     libxl_device_model_info info;
 
     libxl_build_xenpv_qemu_args(ctx, vfb, num_console, console, &info);
-    libxl_create_device_model(ctx, &info, NULL, 0);
+    libxl_create_device_model(ctx, &info, NULL, 0, starting_r);
     return 0;
 }
 
@@ -1195,7 +1257,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1209,7 +1271,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
                            pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted")
< 0)
+        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted",
0,0) < 0)
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/parameter", domid);
         vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
@@ -1283,7 +1345,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1293,7 +1355,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
                        pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed")
< 0) {
+        if (libxl_wait_for_device_model(ctx, domid, "pci-removed",
0,0) < 0) {
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
             return -1;
         }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ac4c79e..29ffde2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -41,6 +41,11 @@ struct libxl_ctx {
     /* mini-GC */
     int alloc_maxsize;
     void **alloc_ptrs;
+
+    /* for callers who reap children willy-nilly; caller must only
+     * set this after libxl_init and before any other call - or
+     * may leave them untouched */
+    int (*waitpid_instead)(pid_t pid, int *status, int flags);
 };
 
 typedef struct {
@@ -235,11 +240,22 @@ int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t
domid);
 struct libxl_dominfo * libxl_domain_list(struct libxl_ctx *ctx, int
*nb_domain);
 xc_dominfo_t * libxl_domain_infolist(struct libxl_ctx *ctx, int *nb_domain);
 
+typedef struct libxl_device_model_starting libxl_device_model_starting;
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs);
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r);
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console);
+                            int num_console, libxl_device_console *console,
+                            libxl_device_model_starting **starting_r);
+  /* Caller must either: pass starting_r==0, or on successful
+   * return pass *starting_r (which will be non-0) to
+   * libxl_confirm_device_model or libxl_detach_device_model. */
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+  /* DM is detached even if error is returned */
 
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
 int libxl_device_disk_clean_shutdown(struct libxl_ctx *ctx, uint32_t domid);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 451233f..39932a6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -287,12 +287,17 @@ int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned
int domain, unsigned in
     return -1;
 }
 
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state)
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata)
 {
     char path[50];
     char *p;
     int watchdog = 100;
     unsigned int len;
+    int rc;
 
     snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
     while (watchdog > 0) {
@@ -310,6 +315,10 @@ int libxl_wait_for_device_model(struct libxl_ctx *ctx,
uint32_t domid, char *sta
                 watchdog--;
             }
         }
+        if (check_callback) {
+            rc = check_callback(ctx, check_callback_userdata);
+            if (rc) return rc;
+        }
     }
     XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready");
     return -1;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 5186ac8..6a3373e 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -18,34 +18,191 @@
 #include "libxl_osdeps.h"
 
 #include <stdio.h>
+#include <string.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <unistd.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include "libxl.h"
 #include "libxl_internal.h"
 
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args)
+pid_t libxl_fork(struct libxl_ctx *ctx)
 {
-    int pid, i;
+    pid_t pid;
 
     pid = fork();
     if (pid == -1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "fork failed");
         return -1;
     }
-    if (pid == 0) {
-        /* child */
-        if (stdinfd != -1)
-            dup2(stdinfd, STDIN_FILENO);
-        if (stdoutfd != -1)
-            dup2(stdoutfd, STDOUT_FILENO);
-        if (stderrfd != -1)
-            dup2(stderrfd, STDERR_FILENO);
-        for (i = 4; i < 256; i++)
-            close(i);
-        execv(arg0, args);
-        exit(256);
-    }
+    
     return pid;
 }
+
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args)
+     /* call this in the child */
+{
+    int i;
+
+    if (stdinfd != -1)
+        dup2(stdinfd, STDIN_FILENO);
+    if (stdoutfd != -1)
+        dup2(stdoutfd, STDOUT_FILENO);
+    if (stderrfd != -1)
+        dup2(stderrfd, STDERR_FILENO);
+    for (i = 4; i < 256; i++)
+        close(i);
+    execv(arg0, args);
+    XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "exec %s failed", arg0);
+    _exit(-1);
+}
+
+void libxl_report_child_exitstatus(struct libxl_ctx *ctx,
+                                   const char *what, pid_t pid, int status) {
+    /* treats all exit statuses as errors; if that''s not what you
want,
+     * check status yourself first */
+    
+    if (WIFEXITED(status)) {
+        int st= WEXITSTATUS(status);
+        if (st)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] exited"
+                   " with error status %d", what, (unsigned long)pid,
st);
+        else
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] unexpectedly"
+                   " exited status zero", what, (unsigned long)pid);
+    } else if (WIFSIGNALED(status)) {
+        int sig= WTERMSIG(status);
+        const char *str= strsignal(sig);
+        const char *coredump= WCOREDUMP(status) ? " (core dumped)" :
"";
+        if (str)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to"
+                   " fatal signal %s%s", what, (unsigned long)pid,
+                   str, coredump);
+        else 
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to unknown"
+                   " fatal signal number %d%s", what, (unsigned
long)pid,
+                   sig, coredump);
+    } else {
+        XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] gave unknown"
+               " wait status 0x%x", what, (unsigned long)pid,
status);
+    }
+}
+
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags) {
+    return waitpid(pid,status,flags);
+}
+
+
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild)) {
+    pid_t child, got;
+    int status;
+    pid_t intermediate;
+
+    if (for_spawn) {
+        for_spawn->what= strdup(what);
+        if (!for_spawn->what) return ERROR_NOMEM;
+    }
+
+    intermediate = libxl_fork(ctx);
+    if (intermediate==-1) {
+        if (for_spawn) free(for_spawn->what);
+        return ERROR_FAIL;
+    }
+    if (intermediate) {
+        /* parent */
+        if (for_spawn) for_spawn->intermediate= intermediate;
+        return 1;
+    }
+    
+    /* we are now the intermediate process */
+
+    child = libxl_fork(ctx);
+    if (!child) return 0; /* caller runs child code */
+    if (child<0) exit(255);
+
+    intermediate_hook(ctx, for_spawn, child);
+
+    if (!for_spawn) _exit(0); /* just detach then */
+
+    got = ctx->waitpid_instead(child, &status, 0);
+    assert(got == child);
+
+    libxl_report_child_exitstatus(ctx, what, child, status);
+    _exit(WIFEXITED(status) ? WEXITSTATUS(status) :
+          WIFSIGNALED(status) && WTERMSIG(status)<127
+          ? WTERMSIG(status)+128 : -1);
+}
+
+static void report_spawn_intermediate_status(struct libxl_ctx *ctx,
+                                 struct libxl_spawn_starting *for_spawn,
+                                 int status) {
+    if (!WIFEXITED(status)) {
+        /* intermediate process did the logging itself if it exited */
+        char *intermediate_what+            libxl_sprintf(ctx,
+                          "%s intermediate process (startup
monitor)",
+                          for_spawn->what);
+        libxl_report_child_exitstatus(ctx, intermediate_what,
+                                      for_spawn->intermediate, status);
+    }
+}
+
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn) {
+    int r, status;
+    pid_t got;
+    int rc = 0;
+
+    if (!for_spawn) return 0;
+    
+    if (for_spawn->intermediate) {
+        r = kill(for_spawn->intermediate, SIGKILL);
+        if (r) {
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
+                         "could not kill %s intermediate process
[%ld]",
+                         for_spawn->what,
+                         (unsigned long)for_spawn->intermediate);
+            abort(); /* things are very wrong */
+        }
+        got = ctx->waitpid_instead(for_spawn->intermediate, &status,
0);
+        assert(got == for_spawn->intermediate);
+        if (!(WIFSIGNALED(status) && WTERMSIG(status)==SIGKILL)) {
+            report_spawn_intermediate_status(ctx, for_spawn, status);
+            rc = ERROR_FAIL;
+        }
+        for_spawn->intermediate = 0;
+    }
+
+    free(for_spawn->what);
+    for_spawn->what = 0;
+    
+    return rc;
+}
+
+int libxl_spawn_check(struct libxl_ctx *ctx, void *for_spawn_void) {
+    struct libxl_spawn_starting *for_spawn = for_spawn_void;
+    pid_t got;
+    int status;
+
+    if (!for_spawn) return 0;
+    
+    assert(for_spawn->intermediate);
+    got = ctx->waitpid_instead(for_spawn->intermediate, &status,
WNOHANG);
+    if (!got) return 0;
+
+    assert(got == for_spawn->intermediate);
+    report_spawn_intermediate_status(ctx, for_spawn, status);
+    
+    for_spawn->intermediate= 0;
+    return ERROR_FAIL;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 046e51d..957bd9e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -135,7 +135,11 @@ int libxl_device_generic_add(struct libxl_ctx *ctx,
libxl_device *device,
                              char **bents, char **fents);
 int libxl_device_destroy(struct libxl_ctx *ctx, char *be_path, int force);
 int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t domid, int force);
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state);
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata);
 int libxl_wait_for_backend(struct libxl_ctx *ctx, char *be_path, char *state);
 int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned int domain, unsigned
int bus,
                          unsigned int dev, unsigned int func);
@@ -146,8 +150,47 @@ int hvm_build_set_params(int handle, uint32_t domid,
                          int vcpus, int store_evtchn, unsigned long
*store_mfn);
 
 /* xl_exec */
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args);
+
+ /* higher-level double-fork and separate detach eg as for device models */
+
+struct libxl_spawn_starting {
+    /* put this in your own stateu structure as returned to application */
+    /* all fields are private to libxl_spawn_... */
+    pid_t intermediate;
+    char *what; /* malloc''d in spawn_spawn */
+};
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild));
+  /* Logs errors.  A copy of "what" is taken.  Return values:
+   *  < 0   error, for_spawn need not be detached
+   *   +1   caller is now the inner child, should probably call libxl_exec
+   *    0   caller is the parent, must call detach on *for_spawn eventually
+   * Caller, may pass 0 for for_spawn, in which case no need to detach.
+   */
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn);
+  /* Logs errors.  Idempotent, but only permitted after successful
+   * call to libxl_spawn_spawn, and no point calling it again if it fails. */
+int libxl_spawn_check(struct libxl_ctx *ctx,
+                      void *for_spawn);
+  /* Logs errors but also returns them.
+   * for_spawn must actually be a  struct libxl_spawn_starting*  but
+   * we take void* so you can pass this function directly to
+   * libxl_wait_for_device_model.  Caller must still call detach. */
+
+ /* low-level stuff, for synchronous subprocesses etc. */
+
+pid_t libxl_fork(struct libxl_ctx *ctx); // logs errors
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args); // logs errors, never returns
+void libxl_log_child_exitstatus(struct libxl_ctx *ctx,
+                                const char *what, pid_t pid, int status);
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags);
 
 #endif
 
diff --git a/tools/libxl/osdeps.c b/tools/libxl/osdeps.c
index ad96480..b146a9d 100644
--- a/tools/libxl/osdeps.c
+++ b/tools/libxl/osdeps.c
@@ -13,6 +13,8 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <unistd.h>
 #include <stdarg.h>
 #include <stdio.h>
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 727fe4a..6d43f8b 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -699,6 +699,15 @@ skip_pci:
     config_destroy(&config);
 }
 
+#define MUST( call ) ({                                                 \
+        int must_rc = (call);                                           \
+        if (must_rc) {                                                  \
+            fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n",    
\
+                    __FILE__,__LINE__, must_rc, #call);                 \
+            exit(-must_rc);                                             \
+        }                                                               \
+    })
+
 static void create_domain(int debug, const char *filename)
 {
     struct libxl_ctx ctx;
@@ -715,6 +724,7 @@ static void create_domain(int debug, const char *filename)
     libxl_device_console console;
     int num_disks = 0, num_vifs = 0, num_pcidevs = 0, num_vfbs = 0, num_vkbs =
0;
     int i;
+    libxl_device_model_starting *dm_starting = 0;
 
     printf("Parsing config file %s\n", filename);
     parse_config_file(filename, &info1, &info2, &disks,
&num_disks, &vifs, &num_vifs, &pcidevs, &num_pcidevs,
&vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
@@ -736,7 +746,8 @@ static void create_domain(int debug, const char *filename)
     }
     if (info1.hvm) {
         device_model_info_domid_fixup(&dm_info, domid);
-        libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs);
+        MUST( libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs,
+                                        &dm_starting) );
     } else {
         for (i = 0; i < num_vfbs; i++) {
             vfb_info_domid_fixup(vfbs + i, domid);
@@ -750,10 +761,14 @@ static void create_domain(int debug, const char *filename)
             console.constype = CONSTYPE_IOEMU;
         libxl_device_console_add(&ctx, domid, &console);
         if (num_vfbs)
-            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console);
+            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console,
&dm_starting);
     }
+
     for (i = 0; i < num_pcidevs; i++)
         libxl_device_pci_add(&ctx, domid, &pcidevs[i]);
+    if (dm_starting)
+        MUST( libxl_confirm_device_model_startup(&ctx, dm_starting) );
+    
     libxl_domain_unpause(&ctx, domid);
 
 }

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

I wrote:
> commit 6704f959a0ea543f6bbf07e0a58e8b84cdef2236
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Wed Nov 18 15:24:17 2009 +0000
But apparently I was having a bad day yesterday.  I generated that
patch without first committing all of my changes :-(.

Here is the real version.


commit eb5d75cb1157ccdedf0c292e647b0a26790a164c
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Wed Nov 18 15:24:17 2009 +0000

libxenlight: check for early failures of qemu-dm (v4)

This patch makes xl create check whether qemu-dm has started
correctly, and causes it to fail immediately with appropriate errors
if not.  There are other bugfixes too.

More specifically:

 * libxl_create_device_model forks twice rather than once so that the
   process which calls libxl does not end up being the actual parent
   of qemu.  That avoids the need for the qemu-dm process to be reaped
   at some indefinite time in the future.

 * The first fork generates an intermediate process which is
   responsible for writing the qemu-dm pid to xenstore and then merely
   waits to collect and report on qemu-dm''s exit status during
   startup.  New arguments to libxl_create_device_model allow the
   preservation of its pid so that a later call can check whether the
   startup is successful.

 * The core of this functionality (the double fork, waitpid, signal
   handling and so forth) is abstracted away into a new facility
   libxl_spawn_... in libxl_exec.c.

Consequential changes:

 * libxl_wait_for_device_model now takes a callback function parameter
   which is called repeatedly in the loop iteration and allows the
   caller to abort the wait.

 * libxl_exec no longer calls fork; there is a new libxl_fork.

 * There is a hook to override waitpid, which will be necessary for
   some callers.

Remaining problems and other issues I noticed or we found:

 * The error handling is rather inconsistent still and lacking in
   places.
 * destroy_device_model can kill random dom0 processes (!)

Changes since v3 of this patch:
 * Fixed bug which prevented qemu-dm from being properly killed on domain
   shutdown.  (Failure to set dom_path in dm starting state.)
 * Slightly tidy some memory allocation and error handling in
   dm_xenstore_record_pid.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>


diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 421628c..107a8ee 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -23,10 +23,12 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <sys/select.h>
+#include <sys/wait.h>
 #include <signal.h>
 #include <unistd.h> /* for write, unlink and close */
 #include <stdint.h>
 #include <inttypes.h>
+#include <assert.h>
 
 #include "libxl.h"
 #include "libxl_utils.h"
@@ -43,6 +45,8 @@ int libxl_ctx_init(struct libxl_ctx *ctx)
 
     ctx->xch = xc_interface_open();
     ctx->xsh = xs_daemon_open();
+
+    ctx->waitpid_instead= libxl_waitpid_instead_default;
     return 0;
 }
 
@@ -520,25 +524,50 @@ static char ** libxl_build_device_model_args(struct
libxl_ctx *ctx,
     return (char **) flexarray_contents(dm_args);
 }
 
+struct libxl_device_model_starting {
+    struct libxl_spawn_starting for_spawn; /* first! */
+    char *dom_path; /* from libxl_malloc, only for dm_xenstore_record_pid */
+    int domid;
+};
+
+void dm_xenstore_record_pid(struct libxl_ctx *ctx, void *for_spawn,
+                            pid_t innerchild) {
+    struct libxl_device_model_starting *starting = for_spawn;
+    struct libxl_ctx clone;
+    char *kvs[3];
+    int rc;
+    
+    clone = *ctx;
+    clone.xsh = xs_daemon_open();
+    /* we mustn''t use the parent''s handle in the child */
+
+    kvs[0] = "image/device-model-pid";
+    kvs[1] = libxl_sprintf(ctx, "%d", innerchild);
+    kvs[2] = NULL;
+    rc = libxl_xs_writev(ctx, XBT_NULL, starting->dom_path, kvs);
+    if (rc) XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
+                         "Couldn''t record device model pid %ld at
%s/%s",
+                         (unsigned long)innerchild, starting->dom_path,
kvs);
+}
+
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs)
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r)
 {
-    char *dom_path, *path, *logfile, *logfile_new;
-    char *kvs[3];
+    char *path, *logfile, *logfile_new;
     struct stat stat_buf;
-    int logfile_w, null, pid;
-    int i;
+    int logfile_w, null;
+    int i, rc;
     char **args;
+    struct libxl_spawn_starting buf_spawn, *for_spawn;
+
+    *starting_r= 0;
 
     args = libxl_build_device_model_args(ctx, info, vifs, num_vifs);
     if (!args)
         return ERROR_FAIL;
 
-    dom_path = libxl_xs_get_dompath(ctx, info->domid);
-    if (!dom_path)
-        return ERROR_FAIL;
-
     path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d",
info->domid);
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
@@ -559,18 +588,54 @@ int libxl_create_device_model(struct libxl_ctx *ctx,
     logfile = libxl_sprintf(ctx, "/var/log/xen/qemu-dm-%s.log",
info->dom_name);
     logfile_w = open(logfile, O_WRONLY|O_CREAT, 0644);
     null = open("/dev/null", O_RDONLY);
-    pid = libxl_exec(ctx, null, logfile_w, logfile_w, info->device_model,
args);
+
+    if (starting_r) {
+        *starting_r= libxl_calloc(ctx, sizeof(**starting_r), 1);
+        if (!*starting_r) return ERROR_NOMEM;
+        (*starting_r)->domid= info->domid;
+
+        (*starting_r)->dom_path = libxl_xs_get_dompath(ctx, info->domid);
+        if (!(*starting_r)->dom_path) { free(*starting_r); return
ERROR_FAIL; }
+
+        for_spawn= &(*starting_r)->for_spawn;
+    } else {
+        for_spawn= &buf_spawn;
+    }
+    rc = libxl_spawn_spawn(ctx, for_spawn, "device model",
+                           dm_xenstore_record_pid);
+    if (rc < 0) goto xit;
+    if (!rc) { /* inner child */
+        libxl_exec(ctx, null, logfile_w, logfile_w,
+                   info->device_model, args);
+    }
+
+    rc = 0;
+ xit:
     close(null);
     close(logfile_w);
 
-    kvs[0] = libxl_sprintf(ctx, "image/device-model-pid");
-    kvs[1] = libxl_sprintf(ctx, "%d", pid);
-    kvs[2] = NULL;
-    libxl_xs_writev(ctx, XBT_NULL, dom_path, kvs);
+    return rc;
+}
 
-    return 0;
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting) {
+    int rc;
+    rc = libxl_spawn_detach(ctx, &starting->for_spawn);
+    libxl_free(ctx, starting);
+    return rc;
 }
 
+
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                                       libxl_device_model_starting *starting) {
+    int problem = libxl_wait_for_device_model(ctx, starting->domid,
"running",
+                                              libxl_spawn_check,
+                                              &starting->for_spawn);
+    int detach = libxl_detach_device_model(ctx, starting);
+    return problem ? problem : detach;
+}
+
+
 /******************************************************************************/
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
@@ -917,12 +982,13 @@ static int libxl_build_xenpv_qemu_args(struct libxl_ctx
*ctx,
 }
 
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console)
+                            int num_console, libxl_device_console *console,
+                            struct libxl_device_model_starting **starting_r)
 {
     libxl_device_model_info info;
 
     libxl_build_xenpv_qemu_args(ctx, vfb, num_console, console, &info);
-    libxl_create_device_model(ctx, &info, NULL, 0);
+    libxl_create_device_model(ctx, &info, NULL, 0, starting_r);
     return 0;
 }
 
@@ -1195,7 +1261,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1209,7 +1275,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t
domid, libxl_device_pci
                            pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-ins",
strlen("pci-ins"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted")
< 0)
+        if (libxl_wait_for_device_model(ctx, domid, "pci-inserted",
0,0) < 0)
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/parameter", domid);
         vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
@@ -1283,7 +1349,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {
-        if (libxl_wait_for_device_model(ctx, domid, "running") <
0) {
+        if (libxl_wait_for_device_model(ctx, domid, "running", 0,0)
< 0) {
             return -1;
         }
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
@@ -1293,7 +1359,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx,
uint32_t domid, libxl_device_
                        pcidev->bus, pcidev->dev, pcidev->func);
         snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed")
< 0) {
+        if (libxl_wait_for_device_model(ctx, domid, "pci-removed",
0,0) < 0) {
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
             return -1;
         }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ac4c79e..29ffde2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -41,6 +41,11 @@ struct libxl_ctx {
     /* mini-GC */
     int alloc_maxsize;
     void **alloc_ptrs;
+
+    /* for callers who reap children willy-nilly; caller must only
+     * set this after libxl_init and before any other call - or
+     * may leave them untouched */
+    int (*waitpid_instead)(pid_t pid, int *status, int flags);
 };
 
 typedef struct {
@@ -235,11 +240,22 @@ int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t
domid);
 struct libxl_dominfo * libxl_domain_list(struct libxl_ctx *ctx, int
*nb_domain);
 xc_dominfo_t * libxl_domain_infolist(struct libxl_ctx *ctx, int *nb_domain);
 
+typedef struct libxl_device_model_starting libxl_device_model_starting;
 int libxl_create_device_model(struct libxl_ctx *ctx,
                               libxl_device_model_info *info,
-                              libxl_device_nic *vifs, int num_vifs);
+                              libxl_device_nic *vifs, int num_vifs,
+                              libxl_device_model_starting **starting_r);
 int libxl_create_xenpv_qemu(struct libxl_ctx *ctx, libxl_device_vfb *vfb,
-                            int num_console, libxl_device_console *console);
+                            int num_console, libxl_device_console *console,
+                            libxl_device_model_starting **starting_r);
+  /* Caller must either: pass starting_r==0, or on successful
+   * return pass *starting_r (which will be non-0) to
+   * libxl_confirm_device_model or libxl_detach_device_model. */
+int libxl_confirm_device_model_startup(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+int libxl_detach_device_model(struct libxl_ctx *ctx,
+                              libxl_device_model_starting *starting);
+  /* DM is detached even if error is returned */
 
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk);
 int libxl_device_disk_clean_shutdown(struct libxl_ctx *ctx, uint32_t domid);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 451233f..39932a6 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -287,12 +287,17 @@ int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned
int domain, unsigned in
     return -1;
 }
 
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state)
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata)
 {
     char path[50];
     char *p;
     int watchdog = 100;
     unsigned int len;
+    int rc;
 
     snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/state", domid);
     while (watchdog > 0) {
@@ -310,6 +315,10 @@ int libxl_wait_for_device_model(struct libxl_ctx *ctx,
uint32_t domid, char *sta
                 watchdog--;
             }
         }
+        if (check_callback) {
+            rc = check_callback(ctx, check_callback_userdata);
+            if (rc) return rc;
+        }
     }
     XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready");
     return -1;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 5186ac8..6a3373e 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -18,34 +18,191 @@
 #include "libxl_osdeps.h"
 
 #include <stdio.h>
+#include <string.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <unistd.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include "libxl.h"
 #include "libxl_internal.h"
 
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args)
+pid_t libxl_fork(struct libxl_ctx *ctx)
 {
-    int pid, i;
+    pid_t pid;
 
     pid = fork();
     if (pid == -1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "fork failed");
         return -1;
     }
-    if (pid == 0) {
-        /* child */
-        if (stdinfd != -1)
-            dup2(stdinfd, STDIN_FILENO);
-        if (stdoutfd != -1)
-            dup2(stdoutfd, STDOUT_FILENO);
-        if (stderrfd != -1)
-            dup2(stderrfd, STDERR_FILENO);
-        for (i = 4; i < 256; i++)
-            close(i);
-        execv(arg0, args);
-        exit(256);
-    }
+    
     return pid;
 }
+
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args)
+     /* call this in the child */
+{
+    int i;
+
+    if (stdinfd != -1)
+        dup2(stdinfd, STDIN_FILENO);
+    if (stdoutfd != -1)
+        dup2(stdoutfd, STDOUT_FILENO);
+    if (stderrfd != -1)
+        dup2(stderrfd, STDERR_FILENO);
+    for (i = 4; i < 256; i++)
+        close(i);
+    execv(arg0, args);
+    XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "exec %s failed", arg0);
+    _exit(-1);
+}
+
+void libxl_report_child_exitstatus(struct libxl_ctx *ctx,
+                                   const char *what, pid_t pid, int status) {
+    /* treats all exit statuses as errors; if that''s not what you
want,
+     * check status yourself first */
+    
+    if (WIFEXITED(status)) {
+        int st= WEXITSTATUS(status);
+        if (st)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] exited"
+                   " with error status %d", what, (unsigned long)pid,
st);
+        else
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] unexpectedly"
+                   " exited status zero", what, (unsigned long)pid);
+    } else if (WIFSIGNALED(status)) {
+        int sig= WTERMSIG(status);
+        const char *str= strsignal(sig);
+        const char *coredump= WCOREDUMP(status) ? " (core dumped)" :
"";
+        if (str)
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to"
+                   " fatal signal %s%s", what, (unsigned long)pid,
+                   str, coredump);
+        else 
+            XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] died due to unknown"
+                   " fatal signal number %d%s", what, (unsigned
long)pid,
+                   sig, coredump);
+    } else {
+        XL_LOG(ctx, XL_LOG_ERROR, "%s [%ld] gave unknown"
+               " wait status 0x%x", what, (unsigned long)pid,
status);
+    }
+}
+
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags) {
+    return waitpid(pid,status,flags);
+}
+
+
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild)) {
+    pid_t child, got;
+    int status;
+    pid_t intermediate;
+
+    if (for_spawn) {
+        for_spawn->what= strdup(what);
+        if (!for_spawn->what) return ERROR_NOMEM;
+    }
+
+    intermediate = libxl_fork(ctx);
+    if (intermediate==-1) {
+        if (for_spawn) free(for_spawn->what);
+        return ERROR_FAIL;
+    }
+    if (intermediate) {
+        /* parent */
+        if (for_spawn) for_spawn->intermediate= intermediate;
+        return 1;
+    }
+    
+    /* we are now the intermediate process */
+
+    child = libxl_fork(ctx);
+    if (!child) return 0; /* caller runs child code */
+    if (child<0) exit(255);
+
+    intermediate_hook(ctx, for_spawn, child);
+
+    if (!for_spawn) _exit(0); /* just detach then */
+
+    got = ctx->waitpid_instead(child, &status, 0);
+    assert(got == child);
+
+    libxl_report_child_exitstatus(ctx, what, child, status);
+    _exit(WIFEXITED(status) ? WEXITSTATUS(status) :
+          WIFSIGNALED(status) && WTERMSIG(status)<127
+          ? WTERMSIG(status)+128 : -1);
+}
+
+static void report_spawn_intermediate_status(struct libxl_ctx *ctx,
+                                 struct libxl_spawn_starting *for_spawn,
+                                 int status) {
+    if (!WIFEXITED(status)) {
+        /* intermediate process did the logging itself if it exited */
+        char *intermediate_what+            libxl_sprintf(ctx,
+                          "%s intermediate process (startup
monitor)",
+                          for_spawn->what);
+        libxl_report_child_exitstatus(ctx, intermediate_what,
+                                      for_spawn->intermediate, status);
+    }
+}
+
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn) {
+    int r, status;
+    pid_t got;
+    int rc = 0;
+
+    if (!for_spawn) return 0;
+    
+    if (for_spawn->intermediate) {
+        r = kill(for_spawn->intermediate, SIGKILL);
+        if (r) {
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
+                         "could not kill %s intermediate process
[%ld]",
+                         for_spawn->what,
+                         (unsigned long)for_spawn->intermediate);
+            abort(); /* things are very wrong */
+        }
+        got = ctx->waitpid_instead(for_spawn->intermediate, &status,
0);
+        assert(got == for_spawn->intermediate);
+        if (!(WIFSIGNALED(status) && WTERMSIG(status)==SIGKILL)) {
+            report_spawn_intermediate_status(ctx, for_spawn, status);
+            rc = ERROR_FAIL;
+        }
+        for_spawn->intermediate = 0;
+    }
+
+    free(for_spawn->what);
+    for_spawn->what = 0;
+    
+    return rc;
+}
+
+int libxl_spawn_check(struct libxl_ctx *ctx, void *for_spawn_void) {
+    struct libxl_spawn_starting *for_spawn = for_spawn_void;
+    pid_t got;
+    int status;
+
+    if (!for_spawn) return 0;
+    
+    assert(for_spawn->intermediate);
+    got = ctx->waitpid_instead(for_spawn->intermediate, &status,
WNOHANG);
+    if (!got) return 0;
+
+    assert(got == for_spawn->intermediate);
+    report_spawn_intermediate_status(ctx, for_spawn, status);
+    
+    for_spawn->intermediate= 0;
+    return ERROR_FAIL;
+}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 046e51d..957bd9e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -135,7 +135,11 @@ int libxl_device_generic_add(struct libxl_ctx *ctx,
libxl_device *device,
                              char **bents, char **fents);
 int libxl_device_destroy(struct libxl_ctx *ctx, char *be_path, int force);
 int libxl_devices_destroy(struct libxl_ctx *ctx, uint32_t domid, int force);
-int libxl_wait_for_device_model(struct libxl_ctx *ctx, uint32_t domid, char
*state);
+int libxl_wait_for_device_model(struct libxl_ctx *ctx,
+                                uint32_t domid, char *state,
+                                int (*check_callback)(struct libxl_ctx *ctx,
+                                                      void *userdata),
+                                void *check_callback_userdata);
 int libxl_wait_for_backend(struct libxl_ctx *ctx, char *be_path, char *state);
 int libxl_device_pci_flr(struct libxl_ctx *ctx, unsigned int domain, unsigned
int bus,
                          unsigned int dev, unsigned int func);
@@ -146,8 +150,47 @@ int hvm_build_set_params(int handle, uint32_t domid,
                          int vcpus, int store_evtchn, unsigned long
*store_mfn);
 
 /* xl_exec */
-int libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
-               char *arg0, char **args);
+
+ /* higher-level double-fork and separate detach eg as for device models */
+
+struct libxl_spawn_starting {
+    /* put this in your own stateu structure as returned to application */
+    /* all fields are private to libxl_spawn_... */
+    pid_t intermediate;
+    char *what; /* malloc''d in spawn_spawn */
+};
+
+int libxl_spawn_spawn(struct libxl_ctx *ctx,
+                      struct libxl_spawn_starting *for_spawn,
+                      const char *what,
+                      void (*intermediate_hook)(struct libxl_ctx *ctx,
+                                                void *for_spawn,
+                                                pid_t innerchild));
+  /* Logs errors.  A copy of "what" is taken.  Return values:
+   *  < 0   error, for_spawn need not be detached
+   *   +1   caller is now the inner child, should probably call libxl_exec
+   *    0   caller is the parent, must call detach on *for_spawn eventually
+   * Caller, may pass 0 for for_spawn, in which case no need to detach.
+   */
+int libxl_spawn_detach(struct libxl_ctx *ctx,
+                       struct libxl_spawn_starting *for_spawn);
+  /* Logs errors.  Idempotent, but only permitted after successful
+   * call to libxl_spawn_spawn, and no point calling it again if it fails. */
+int libxl_spawn_check(struct libxl_ctx *ctx,
+                      void *for_spawn);
+  /* Logs errors but also returns them.
+   * for_spawn must actually be a  struct libxl_spawn_starting*  but
+   * we take void* so you can pass this function directly to
+   * libxl_wait_for_device_model.  Caller must still call detach. */
+
+ /* low-level stuff, for synchronous subprocesses etc. */
+
+pid_t libxl_fork(struct libxl_ctx *ctx); // logs errors
+void libxl_exec(struct libxl_ctx *ctx, int stdinfd, int stdoutfd, int stderrfd,
+                char *arg0, char **args); // logs errors, never returns
+void libxl_log_child_exitstatus(struct libxl_ctx *ctx,
+                                const char *what, pid_t pid, int status);
+pid_t libxl_waitpid_instead_default(pid_t pid, int *status, int flags);
 
 #endif
 
diff --git a/tools/libxl/osdeps.c b/tools/libxl/osdeps.c
index ad96480..b146a9d 100644
--- a/tools/libxl/osdeps.c
+++ b/tools/libxl/osdeps.c
@@ -13,6 +13,8 @@
  * GNU Lesser General Public License for more details.
  */
 
+#include "libxl_osdeps.h"
+
 #include <unistd.h>
 #include <stdarg.h>
 #include <stdio.h>
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 727fe4a..6d43f8b 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -699,6 +699,15 @@ skip_pci:
     config_destroy(&config);
 }
 
+#define MUST( call ) ({                                                 \
+        int must_rc = (call);                                           \
+        if (must_rc) {                                                  \
+            fprintf(stderr,"xl: fatal error: %s:%d, rc=%d: %s\n",    
\
+                    __FILE__,__LINE__, must_rc, #call);                 \
+            exit(-must_rc);                                             \
+        }                                                               \
+    })
+
 static void create_domain(int debug, const char *filename)
 {
     struct libxl_ctx ctx;
@@ -715,6 +724,7 @@ static void create_domain(int debug, const char *filename)
     libxl_device_console console;
     int num_disks = 0, num_vifs = 0, num_pcidevs = 0, num_vfbs = 0, num_vkbs =
0;
     int i;
+    libxl_device_model_starting *dm_starting = 0;
 
     printf("Parsing config file %s\n", filename);
     parse_config_file(filename, &info1, &info2, &disks,
&num_disks, &vifs, &num_vifs, &pcidevs, &num_pcidevs,
&vfbs, &num_vfbs, &vkbs, &num_vkbs, &dm_info);
@@ -736,7 +746,8 @@ static void create_domain(int debug, const char *filename)
     }
     if (info1.hvm) {
         device_model_info_domid_fixup(&dm_info, domid);
-        libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs);
+        MUST( libxl_create_device_model(&ctx, &dm_info, vifs, num_vifs,
+                                        &dm_starting) );
     } else {
         for (i = 0; i < num_vfbs; i++) {
             vfb_info_domid_fixup(vfbs + i, domid);
@@ -750,10 +761,14 @@ static void create_domain(int debug, const char *filename)
             console.constype = CONSTYPE_IOEMU;
         libxl_device_console_add(&ctx, domid, &console);
         if (num_vfbs)
-            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console);
+            libxl_create_xenpv_qemu(&ctx, vfbs, 1, &console,
&dm_starting);
     }
+
     for (i = 0; i < num_pcidevs; i++)
         libxl_device_pci_add(&ctx, domid, &pcidevs[i]);
+    if (dm_starting)
+        MUST( libxl_confirm_device_model_startup(&ctx, dm_starting) );
+    
     libxl_domain_unpause(&ctx, domid);
 
 }

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