thr3ads.net - Xen devel - [PATCH v5 00/19] libxl: improvements, prep for subprocess handling [Apr 2012]

If this information is useful, please help other people find it:
Share via:

Ian Jackson

2012-Apr-11 12:59 UTC

[PATCH v5 00/19] libxl: improvements, prep for subprocess handling

This is the initial portion of my child process series which has been
acked and which I intend to apply right away.  Changes are exactly
those discussed on the list since v4; I''m reposting the final version
for form''s sake.

Bugfixes for problems reported by Roger Pau Monne:
 02/19 libxl: ao: allow immediate completion
 03/19 libxl: fix hang due to libxl__initiate_device_remove
 04/19 libxl: Fix eventloop_iteration over-locking
 05/19 libxl: remove poller from list in libxl__poller_get

Other general bugfixes:
 01/19 .gitignore: Add a missing file
 06/19 libxl: Fix leak of ctx->lock
 07/19 tools: Correct PTHREAD options in config/StdGNU.mk
 08/19 libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS
 09/19 tools: Use PTHREAD_CFLAGS, _LDFLAGS, _LIBS

Clarifications and improvements related to memory allocation:
 10/19 libxl: Crash (more sensibly) on malloc failure
 11/19 libxl: Make libxl__zalloc et al tolerate a NULL gc

Preparatory work for child process handling:
 12/19 libxl: Introduce some convenience macros
 13/19 libxl: include <ctype.h> and introduce CTYPE helper macro
 14/19 libxl: Provide libxl_string_list_length
 15/19 libxl: include <_libxl_paths.h> in libxl_internal.h
 16/19 libxl: abolish libxl_ctx_postfork

Event-related infrastructure and fixes:
 17/19 libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS
 18/19 libxl: Protect fds with CLOEXEC even with forking threads
 19/19 libxl: provide STATE_AO_GC

The remaining patches (20-31 from v4) remain outstanding.

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 01/19] .gitignore: Add a missing file

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 .gitignore |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 2782ee5..cd12b56 100644
--- a/.gitignore
+++ b/.gitignore
@@ -357,6 +357,7 @@ tools/firmware/etherboot/eb-roms.h
 tools/firmware/etherboot/gpxe-git-snapshot.tar.gz
 tools/misc/xenwatchdogd
 tools/misc/xen-hvmcrash
+tools/misc/xen-lowmemd
 tools/libvchan/vchan-node[12]
 tools/ocaml/*/.ocamldep.make
 tools/ocaml/*/*.cm[ixao]
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 02/19] libxl: ao: allow immediate completion

Make it possible to complete an ao during its initating function.

Previously this was not generally possible because initiators did not
have an egc.  But there is no reason why an ao initiator should not
have an egc, so make the standard macros provide one.

Change the internal documentation comments accordingly.  (This change,
which means that an initiator function may call a completion callback
directly, is already consistent with the documented external API.)

We also invent of a new state flag "constructing" which indicates
whether we are between ao__create and ao__inprogress.  This is a
slightly optimisation which allows ao_complete to not bother poking
the wakeup pipe, since the logic in ao__inprogress will not run the
event loop if the ao is complete on entry.

Also fix the wording in the libxl_internal.h comment forbidding use of
ao_how-taking functions from within libxl.  (There are sadly currently
some such functions.)

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@entel.upc.edu>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_event.c    |    7 ++++++-
 tools/libxl/libxl_internal.h |   14 ++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 271949a..c89add8 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1225,7 +1225,9 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao,
int rc)
 
     if (ao->poller) {
         assert(ao->in_initiator);
-        libxl__poller_wakeup(egc, ao->poller);
+        if (!ao->constructing)
+            /* don''t bother with this if we''re not in the
event loop */
+            libxl__poller_wakeup(egc, ao->poller);
     } else if (ao->how.callback) {
         LIBXL_TAILQ_INSERT_TAIL(&egc->aos_for_callback, ao,
entry_for_callback);
     } else {
@@ -1251,6 +1253,7 @@ libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t
domid,
     if (!ao) goto out;
 
     ao->magic = LIBXL__AO_MAGIC;
+    ao->constructing = 1;
     ao->in_initiator = 1;
     ao->poller = 0;
     ao->domid = domid;
@@ -1275,7 +1278,9 @@ int libxl__ao_inprogress(libxl__ao *ao)
     int rc;
 
     assert(ao->magic == LIBXL__AO_MAGIC);
+    assert(ao->constructing);
     assert(ao->in_initiator);
+    ao->constructing = 0;
 
     if (ao->poller) {
         /* Caller wants it done synchronously. */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e0a1070..b1e0588 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -347,7 +347,7 @@ struct libxl__egc {
 
 struct libxl__ao {
     uint32_t magic;
-    unsigned in_initiator:1, complete:1, notified:1;
+    unsigned constructing:1, in_initiator:1, complete:1, notified:1;
     int rc;
     libxl__gc gc;
     libxl_asyncop_how how;
@@ -1209,7 +1209,11 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
  * operation ("ao") machinery.  The function should take a parameter
  * const libxl_asyncop_how *ao_how and must start with a call to
  * AO_INITIATOR_ENTRY.  These functions MAY NOT be called from
- * outside libxl, because they can cause reentrancy callbacks.
+ * inside libxl, because they can cause reentrancy callbacks.
+ *
+ * For the same reason functions taking an ao_how may make themselves
+ * an egc with EGC_INIT (and they will generally want to, to be able
+ * to immediately complete an ao during its setup).
  *
  * Lifecycle of an ao:
  *
@@ -1240,8 +1244,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
  *   directly or indirectly, should call libxl__ao_complete (with the
  *   ctx locked, as it will generally already be in any event callback
  *   function).  This must happen exactly once for each ao (and not if
- *   the ao has been destroyed, obviously), and it may not happen
- *   until libxl__ao_inprogress has been called on the ao.
+ *   the ao has been destroyed, obviously).
  *
  * - Note that during callback functions, two gcs are available:
  *    - The one in egc, whose lifetime is only this callback
@@ -1255,12 +1258,14 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
     libxl__ctx_lock(ctx);                                       \
     libxl__ao *ao = libxl__ao_create(ctx, domid, ao_how);       \
     if (!ao) { libxl__ctx_unlock(ctx); return ERROR_NOMEM; }    \
+    libxl__egc egc[1]; LIBXL_INIT_EGC(egc[0],ctx);              \
     AO_GC;
 
 #define AO_INPROGRESS ({                                        \
         libxl_ctx *ao__ctx = libxl__gc_owner(&ao->gc);          \
         int ao__rc = libxl__ao_inprogress(ao);                  \
         libxl__ctx_unlock(ao__ctx); /* gc is now invalid */     \
+        EGC_FREE;                                               \
         (ao__rc);                                               \
    })
 
@@ -1269,6 +1274,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
         assert(rc);                                             \
         libxl__ao_abort(ao);                                    \
         libxl__ctx_unlock(ao__ctx); /* gc is now invalid */     \
+        EGC_FREE;                                               \
         (rc);                                                   \
     })
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 03/19] libxl: fix hang due to libxl__initiate_device_remove

libxl__initiate_device_remove might discover that the operation was
complete, immediately (typically, if the device is already removed).

Previously, in this situation, it would return 0 to the caller but
never call libxl__ao_complete.  Fix this.  This necessitates passing
the egc in from the functions which are the ao initiators.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@entel.upc.edu>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c          |    8 ++++----
 tools/libxl/libxl_device.c   |   18 ++++++++++++------
 tools/libxl/libxl_internal.h |    3 ++-
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 7a54524..dd948a8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1406,7 +1406,7 @@ int libxl_device_disk_remove(libxl_ctx *ctx, uint32_t
domid,
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(ao, &device);
+    rc = libxl__initiate_device_remove(egc, ao, &device);
     if (rc) goto out;
 
     return AO_INPROGRESS;
@@ -1873,7 +1873,7 @@ int libxl_device_nic_remove(libxl_ctx *ctx, uint32_t
domid,
     rc = libxl__device_from_nic(gc, domid, nic, &device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(ao, &device);
+    rc = libxl__initiate_device_remove(egc, ao, &device);
     if (rc) goto out;
 
     return AO_INPROGRESS;
@@ -2220,7 +2220,7 @@ int libxl_device_vkb_remove(libxl_ctx *ctx, uint32_t
domid,
     rc = libxl__device_from_vkb(gc, domid, vkb, &device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(ao, &device);
+    rc = libxl__initiate_device_remove(egc, ao, &device);
     if (rc) goto out;
 
     return AO_INPROGRESS;
@@ -2353,7 +2353,7 @@ int libxl_device_vfb_remove(libxl_ctx *ctx, uint32_t
domid,
     rc = libxl__device_from_vfb(gc, domid, vfb, &device);
     if (rc != 0) goto out;
 
-    rc = libxl__initiate_device_remove(ao, &device);
+    rc = libxl__initiate_device_remove(egc, ao, &device);
     if (rc) goto out;
 
     return AO_INPROGRESS;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c2880e0..c7e057d 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -376,7 +376,8 @@ static void device_remove_callback(libxl__egc *egc,
libxl__ev_devstate *ds,
     device_remove_cleanup(gc, aorm);
 }
 
-int libxl__initiate_device_remove(libxl__ao *ao, libxl__device *dev)
+int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
+                                  libxl__device *dev)
 {
     AO_GC;
     libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -388,11 +389,11 @@ int libxl__initiate_device_remove(libxl__ao *ao,
libxl__device *dev)
     libxl__ao_device_remove *aorm = 0;
 
     if (!state)
-        goto out;
+        goto out_ok;
     if (atoi(state) != 4) {
         libxl__device_destroy_tapdisk(gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
-        goto out;
+        goto out_ok;
     }
 
 retry_transaction:
@@ -404,7 +405,7 @@ retry_transaction:
             goto retry_transaction;
         else {
             rc = ERROR_FAIL;
-            goto out;
+            goto out_fail;
         }
     }
 
@@ -417,13 +418,18 @@ retry_transaction:
     rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
                                  state_path, XenbusStateClosed,
                                  LIBXL_DESTROY_TIMEOUT * 1000);
-    if (rc) goto out;
+    if (rc) goto out_fail;
 
     return 0;
 
- out:
+ out_fail:
+    assert(rc);
     device_remove_cleanup(gc, aorm);
     return rc;
+
+ out_ok:
+    libxl__ao_complete(egc, ao, 0);
+    return 0;
 }
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b1e0588..5167b71 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -692,7 +692,8 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, char
*be_path, char *state);
  * this is done, the ao will be completed.  An error
  * return from libxl__initiate_device_remove means that the ao
  * will _not_ be completed and the caller must do so. */
-_hidden int libxl__initiate_device_remove(libxl__ao*, libxl__device *dev);
+_hidden int libxl__initiate_device_remove(libxl__egc*, libxl__ao*,
+                                          libxl__device *dev);
 
 /*
  * libxl__ev_devstate - waits a given time for a device to
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 04/19] libxl: Fix eventloop_iteration over-locking

eventloop_iteration''s head comment says that it must be called with
the ctx locked exactly once, and this is indeed true, and it''s done
correctly at both the call sites.

However, it takes out the lock an additional time itself.  This is
wrong because it prevents the unlocks around poll from being
effective.  This would mean that a multithreaded event-loop using
program might suffer from undesired blocking, as one thread trying to
enter libxl might end up stalled by another thread waiting for a slow
event.  So remove those two lock calls.

Also add a couple of comments documenting the locking behaviour of
libxl__ao_inprogress and libxl__egc_cleanup.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_event.c    |    4 ----
 tools/libxl/libxl_internal.h |    4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index c89add8..5ac6334 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1058,8 +1058,6 @@ static int eventloop_iteration(libxl__egc *egc,
libxl__poller *poller) {
     int rc;
     struct timeval now;
     
-    CTX_LOCK;
-
     rc = libxl__gettimeofday(gc, &now);
     if (rc) goto out;
 
@@ -1102,8 +1100,6 @@ static int eventloop_iteration(libxl__egc *egc,
libxl__poller *poller) {
     afterpoll_internal(egc, poller,
                        poller->fd_polls_allocd, poller->fd_polls, now);
 
-    CTX_UNLOCK;
-
     rc = 0;
  out:
     return rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5167b71..04f13f6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1191,7 +1191,7 @@ libxl__device_model_version_running(libxl__gc *gc,
uint32_t domid);
 _hidden void libxl__egc_cleanup(libxl__egc *egc);
   /* Frees memory allocated within this egc''s gc, and and report all
    * occurred events via callback, if applicable.  May reenter the
-   * application; see restrictions above. */
+   * application; see restrictions above.  The ctx must be UNLOCKED. */
 
 /* convenience macros: */
 
@@ -1287,7 +1287,7 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
  * libxl__ao_inprogress MUST be called with the ctx locked exactly once. */
 _hidden libxl__ao *libxl__ao_create(libxl_ctx*, uint32_t domid,
                                     const libxl_asyncop_how*);
-_hidden int libxl__ao_inprogress(libxl__ao *ao);
+_hidden int libxl__ao_inprogress(libxl__ao *ao); /* temporarily unlocks */
 _hidden void libxl__ao_abort(libxl__ao *ao);
 _hidden void libxl__ao_complete(libxl__egc *egc, libxl__ao *ao, int rc);
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 05/19] libxl: remove poller from list in libxl__poller_get

From: Roger Pau Monne <roger.pau@citrix.com>

Remove poller from the list once it has been requested.
Fixes a double-free bug.

Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@citrix.com>
---
 tools/libxl/libxl_event.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 5ac6334..9cb172a 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1010,8 +1010,10 @@ libxl__poller *libxl__poller_get(libxl_ctx *ctx)
     int rc;
 
     libxl__poller *p = LIBXL_LIST_FIRST(&ctx->pollers_idle);
-    if (p)
+    if (p) {
+        LIBXL_LIST_REMOVE(p, entry);
         return p;
+    }
 
     p = malloc(sizeof(*p));
     if (!p) {
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 06/19] libxl: Fix leak of ctx->lock

A mutex created with pthread_mutex_init, like ctx->lock, may need to
be destroyed with pthread_mutex_destroy.

Also, previously, if libxl__init_recursive_mutex failed, the nascent
ctx would be leaked.  Add some comments which will hopefully make
these kind of mistakes less likely in future.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index dd948a8..f41b62f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -39,10 +39,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     memset(ctx, 0, sizeof(libxl_ctx));
     ctx->lg = lg;
 
-    if (libxl__init_recursive_mutex(ctx, &ctx->lock) < 0) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Failed to initialize
mutex");
-        return ERROR_FAIL;
-    }
+    /* First initialise pointers (cannot fail) */
 
     LIBXL_TAILQ_INIT(&ctx->occurred);
 
@@ -61,6 +58,16 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     LIBXL_TAILQ_INIT(&ctx->death_list);
     libxl__ev_xswatch_init(&ctx->death_watch);
 
+    /* The mutex is special because we can''t idempotently destroy it
*/
+
+    if (libxl__init_recursive_mutex(ctx, &ctx->lock) < 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Failed to initialize
mutex");
+        free(ctx);
+        ctx = 0;
+    }
+
+    /* Now ctx is safe for ctx_free; failures simply set rc and "goto
out" */
+
     rc = libxl__poller_init(ctx, &ctx->poller_app);
     if (rc) goto out;
 
@@ -150,6 +157,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
 
     discard_events(&ctx->occurred);
 
+    pthread_mutex_destroy(&ctx->lock);
+
     GC_FREE;
     free(ctx);
     return 0;
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 07/19] tools: Correct PTHREAD options in config/StdGNU.mk

It is not correct to say -lpthread.  The correct option is -pthread,
which may have sundry other effects on code generation etc.  It needs
to be passed both to compilation and linking.

Fix the configure test to test -pthread, and plumb the resulting flag
through to PTHREAD_{CFLAGS,LDFLAGS} in Tools.mk; also substitute
PTHREAD_LIBS (although this will currently always be empty).
Remove PTHREAD_LIBS setting from StdGNU.mk.

Fix the one user (libxc) to use PTHREAD_{CFLAGS,LDFLAGS} too.

There are still some other users in tree which pass -pthread or
-lpthread by adding it as a literal to their own compiler options.
These will be fixed in a later patch.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@entel.upc.edu>
Acked-by: Roger Pau Monne <roger.pau@entel.upc.edu>
---
 config/StdGNU.mk     |    1 -
 config/Tools.mk.in   |    4 ++
 tools/configure      |  101 ++++++++++++++++++++++++++++++++++++--------------
 tools/configure.ac   |    5 +-
 tools/libxc/Makefile |    4 +-
 tools/m4/pthread.m4  |   41 ++++++++++++++++++++
 tools/m4/savevar.m4  |    6 +++
 7 files changed, 130 insertions(+), 32 deletions(-)
 create mode 100644 tools/m4/pthread.m4
 create mode 100644 tools/m4/savevar.m4

diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index e2c9335..e2f2e1e 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -67,7 +67,6 @@ XEN_CONFIG_DIR = $(CONFIG_DIR)/xen
 XEN_SCRIPT_DIR = $(XEN_CONFIG_DIR)/scripts
 
 SOCKET_LIBS -PTHREAD_LIBS = -lpthread
 UTIL_LIBS = -lutil
 DLOPEN_LIBS = -ldl
 
diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 339a7b6..912d021 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -26,6 +26,10 @@ PREPEND_LIB         := @PREPEND_LIB@
 APPEND_INCLUDES     := @APPEND_INCLUDES@
 APPEND_LIB          := @APPEND_LIB@
 
+PTHREAD_CFLAGS      := @PTHREAD_CFLAGS@
+PTHREAD_LDFLAGS     := @PTHREAD_LDFLAGS@
+PTHREAD_LIBS        := @PTHREAD_LIBS@
+
 # Download GIT repositories via HTTP or GIT''s own protocol?
 # GIT''s protocol is faster and more robust, when it works at all
(firewalls
 # may block it). We make it the default, but if your GIT repository downloads
diff --git a/tools/configure b/tools/configure
index 64b7eb5..86618f5 100755
--- a/tools/configure
+++ b/tools/configure
@@ -602,6 +602,9 @@ POW_LIB
 LIBOBJS
 ALLOCA
 libiconv
+PTHREAD_LIBS
+PTHREAD_LDFLAGS
+PTHREAD_CFLAGS
 libgcrypt
 libext2fs
 system_aio
@@ -3861,6 +3864,9 @@ case $host_os in *\ *) host_os=`echo "$host_os"
| sed ''s/ /-/g''`;; esac
 
 
 
+
+
+
 # pkg.m4 - Macros to locate and utilise pkg-config.            -*- Autoconf -*-
 # serial 1 (pkg-config-0.24)
 #
@@ -3924,6 +3930,22 @@ case $host_os in *\ *) host_os=`echo "$host_os"
| sed ''s/ /-/g''`;; esac
 
 
 
+# We define, separately, PTHREAD_CFLAGS, _LDFLAGS and _LIBS
+# even though currently we don''t set them very separately.
+# This means that the makefiles will not need to change in
+# the future if we make the test more sophisticated.
+
+
+
+# We invoke AX_PTHREAD_VARS with the name of another macro
+# which is then expanded once for each variable.
+
+
+
+
+
+
+
 
 # Enable/disable options
 
@@ -7228,47 +7250,70 @@ else
 fi
 
 
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for pthread_create in
-lpthread" >&5
-$as_echo_n "checking for pthread_create in -lpthread... " >&6;
}
-if test "${ac_cv_lib_pthread_pthread_create+set}" = set; then :
+
+    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for pthread
flag" >&5
+$as_echo_n "checking for pthread flag... " >&6; }
+if test "${ax_cv_pthread_flags+set}" = set; then :
   $as_echo_n "(cached) " >&6
 else
-  ac_check_lib_save_LIBS=$LIBS
-LIBS="-lpthread  $LIBS"
-cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+
+        ax_cv_pthread_flags=-pthread
+
+    PTHREAD_CFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LDFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LIBS=""
+
+
+    saved_CFLAGS="$CFLAGS"
+
+    saved_LDFLAGS="$LDFLAGS"
+
+    saved_LIBS="$LIBS"
+
+
+    CFLAGS="$CFLAGS $PTHREAD_CFLAGS"
+
+    LDFLAGS="$LDFLAGS $PTHREAD_LDFLAGS"
+
+    LIBS="$LIBS $PTHREAD_LIBS"
+
+        cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 
-/* Override any GCC internal prototype to avoid an error.
-   Use char because int might match the return type of a GCC
-   builtin and then its argument prototype would still apply.  */
-#ifdef __cplusplus
-extern "C"
-#endif
-char pthread_create ();
-int
-main ()
-{
-return pthread_create ();
-  ;
-  return 0;
+#include <pthread.h>
+int main(void) {
+  pthread_atfork(0,0,0);
+  pthread_create(0,0,0,0);
 }
+
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_pthread_pthread_create=yes
+
 else
-  ac_cv_lib_pthread_pthread_create=no
+  ax_cv_pthread_flags=failed
 fi
 rm -f core conftest.err conftest.$ac_objext \
     conftest$ac_exeext conftest.$ac_ext
-LIBS=$ac_check_lib_save_LIBS
-fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$ac_cv_lib_pthread_pthread_create" >&5
-$as_echo "$ac_cv_lib_pthread_pthread_create" >&6; }
-if test "x$ac_cv_lib_pthread_pthread_create" = x""yes; then
:
 
-else
-  as_fn_error $? "Could not find libpthread" "$LINENO" 5
+    CFLAGS="$saved_CFLAGS"
+
+    LDFLAGS="$saved_LDFLAGS"
+
+    LIBS="$saved_LIBS"
+
+
 fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$ax_cv_pthread_flags" >&5
+$as_echo "$ax_cv_pthread_flags" >&6; }
+    if test "x$ax_cv_pthread_flags" = xfailed; then
+        as_fn_error $? "-pthread does not work" "$LINENO" 5
+    fi
+
+    PTHREAD_CFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LDFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LIBS=""
+
+
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for clock_gettime in
-lrt" >&5
 $as_echo_n "checking for clock_gettime in -lrt... " >&6; }
diff --git a/tools/configure.ac b/tools/configure.ac
index 0204e36..250dffd 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -23,6 +23,7 @@ AC_USE_SYSTEM_EXTENSIONS
 AC_CANONICAL_HOST
 
 # M4 Macro includes
+m4_include([m4/savevar.m4])
 m4_include([m4/features.m4])
 m4_include([m4/path_or_fail.m4])
 m4_include([m4/python_version.m4])
@@ -33,6 +34,7 @@ m4_include([m4/set_cflags_ldflags.m4])
 m4_include([m4/uuid.m4])
 m4_include([m4/pkg.m4])
 m4_include([m4/curses.m4])
+m4_include([m4/pthread.m4])
 
 # Enable/disable options
 AX_ARG_DEFAULT_DISABLE([githttp], [Download GIT repositories via HTTP])
@@ -129,8 +131,7 @@ AC_CHECK_LIB([ext2fs], [ext2fs_open2],
[libext2fs="y"], [libext2fs="n"])
 AC_SUBST(libext2fs)
 AC_CHECK_LIB([gcrypt], [gcry_md_hash_buffer], [libgcrypt="y"],
[libgcrypt="n"])
 AC_SUBST(libgcrypt)
-AC_CHECK_LIB([pthread], [pthread_create], [] ,
-    [AC_MSG_ERROR([Could not find libpthread])])
+AX_CHECK_PTHREAD
 AC_CHECK_LIB([rt], [clock_gettime])
 AC_CHECK_LIB([yajl], [yajl_alloc], [],
     [AC_MSG_ERROR([Could not find yajl])])
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 55eb755..a1ba134 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -73,6 +73,8 @@ CFLAGS   += -I. $(CFLAGS_xeninclude)
 # Needed for posix_fadvise64() in xc_linux.c
 CFLAGS-$(CONFIG_Linux) += -D_GNU_SOURCE
 
+CFLAGS	+= $(PTHREAD_CFLAGS)
+
 # Define this to make it possible to run valgrind on code linked with these
 # libraries.
 #CFLAGS   += -DVALGRIND -O0 -ggdb3
@@ -157,7 +159,7 @@ libxenctrl.so.$(MAJOR): libxenctrl.so.$(MAJOR).$(MINOR)
 	ln -sf $< $@
 
 libxenctrl.so.$(MAJOR).$(MINOR): $(CTRL_PIC_OBJS)
-	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenctrl.so.$(MAJOR)
$(SHLIB_LDFLAGS) -o $@ $^ $(DLOPEN_LIBS) $(PTHREAD_LIBS) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG)
-Wl,libxenctrl.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(DLOPEN_LIBS)
$(PTHREAD_LIBS) $(APPEND_LDFLAGS)
 
 # libxenguest
 
diff --git a/tools/m4/pthread.m4 b/tools/m4/pthread.m4
new file mode 100644
index 0000000..57ea85c
--- /dev/null
+++ b/tools/m4/pthread.m4
@@ -0,0 +1,41 @@
+# We define, separately, PTHREAD_CFLAGS, _LDFLAGS and _LIBS
+# even though currently we don''t set them very separately.
+# This means that the makefiles will not need to change in
+# the future if we make the test more sophisticated.
+
+AC_DEFUN([AX_PTHREAD_CV2VARS],[
+    PTHREAD_CFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LDFLAGS="$ax_cv_pthread_flags"
+    PTHREAD_LIBS=""
+])
+
+# We invoke AX_PTHREAD_VARS with the name of another macro
+# which is then expanded once for each variable.
+AC_DEFUN([AX_PTHREAD_VARS],[$1(CFLAGS) $1(LDFLAGS) $1(LIBS)])
+
+AC_DEFUN([AX_PTHREAD_VAR_APPLY],[
+    $1="$$1 $PTHREAD_$1"
+])
+AC_DEFUN([AX_PTHREAD_VAR_SUBST],[AC_SUBST(PTHREAD_$1)])
+
+AC_DEFUN([AX_CHECK_PTHREAD],[
+    AC_CACHE_CHECK([for pthread flag], [ax_cv_pthread_flags], [
+        ax_cv_pthread_flags=-pthread
+        AX_PTHREAD_CV2VARS
+        AX_PTHREAD_VARS([AX_SAVEVAR_SAVE])
+        AX_PTHREAD_VARS([AX_PTHREAD_VAR_APPLY])
+        AC_LINK_IFELSE([
+#include <pthread.h>
+int main(void) {
+  pthread_atfork(0,0,0);
+  pthread_create(0,0,0,0);
+}
+],[],[ax_cv_pthread_flags=failed])
+        AX_PTHREAD_VARS([AX_SAVEVAR_RESTORE])
+    ])
+    if test "x$ax_cv_pthread_flags" = xfailed; then
+        AC_MSG_ERROR([-pthread does not work])
+    fi
+    AX_PTHREAD_CV2VARS
+    AX_PTHREAD_VARS([AX_PTHREAD_VAR_SUBST])
+])
diff --git a/tools/m4/savevar.m4 b/tools/m4/savevar.m4
new file mode 100644
index 0000000..2156bee
--- /dev/null
+++ b/tools/m4/savevar.m4
@@ -0,0 +1,6 @@
+AC_DEFUN([AX_SAVEVAR_SAVE],[
+    saved_$1="$$1"
+])
+AC_DEFUN([AX_SAVEVAR_RESTORE],[
+    $1="$saved_$1"
+])
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 08/19] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS

This is going to be needed for pthread_atfork.  It is a mystery why it
hasn''t been needed before.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/Makefile |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 748d057..b0143e6 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -22,6 +22,10 @@ endif
 LIBXL_LIBS  LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
$(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(UTIL_LIBS) $(LIBUUID_LIBS)
 
+CFLAGS += $(PTHREAD_CFLAGS)
+LDFLAGS += $(PTHREAD_LDFLAGS)
+LIBXL_LIBS += $(PTHREAD_LIBS)
+
 LIBXLU_LIBS  
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 09/19] tools: Use PTHREAD_CFLAGS, _LDFLAGS, _LIBS

Replace all literal occurrences of -lpthread and -pthread in Makefiles
by references to PTHREAD_CFLAGS, PTHREAD_LDFLAGS and PTHREAD_LIBS.
These are the new variables set by configure, and currently expand to
-pthread on the compilation and link lines as is required.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/blktap/drivers/Makefile       |    7 +++++--
 tools/libfsimage/common/Makefile    |    5 ++++-
 tools/vtpm_manager/manager/Makefile |    4 +++-
 tools/xenpaging/Makefile            |    5 +++--
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/tools/blktap/drivers/Makefile b/tools/blktap/drivers/Makefile
index addb2fe..941592e 100644
--- a/tools/blktap/drivers/Makefile
+++ b/tools/blktap/drivers/Makefile
@@ -35,8 +35,11 @@ else
 AIOLIBS     := -laio
 endif
 
-LDLIBS_blktapctrl := $(MEMSHRLIBS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore)
-L../lib -lblktap -lrt -lm -lpthread
-LDLIBS_img := $(AIOLIBS) $(CRYPT_LIB) -lpthread -lz
+CFLAGS += $(PTHREAD_CFLAGS)
+LDFLAGS += $(PTHREAD_LDFLAGS)
+
+LDLIBS_blktapctrl := $(MEMSHRLIBS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore)
-L../lib -lblktap -lrt -lm $(PTHREAD_LIBS)
+LDLIBS_img := $(AIOLIBS) $(CRYPT_LIB) $(PTHREAD_LIBS) -lz
 
 BLK-OBJS-y  := block-aio.o
 BLK-OBJS-y  += block-sync.o
diff --git a/tools/libfsimage/common/Makefile b/tools/libfsimage/common/Makefile
index 11621e7..3684913 100644
--- a/tools/libfsimage/common/Makefile
+++ b/tools/libfsimage/common/Makefile
@@ -8,6 +8,9 @@ LDFLAGS-$(CONFIG_SunOS) = -Wl,-M -Wl,mapfile-SunOS
 LDFLAGS-$(CONFIG_Linux) = -Wl,mapfile-GNU
 LDFLAGS = $(LDFLAGS-y)
 
+CFLAGS += $(PTHREAD_CFLAGS)
+LDFLAGS += $(PTHREAD_LDFLAGS)
+
 LIB_SRCS-y = fsimage.c fsimage_plugin.c fsimage_grub.c
 
 PIC_OBJS := $(patsubst %.c,%.opic,$(LIB_SRCS-y))
@@ -37,7 +40,7 @@ libfsimage.so.$(MAJOR): libfsimage.so.$(MAJOR).$(MINOR)
 	ln -sf $< $@
 
 libfsimage.so.$(MAJOR).$(MINOR): $(PIC_OBJS)
-	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libfsimage.so.$(MAJOR)
$(SHLIB_LDFLAGS) -o $@ $^ -lpthread
+	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libfsimage.so.$(MAJOR)
$(SHLIB_LDFLAGS) -o $@ $^ $(PTHREAD_LIBS)
 
 -include $(DEPS)
 
diff --git a/tools/vtpm_manager/manager/Makefile
b/tools/vtpm_manager/manager/Makefile
index 149f01d..7564af1 100644
--- a/tools/vtpm_manager/manager/Makefile
+++ b/tools/vtpm_manager/manager/Makefile
@@ -33,4 +33,6 @@ $(BIN): $(OBJS)
 
 # libraries
 LIBS += ../tcs/libTCS.a ../util/libTCGUtils.a ../crypto/libtcpaCrypto.a
-LIBS += -lcrypto -lpthread -lm
+LIBS += -lcrypto $(PTHREAD_LIBS) -lm
+CFLAGS += $(PTHREAD_CFLAGS)
+LDFLAGS += $(PTHREAD_LDFLAGS)
diff --git a/tools/xenpaging/Makefile b/tools/xenpaging/Makefile
index 08230a6..548d9dd 100644
--- a/tools/xenpaging/Makefile
+++ b/tools/xenpaging/Makefile
@@ -1,8 +1,9 @@
 XEN_ROOT=$(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
-CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore)
-LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) -pthread
+CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore) $(PTHREAD_CFLAGS)
+LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) $(PTHREAD_LIBS)
+LDFLAGS += $(PTHREAD_LDFLAGS)
 
 POLICY    = default
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 10/19] libxl: Crash (more sensibly) on malloc failure

Formally change the libxl memory allocation failure policy to "crash".

Previously we had a very uneven approach; much code assumed that
libxl__sprintf (for example) would never return NULL, but some code
was written more carefully.

We think it is unlikely that we will be able to make the library
actually robust against allocation failure (since that would be an
awful lot of never-tested error paths) and few calling environments
will be able to cope anyway.  So, instead, adopt the alternative
approach: provide allocation functions which never return null, but
will crash the whole process instead.

Consequently,
 - New noreturn function libxl__alloc_failed which may be used for
   printing a vaguely-useful error message, rather than simply
   dereferencing a null pointer.
 - libxl__ptr_add now returns void as it crashes on failure.
 - libxl__zalloc, _calloc, _strdup, _strndup, crash on failure using
   libxl__alloc_failed.  So all the code that uses these can no longer
   dereference null on malloc failure.

While we''re at it, make libxl__ptr_add use realloc rather than
emulating it with calloc and free, and make it grow the array
exponentially rather than linearly.

Things left to do:
 - Remove a lot of now-spurious error handling.
 - Remove the ERROR_NOMEM error code.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.h          |    4 ++
 tools/libxl/libxl_internal.c |   87 ++++++++++++++++++++---------------------
 tools/libxl/libxl_internal.h |    8 +++-
 3 files changed, 53 insertions(+), 46 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 2aec910..0219f81 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -194,6 +194,10 @@
  * No temporary objects allocated from the pool may be explicitly freed.
  * Therefore public functions which initialize a libxl__gc MUST call
  * libxl__free_all() before returning.
+ *
+ * Memory allocation failures are not handled gracefully.  If malloc
+ * (or realloc) fails, libxl will cause the entire process to print
+ * a message to stderr and exit with status 255.
  */
 /*
  * libxl types
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 12c32dc..4ed9412 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -17,40 +17,45 @@
 
 #include "libxl_internal.h"
 
-int libxl__error_set(libxl__gc *gc, int code)
-{
-    return 0;
+void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
+                         size_t nmemb, size_t size) {
+#define M "libxl: FATAL ERROR: memory allocation failure"
+#define L (size ? M " (%s, %lu x %lu)\n" : M " (%s)\n"), \
+          func, (unsigned long)nmemb, (unsigned long)size
+    libxl__log(ctx, XTL_CRITICAL, ENOMEM, 0,0, func, L);
+    fprintf(stderr, L);
+    fflush(stderr);
+    _exit(-1);
+#undef M
+#undef L
 }
 
-int libxl__ptr_add(libxl__gc *gc, void *ptr)
+void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
-    void **re;
 
     if (!ptr)
-        return 0;
+        return;
 
     /* fast case: we have space in the array for storing the pointer */
     for (i = 0; i < gc->alloc_maxsize; i++) {
         if (!gc->alloc_ptrs[i]) {
             gc->alloc_ptrs[i] = ptr;
-            return 0;
+            return;
         }
     }
-    /* realloc alloc_ptrs manually with calloc/free/replace */
-    re = calloc(gc->alloc_maxsize + 25, sizeof(void *));
-    if (!re)
-        return -1;
-    for (i = 0; i < gc->alloc_maxsize; i++)
-        re[i] = gc->alloc_ptrs[i];
-    /* assign the next pointer */
-    re[i] = ptr;
+    int new_maxsize = gc->alloc_maxsize * 2 + 25;
+    assert(new_maxsize < INT_MAX / sizeof(void*) / 2);
+    gc->alloc_ptrs = realloc(gc->alloc_ptrs, new_maxsize * sizeof(void
*));
+    if (!gc->alloc_ptrs)
+        libxl__alloc_failed(CTX, __func__, new_maxsize, sizeof(void*));
 
-    /* replace the old alloc_ptr */
-    free(gc->alloc_ptrs);
-    gc->alloc_ptrs = re;
-    gc->alloc_maxsize += 25;
-    return 0;
+    gc->alloc_ptrs[gc->alloc_maxsize++] = ptr;
+
+    while (gc->alloc_maxsize < new_maxsize)
+        gc->alloc_ptrs[gc->alloc_maxsize++] = 0;
+
+    return;
 }
 
 void libxl__free_all(libxl__gc *gc)
@@ -71,10 +76,7 @@ void libxl__free_all(libxl__gc *gc)
 void *libxl__zalloc(libxl__gc *gc, int bytes)
 {
     void *ptr = calloc(bytes, 1);
-    if (!ptr) {
-        libxl__error_set(gc, ENOMEM);
-        return NULL;
-    }
+    if (!ptr) libxl__alloc_failed(CTX, __func__, bytes, 1);
 
     libxl__ptr_add(gc, ptr);
     return ptr;
@@ -83,10 +85,7 @@ void *libxl__zalloc(libxl__gc *gc, int bytes)
 void *libxl__calloc(libxl__gc *gc, size_t nmemb, size_t size)
 {
     void *ptr = calloc(nmemb, size);
-    if (!ptr) {
-        libxl__error_set(gc, ENOMEM);
-        return NULL;
-    }
+    if (!ptr) libxl__alloc_failed(CTX, __func__, nmemb, size);
 
     libxl__ptr_add(gc, ptr);
     return ptr;
@@ -97,9 +96,8 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t
new_size)
     void *new_ptr = realloc(ptr, new_size);
     int i = 0;
 
-    if (new_ptr == NULL && new_size != 0) {
-        return NULL;
-    }
+    if (new_ptr == NULL && new_size != 0)
+        libxl__alloc_failed(CTX, __func__, new_size, 1);
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
@@ -112,7 +110,6 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t
new_size)
         }
     }
 
-
     return new_ptr;
 }
 
@@ -126,16 +123,13 @@ char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
     ret = vsnprintf(NULL, 0, fmt, ap);
     va_end(ap);
 
-    if (ret < 0) {
-        return NULL;
-    }
+    assert(ret >= 0);
 
     s = libxl__zalloc(gc, ret + 1);
-    if (s) {
-        va_start(ap, fmt);
-        ret = vsnprintf(s, ret + 1, fmt, ap);
-        va_end(ap);
-    }
+    va_start(ap, fmt);
+    ret = vsnprintf(s, ret + 1, fmt, ap);
+    va_end(ap);
+
     return s;
 }
 
@@ -143,8 +137,9 @@ char *libxl__strdup(libxl__gc *gc, const char *c)
 {
     char *s = strdup(c);
 
-    if (s)
-        libxl__ptr_add(gc, s);
+    if (!s) libxl__alloc_failed(CTX, __func__, strlen(c), 1);
+
+    libxl__ptr_add(gc, s);
 
     return s;
 }
@@ -153,8 +148,7 @@ char *libxl__strndup(libxl__gc *gc, const char *c, size_t n)
 {
     char *s = strndup(c, n);
 
-    if (s)
-        libxl__ptr_add(gc, s);
+    if (!s) libxl__alloc_failed(CTX, __func__, n, 1);
 
     return s;
 }
@@ -175,6 +169,9 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel,
int errnoval,
              const char *file, int line, const char *func,
              const char *fmt, va_list ap)
 {
+    /* WARNING this function may not call any libxl-provided
+     * memory allocation function, as those may
+     * call libxl__alloc_failed which calls libxl__logv. */
     char *enomem = "[out of memory formatting log message]";
     char *base = NULL;
     int rc, esave;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 04f13f6..2ad446a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -116,6 +116,12 @@ typedef struct libxl__gc libxl__gc;
 typedef struct libxl__egc libxl__egc;
 typedef struct libxl__ao libxl__ao;
 
+void libxl__alloc_failed(libxl_ctx *, const char *func,
+                         size_t nmemb, size_t size) __attribute__((noreturn));
+  /* func, size and nmemb are used only in the log message.
+   * You may pass size==0 if size and nmemb are not meaningful
+   * and should not be printed. */
+
 typedef struct libxl__ev_fd libxl__ev_fd;
 typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
                                    int fd, short events, short revents);
@@ -380,7 +386,7 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
  * collection on exit from the outermost libxl callframe.
  */
 /* register @ptr in @gc for free on exit from outermost libxl callframe. */
-_hidden int libxl__ptr_add(libxl__gc *gc, void *ptr);
+_hidden void libxl__ptr_add(libxl__gc *gc, void *ptr);
 /* if this is the outermost libxl callframe then free all pointers in @gc */
 _hidden void libxl__free_all(libxl__gc *gc);
 /* allocate and zero @bytes. (similar to a gc''d malloc(3)+memzero())
*/
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 11/19] libxl: Make libxl__zalloc et al tolerate a NULL gc

Arrange that if we pass NULL as a gc, we simply don''t register the
pointer.  This instantly gives us non-gc''ing but error-checking
versions of malloc, realloc, vasprintf, etc.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.c |    5 ++++-
 tools/libxl/libxl_internal.h |   21 +++++++++++++--------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 4ed9412..b89aef7 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -34,6 +34,9 @@ void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
 
+    if (!gc)
+        return;
+
     if (!ptr)
         return;
 
@@ -101,7 +104,7 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t
new_size)
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
-    } else if (new_ptr != ptr) {
+    } else if (new_ptr != ptr && gc != NULL) {
         for (i = 0; i < gc->alloc_maxsize; i++) {
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2ad446a..9340bde 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -384,30 +384,35 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
  *
  * All pointers returned by these functions are registered for garbage
  * collection on exit from the outermost libxl callframe.
+ *
+ * However, where the argument is stated to be "gc_opt", NULL may be
+ * passed instead, in which case no garbage collection will occur; the
+ * pointer must later be freed with free().  This is for memory
+ * allocations of types (b) and (c).
  */
 /* register @ptr in @gc for free on exit from outermost libxl callframe. */
-_hidden void libxl__ptr_add(libxl__gc *gc, void *ptr);
+_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr);
 /* if this is the outermost libxl callframe then free all pointers in @gc */
 _hidden void libxl__free_all(libxl__gc *gc);
 /* allocate and zero @bytes. (similar to a gc''d malloc(3)+memzero())
*/
-_hidden void *libxl__zalloc(libxl__gc *gc, int bytes);
+_hidden void *libxl__zalloc(libxl__gc *gc_opt, int bytes);
 /* allocate and zero memory for an array of @nmemb members of @size each.
  * (similar to a gc''d calloc(3)). */
-_hidden void *libxl__calloc(libxl__gc *gc, size_t nmemb, size_t size);
+_hidden void *libxl__calloc(libxl__gc *gc_opt, size_t nmemb, size_t size);
 /* change the size of the memory block pointed to by @ptr to @new_size bytes.
  * unlike other allocation functions here any additional space between the
  * oldsize and @new_size is not initialised (similar to a gc''d
realloc(3)). */
-_hidden void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size);
+_hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size);
 /* print @fmt into an allocated string large enoughto contain the result.
  * (similar to gc''d asprintf(3)). */
-_hidden char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
PRINTF_ATTRIBUTE(2, 3);
+_hidden char *libxl__sprintf(libxl__gc *gc_opt, const char *fmt, ...)
PRINTF_ATTRIBUTE(2, 3);
 /* duplicate the string @c (similar to a gc''d strdup(3)). */
-_hidden char *libxl__strdup(libxl__gc *gc, const char *c);
+_hidden char *libxl__strdup(libxl__gc *gc_opt, const char *c);
 /* duplicate at most @n bytes of string @c (similar to a gc''d
strndup(3)). */
-_hidden char *libxl__strndup(libxl__gc *gc, const char *c, size_t n);
+_hidden char *libxl__strndup(libxl__gc *gc_opt, const char *c, size_t n);
 /* strip the last path component from @s and return as a newly allocated
  * string. (similar to a gc''d dirname(3)). */
-_hidden char *libxl__dirname(libxl__gc *gc, const char *s);
+_hidden char *libxl__dirname(libxl__gc *gc_opt, const char *s);
 
 _hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array,
int length);
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 12/19] libxl: Introduce some convenience macros

We introduce:
   <type> *GCNEW(<type> *var);
   <type> *GCNEW_ARRAY(<type> *var, ssize_t nmemb);
   <type> *GCREALLOC_ARRAY(<type> *var, size_t nmemb);
   char *GCSPRINTF(const char *fmt, ...);
   void LOG(<xtl_level_suffix>, const char *fmt, ...);
   void LOGE(<xtl_level_suffix>, const char *fmt, ...);
   void LOGEV(<xtl_level_suffix>, int errnoval, const char *fmt, ...);
all of which expect, in the calling context,
   libxl__gc *gc;

Most of these will find callers in subsequent patches.  The exceptions
are the orthogonally necessary LOGE and LOGEV, and GCREALLOC_ARRAY.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |   72 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9340bde..95c34f3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1349,6 +1349,78 @@ _hidden void libxl__ao__destroy(libxl_ctx*, libxl__ao
*ao);
 #define GC_FREE       libxl__free_all(gc)
 #define CTX           libxl__gc_owner(gc)
 
+/* Allocation macros all of which use the gc. */
+
+#define ARRAY_SIZE_OK(ptr, nmemb) ((nmemb) < INT_MAX / (sizeof(*(ptr)) * 2))
+
+/*
+ * Expression statement  <type> *GCNEW(<type> *var);
+ * Uses                  libxl__gc *gc;
+ *
+ * Allocates a new object of type <type> from the gc and zeroes it
+ * with memset.  Sets var to point to the new object or zero (setting
+ * errno).  Returns the new value of var.
+ */
+#define GCNEW(var)                                      \
+    (((var) = libxl__zalloc((gc),sizeof(*(var)))))
+
+/*
+ * Expression statement  <type> *GCNEW_ARRAY(<type> *var, ssize_t
nmemb);
+ * Uses                  libxl__gc *gc;
+ *
+ * Like GCNEW but allocates an array of nmemb elements, as if from
+ * calloc.  Does check for integer overflow due to large nmemb.  If
+ * nmemb is 0 may succeed by returning 0.
+ */
+#define GCNEW_ARRAY(var, nmemb)                                 \
+    ((var) = libxl__calloc((gc), (nmemb), sizeof(*(var))))
+    
+/*
+ * Expression statement  <type> *GCREALLOC_ARRAY(<type> *var,
size_t nmemb);
+ * Uses                  libxl__gc *gc;
+ *
+ * Reallocates the array var to be of size nmemb elements.  Updates
+ * var and returns the new value of var.  Does check for integer
+ * overflow due to large nmemb.
+ *
+ * Do not pass nmemb==0.  old may be 0 on entry.
+ */
+#define GCREALLOC_ARRAY(var, nmemb)                                     \
+    (assert(nmemb > 0),                                                 \
+     assert(ARRAY_SIZE_OK((var), (nmemb))),                             \
+     (var) = libxl__realloc((gc), (var), (nmemb)*sizeof(*(var)))))
+
+
+/*
+ * Expression            char *GCSPRINTF(const char *fmt, ...);
+ * Uses                  libxl__gc *gc;
+ *
+ * Trivial convenience wrapper for libxl__sprintf.
+ */
+#define GCSPRINTF(fmt, ...) (libxl__sprintf((gc), (fmt), __VA_ARGS__))
+
+
+/*
+ * Expression statements
+ *    void LOG(<xtl_level_suffix>, const char *fmt, ...);
+ *    void LOGE(<xtl_level_suffix>, const char *fmt, ...);
+ *    void LOGEV(<xtl_level_suffix>, int errnoval, const char *fmt, ...);
+ * Use
+ *    libxl__gc *gc;
+ *
+ * Trivial convenience wrappers for LIBXL__LOG, LIBXL__LOG_ERRNO and
+ * LIBXL__LOG_ERRNOVAL, respectively (and thus for libxl__log).
+ *
+ * XTL_<xtl_level_suffix> should exist and be an xentoollog.h log level
+ * So <xtl_level_suffix> should be one of
+ *   DEBUG VERBOSE DETAIL PROGRESS INFO NOTICE WARN ERROR ERROR CRITICAL
+ * Of these, most of libxl uses
+ *   DEBUG INFO WARN ERROR
+ */
+#define LOG(l,f, ...)     LIBXL__LOG(CTX,XTL_##l,(f),##__VA_ARGS__)
+#define LOGE(l,f, ...)    LIBXL__LOG_ERRNO(CTX,XTL_##l,(f),##__VA_ARGS__)
+#define LOGEV(l,e,f, ...)
LIBXL__LOG_ERRNOVAL(CTX,XTL_##l,(e),(f),##__VA_ARGS__)
+
 
 /* Locking functions.  See comment for "lock" member of libxl__ctx.
*/
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 13/19] libxl: include <ctype.h> and introduce CTYPE helper macro

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 95c34f3..b35421f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -33,6 +33,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <ctype.h>
 
 #include <sys/mman.h>
 #include <sys/poll.h>
@@ -1469,6 +1470,25 @@ static inline void libxl__ctx_unlock(libxl_ctx *ctx) {
     } while(0)
 
 
+/*
+ * int CTYPE(ISFOO, char c);
+ * int CTYPE(toupper, char c);
+ * int CTYPE(tolower, char c);
+ *
+ * This is necessary because passing a simple char to a ctype.h
+ * is forbidden.  ctype.h macros take ints derived from _unsigned_ chars.
+ *
+ * If you have a char which might be EOF then you should already have
+ * it in an int representing an unsigned char, and you can use the
+ * <ctype.h> macros directly.  This generally happens only with values
+ * from fgetc et al.
+ *
+ * For any value known to be a character (eg, anything that came from
+ * a char[]), use CTYPE.
+ */
+#define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))
+
+
 #endif
 
 /*
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 14/19] libxl: Provide libxl_string_list_length

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c |    8 ++++++++
 tools/libxl/libxl.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f41b62f..8910420 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -177,6 +177,14 @@ void libxl_string_list_dispose(libxl_string_list *psl)
     free(sl);
 }
 
+int libxl_string_list_length(const libxl_string_list *psl)
+{
+    if (!psl) return 0;
+    int i = 0;
+    while (*psl++) i++;
+    return i;
+}
+
 void libxl_key_value_list_dispose(libxl_key_value_list *pkvl)
 {
     int i;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0219f81..b376316 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -273,6 +273,7 @@ typedef uint8_t libxl_mac[6];
 
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
+int libxl_string_list_length(const libxl_string_list *sl);
 
 typedef char **libxl_key_value_list;
 void libxl_key_value_list_dispose(libxl_key_value_list *kvl);
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 15/19] libxl: include <_libxl_paths.h> in libxl_internal.h

Ie, we permit general code in libxl direct access to the manifest
constants such as XEN_RUN_DIR.  This simplifies their use in (eg)
format strings.

This might be controversial because it will make it difficult to make
any of these runtime-configurable later without changing lots of use
sites.  But I don''t think it''s likely we''ll want to
do that.

For the moment, leave existing call sites of all the functions in
libxl_paths.c unchanged.  The simplified use arrangements can be used
in new code and when we update call sites for other reasons.

Also correct the dependencies in the Makefile so that _libxl_paths.h
is generated before anything that uses libxl_internal.h.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>
---
 tools/libxl/Makefile         |    4 +---
 tools/libxl/libxl_internal.h |    1 +
 tools/libxl/libxl_paths.c    |    1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index b0143e6..9f7e454 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -102,11 +102,9 @@ _libxl_list.h:
$(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE
 	perl $^ --prefix=libxl >$@.new
 	$(call move-if-changed,$@.new,$@)
 
-libxl_paths.c: _libxl_paths.h
-
 libxl.h: _libxl_types.h
 libxl_json.h: _libxl_types_json.h
-libxl_internal.h: _libxl_types_internal.h
+libxl_internal.h: _libxl_types_internal.h _libxl_paths.h
 libxl_internal_json.h: _libxl_types_internal_json.h
 
 $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS): libxl.h
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b35421f..740bde2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -51,6 +51,7 @@
 #include <xen/io/xenbus.h>
 
 #include "libxl.h"
+#include "_libxl_paths.h"
 
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1)
 #define _hidden __attribute__((visibility("hidden")))
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index a95d29f..388b344 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -14,7 +14,6 @@
 
 #include "libxl_osdeps.h" /* must come before any other headers */
 #include "libxl_internal.h"
-#include "_libxl_paths.h"
 
 const char *libxl_sbindir_path(void)
 {
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 16/19] libxl: abolish libxl_ctx_postfork

libxl''s task has become too complicated (particularly in the presence
of both forking and multithreading) to support reuse of the same
libxl_ctx after fork.

So abolish libxl_ctx_fork.  xl instead simply initialises a new
libxl_ctx.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.h       |    1 -
 tools/libxl/libxl_utils.c |    7 -------
 tools/libxl/xl.c          |    8 ++++++++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |    8 ++------
 5 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b376316..edbca53 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -461,7 +461,6 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags /* none currently defined */,
                     xentoollog_logger *lg);
 int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
-int libxl_ctx_postfork(libxl_ctx *ctx);
 
 /* domain related functions */
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index d6cd78d..0cbd85e 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -365,13 +365,6 @@ READ_WRITE_EXACTLY(read, 1, /* */)
 READ_WRITE_EXACTLY(write, 0, const)
 
 
-int libxl_ctx_postfork(libxl_ctx *ctx) {
-    if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh);
-    ctx->xsh = xs_daemon_open();
-    if (!ctx->xsh) return ERROR_FAIL;
-    return 0;
-}
-
 pid_t libxl_fork(libxl_ctx *ctx)
 {
     pid_t pid;
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 2b14814..62c0abd 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -94,6 +94,14 @@ static void parse_global_config(const char *configfile,
     xlu_cfg_destroy(config);
 }
 
+void postfork(void)
+{
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
(xentoollog_logger*)logger)) {
+        fprintf(stderr, "cannot reinit xl context after fork\n");
+        exit(-1);
+    }
+}
+
 int main(int argc, char **argv)
 {
     int opt = 0;
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 0a3d628..7e258d5 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -105,6 +105,7 @@ struct cmd_spec *cmdtable_lookup(const char *s);
 
 extern libxl_ctx *ctx;
 extern xentoollog_logger_stdiostream *logger;
+void postfork(void);
 
 /* global options */
 extern int autoballoon;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6f4dd09..c9e9943 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1441,7 +1441,7 @@ static int autoconnect_console(libxl_ctx *ctx, uint32_t
domid, void *priv)
     } else if (*pid > 0)
         return 0;
 
-    libxl_ctx_postfork(ctx);
+    postfork();
 
     sleep(1);
     libxl_primary_console_exec(ctx, domid);
@@ -1728,11 +1728,7 @@ start:
             goto out;
         }
 
-        rc = libxl_ctx_postfork(ctx);
-        if (rc) {
-            LOG("failed to reinitialise context after fork");
-            exit(-1);
-        }
+        postfork();
 
         if (asprintf(&name, "xl-%s", d_config.c_info.name) <
0) {
             LOG("Failed to allocate memory in asprintf");
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 17/19] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS

Introduce definition and use of a new function-local macro REQUIRE_FDS
to avoid repeatedly spelling out which fds we are interested in.

We are going to introduce a new fd for the SIGCHLD self-pipe.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_event.c |   82 ++++++++++++++++++++++++++++++--------------
 1 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 9cb172a..638b9ab 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -593,6 +593,45 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller
*poller,
     int rc;
 
     /*
+     * We need to look at the fds we want twice: firstly, to count
+     * them so we can make the rindex array big enough, and secondly
+     * to actually fill the arrays in.
+     *
+     * To ensure correctness and avoid repeating the logic for
+     * deciding which fds are relevant, we define a macro
+     *    REQUIRE_FDS( BODY )
+     * which calls
+     *    do{
+     *        int req_fd;
+     *        int req_events;
+     *        BODY;
+     *    }while(0)
+     * for each fd with a nonzero events.  This is invoked twice.
+     *
+     * The definition of REQUIRE_FDS is simplified with the helper
+     * macro
+     *    void REQUIRE_FD(int req_fd, int req_events, BODY);
+     */
+
+#define REQUIRE_FDS(BODY) do{                                          \
+                                                                       \
+        LIBXL_LIST_FOREACH(efd, &CTX->efds, entry)                     \
+            REQUIRE_FD(efd->fd, efd->events, BODY);                    \
+                                                                       \
+        REQUIRE_FD(poller->wakeup_pipe[0], POLLIN, BODY);              \
+                                                                       \
+    }while(0)
+
+#define REQUIRE_FD(req_fd_, req_events_, BODY) do{      \
+        int req_events = (req_events_);                 \
+        int req_fd = (req_fd_);                         \
+        if (req_events) {                               \
+            BODY;                                       \
+        }                                               \
+    }while(0)
+
+
+    /*
      * In order to be able to efficiently find the libxl__ev_fd
      * for a struct poll during _afterpoll, we maintain a shadow
      * data structure in CTX->fd_beforepolled: each slot in
@@ -609,13 +648,13 @@ static int beforepoll_internal(libxl__gc *gc,
libxl__poller *poller,
          * not to mess with fd_rindex.
          */
 
-        int maxfd = poller->wakeup_pipe[0] + 1;
-        LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
-            if (!efd->events)
-                continue;
-            if (efd->fd >= maxfd)
-                maxfd = efd->fd + 1;
-        }
+        int maxfd = 0;
+
+        REQUIRE_FDS({
+            if (req_fd >= maxfd)
+                maxfd = req_fd + 1;
+        });
+
         /* make sure our array is as big as *nfds_io */
         if (poller->fd_rindex_allocd < maxfd) {
             assert(maxfd < INT_MAX / sizeof(int) / 2);
@@ -630,25 +669,16 @@ static int beforepoll_internal(libxl__gc *gc,
libxl__poller *poller,
 
     int used = 0;
 
-#define REQUIRE_FD(req_fd, req_events, efd) do{                 \
-        if ((req_events)) {                                     \
-            if (used < *nfds_io) {                              \
-                fds[used].fd = (req_fd);                        \
-                fds[used].events = (req_events);                \
-                fds[used].revents = 0;                          \
-                assert((req_fd) < poller->fd_rindex_allocd);    \
-                poller->fd_rindex[(req_fd)] = used;             \
-            }                                                   \
-            used++;                                             \
-        }                                                       \
-    }while(0)
-
-    LIBXL_LIST_FOREACH(efd, &CTX->efds, entry)
-        REQUIRE_FD(efd->fd, efd->events, efd);
-
-    REQUIRE_FD(poller->wakeup_pipe[0], POLLIN, 0);
-
-#undef REQUIRE_FD
+    REQUIRE_FDS({
+        if (used < *nfds_io) {
+            fds[used].fd = req_fd;
+            fds[used].events = req_events;
+            fds[used].revents = 0;
+            assert(req_fd < poller->fd_rindex_allocd);
+            poller->fd_rindex[req_fd] = used;
+        }
+        used++;
+    });
 
     rc = used <= *nfds_io ? 0 : ERROR_BUFFERFULL;
 
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 18/19] libxl: Protect fds with CLOEXEC even with forking threads

We introduce a new "carefd" concept, which relates to fds that we care
about not being inherited by long-lived children.

As yet we do not use this anywhere in libxl.  Until all locations in
libxl which make such fds are converted, libxl__postfork may not work
entirely properly.  If these locations do not use O_CLOEXEC (or use
calls for which there is no O_CLOEXEC) then multithreaded programs may
not work properly.

This introduces a new API call libxl_postfork_child_noexec which must
be called by applications which make long-running non-execing
children.  Add the appropriate call to xl''s postfork function.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/Makefile         |    2 +-
 tools/libxl/libxl.c          |    3 +
 tools/libxl/libxl_event.h    |   13 ++++
 tools/libxl/libxl_fork.c     |  149 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   63 ++++++++++++++++++
 tools/libxl/xl.c             |    3 +
 6 files changed, 232 insertions(+), 1 deletions(-)
 create mode 100644 tools/libxl/libxl_fork.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 9f7e454..e5ea867 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -53,7 +53,7 @@ LIBXL_LIBS += -lyajl
 LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
 			libxl_internal.o libxl_utils.o libxl_uuid.o libxl_json.o \
-			libxl_qmp.o libxl_event.o $(LIBXL_OBJS-y)
+			libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
 $(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest)
$(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include $(XEN_ROOT)/tools/config.h
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 8910420..60dbfdc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -68,6 +68,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     /* Now ctx is safe for ctx_free; failures simply set rc and "goto
out" */
 
+    rc = libxl__atfork_init(ctx);
+    if (rc) goto out;
+
     rc = libxl__poller_init(ctx, &ctx->poller_app);
     if (rc) goto out;
 
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index ea553f6..2d2196f 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -371,6 +371,19 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void
*for_libxl,
  */
 void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
 
+
+/*
+ * An application which initialises a libxl_ctx in a parent process
+ * and then forks a child which does not quickly exec, must
+ * instead libxl_postfork_child_noexec in the child.  One call
+ * on any existing (or specially made) ctx is sufficient; after
+ * this all previously existing libxl_ctx''s are invalidated and
+ * must not be used - or even freed.  It is harmless to call this
+ * postfork function and then exec anyway.
+ */
+void libxl_postfork_child_noexec(libxl_ctx *ctx);
+
+
 #endif
 
 /*
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
new file mode 100644
index 0000000..dce88ad
--- /dev/null
+++ b/tools/libxl/libxl_fork.c
@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * 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.
+ */
+/*
+ * Internal child process machinery for use by other parts of libxl
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*
+ * carefd arrangements
+ *
+ * carefd_begin and _unlock take out the no_forking lock, which we
+ * also take and release in our pthread_atfork handlers.  So when this
+ * lock is held the whole process cannot fork.  We therefore protect
+ * our fds from leaking into children made by other threads.
+ *
+ * We maintain a list of all the carefds, so that if the application
+ * wants to fork a long-running but non-execing child, we can close
+ * them all.
+ *
+ * So the record function sets CLOEXEC for the benefit of execing
+ * children, and makes a note of the fd for the benefit of non-execing
+ * ones.
+ */
+
+struct libxl__carefd {
+    LIBXL_LIST_ENTRY(libxl__carefd) entry;
+    int fd;
+};
+
+static pthread_mutex_t no_forking = PTHREAD_MUTEX_INITIALIZER;
+static int atfork_registered;
+static LIBXL_LIST_HEAD(, libxl__carefd) carefds +   
LIBXL_LIST_HEAD_INITIALIZER(carefds);
+
+static void atfork_lock(void)
+{
+    int r = pthread_mutex_lock(&no_forking);
+    assert(!r);
+}
+
+static void atfork_unlock(void)
+{
+    int r = pthread_mutex_unlock(&no_forking);
+    assert(!r);
+}
+
+int libxl__atfork_init(libxl_ctx *ctx)
+{
+    int r, rc;
+    
+    atfork_lock();
+    if (atfork_registered) { rc = 0; goto out; }
+
+    r = pthread_atfork(atfork_lock, atfork_unlock, atfork_unlock);
+    if (r) {
+        assert(r == ENOMEM);
+        libxl__alloc_failed(ctx, __func__, 0,0);
+    }
+
+    atfork_registered = 1;
+    rc = 0;
+ out:
+    atfork_unlock();
+    return rc;
+}
+
+void libxl__carefd_begin(void) { atfork_lock(); }
+void libxl__carefd_unlock(void) { atfork_unlock(); }
+
+libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd)
+{
+    libxl__carefd *cf = 0;
+
+    libxl_fd_set_cloexec(ctx, fd, 1);
+    cf = libxl__zalloc(NULL, sizeof(*cf));
+    cf->fd = fd;
+    LIBXL_LIST_INSERT_HEAD(&carefds, cf, entry);
+    return cf;
+}
+
+libxl__carefd *libxl__carefd_opened(libxl_ctx *ctx, int fd)
+{
+    libxl__carefd *cf = 0;
+
+    cf = libxl__carefd_record(ctx, fd);
+    libxl__carefd_unlock();
+    return cf;
+}
+
+void libxl_postfork_child_noexec(libxl_ctx *ctx)
+{
+    libxl__carefd *cf, *cf_tmp;
+    int r;
+
+    atfork_lock();
+    LIBXL_LIST_FOREACH_SAFE(cf, &carefds, entry, cf_tmp) {
+        if (cf->fd >= 0) {
+            r = close(cf->fd);
+            if (r)
+                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING,
+                                 "failed to close fd=%d"
+                                 " (perhaps of another libxl ctx)",
cf->fd);
+        }
+        free(cf);
+    }
+    LIBXL_LIST_INIT(&carefds);
+    atfork_unlock();
+}
+
+int libxl__carefd_close(libxl__carefd *cf)
+{
+    if (!cf) return 0;
+    int r = cf->fd < 0 ? 0 : close(cf->fd);
+    int esave = errno;
+    atfork_lock();
+    LIBXL_LIST_REMOVE(cf, entry);
+    atfork_unlock();
+    free(cf);
+    errno = esave;
+    return r;
+}
+
+int libxl__carefd_fd(const libxl__carefd *cf)
+{
+    if (!cf) return -1;
+    return cf->fd;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 740bde2..a8372bb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -611,6 +611,9 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p);
 void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p);
 
 
+int libxl__atfork_init(libxl_ctx *ctx);
+
+
 /* from xl_dom */
 _hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
@@ -1307,6 +1310,66 @@ _hidden void libxl__ao_complete(libxl__egc *egc,
libxl__ao *ao, int rc);
 /* For use by ao machinery ONLY */
 _hidden void libxl__ao__destroy(libxl_ctx*, libxl__ao *ao);
 
+
+/*
+ * File descriptors and CLOEXEC
+ */
+
+/*
+ * For libxl functions which create file descriptors, at least one
+ * of the following must be true:
+ *  (a) libxl does not care if copies of this open-file are inherited
+ *      by random children and might remain open indefinitely
+ *  (b) libxl must take extra care for the fd (the actual descriptor,
+ *      not the open-file) as below.  We call this a "carefd".
+ *
+ * The rules for opening a carefd are:
+ *  (i)   Before bringing any carefds into existence,
+ *        libxl code must call libxl__carefd_begin.
+ *  (ii)  Then for each carefd brought into existence,
+ *        libxl code must call libxl__carefd_record
+ *        and remember the libxl__carefd_record*.
+ *  (iii) Then it must call libxl__carefd_unlock.
+ *  (iv)  When in a child process the fd is to be passed across
+ *        exec by libxl, the libxl code must unset FD_CLOEXEC
+ *        on the fd eg by using libxl_fd_set_cloexec.
+ *  (v)   Later, when the fd is to be closed in the same process,
+ *        libxl code must not call close.  Instead, it must call
+ *        libxl__carefd_close.
+ * Steps (ii) and (iii) can be combined by calling the convenience
+ * function libxl__carefd_opened.
+ */
+/* libxl__carefd_begin and _unlock (or _opened) must be called always
+ * in pairs.  They may be called with the CTX lock held.  In between
+ * _begin and _unlock, the following are prohibited:
+ *   - anything which might block
+ *   - any callbacks to the application
+ *   - nested calls to libxl__carefd_begin
+ *   - fork (libxl__fork)
+ * In general nothing should be done before _unlock that could be done
+ * afterwards.
+ */
+typedef struct libxl__carefd libxl__carefd;
+
+void libxl__carefd_begin(void);
+void libxl__carefd_unlock(void);
+
+/* fd may be -1, in which case this returns a dummy libxl__fd_record
+ * on which it _carefd_close is a no-op.  Cannot fail. */
+libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd);
+
+/* Combines _record and _unlock in a single call.  If fd==-1,
+ * still does the unlock, but returns 0.  Cannot fail. */
+libxl__carefd *libxl__carefd_opened(libxl_ctx *ctx, int fd);
+
+/* Works just like close(2).  You may pass NULL, in which case it''s
+ * a successful no-op. */
+int libxl__carefd_close(libxl__carefd*);
+
+/* You may pass NULL in which case the answer is -1. */
+int libxl__carefd_fd(const libxl__carefd*);
+
+
 /*
  * Convenience macros.
  */
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 62c0abd..a6ffd25 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -96,6 +96,9 @@ static void parse_global_config(const char *configfile,
 
 void postfork(void)
 {
+    libxl_postfork_child_noexec(ctx); /* in case we don''t exit/exec */
+    ctx = 0;
+
     if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0,
(xentoollog_logger*)logger)) {
         fprintf(stderr, "cannot reinit xl context after fork\n");
         exit(-1);
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 12:59 UTC

head link

[PATCH 19/19] libxl: provide STATE_AO_GC

Provide a convenience macro for use in ao callback functions, and
document that it should be used.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a8372bb..a4b933b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1266,9 +1266,10 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
  * - Note that during callback functions, two gcs are available:
  *    - The one in egc, whose lifetime is only this callback
  *    - The one in ao, whose lifetime is the asynchronous operation
- *   Usually callback function should use CONTAINER_OF
- *   to obtain its own structure, containing a pointer to the ao,
- *   and then use the gc from that ao.
+ *   Usually callback function should use CONTAINER_OF to obtain its
+ *   own state structure, containing a pointer to the ao.  It should
+ *   then obtain the ao and use the ao''s gc; this is most easily done
+ *   using the convenience macro STATE_AO_GC.
  */
 
 #define AO_CREATE(ctx, domid, ao_how)                           \
@@ -1298,6 +1299,10 @@ _hidden void libxl__egc_cleanup(libxl__egc *egc);
 #define AO_GC                                   \
     libxl__gc *const gc = &ao->gc
 
+#define STATE_AO_GC(op_ao)                      \
+    libxl__ao *const ao = (op_ao);              \
+    AO_GC
+
 
 /* All of these MUST be called with the ctx locked.
  * libxl__ao_inprogress MUST be called with the ctx locked exactly once. */
-- 
1.7.2.5

Ian Jackson

2012-Apr-11 13:35 UTC

head link

Re: [PATCH v5 00/19] libxl: improvements, prep for subprocess handling

Ian Jackson writes ("[PATCH v5 00/19] libxl: improvements, prep for
subprocess handling"):> This is the initial portion of my child process series which has been
> acked and which I intend to apply right away.  Changes are exactly
> those discussed on the list since v4; I''m reposting the final
version
> for form''s sake.
I have now pushed these to xen-unstable.  Thanks to Ian Campbell for
the reviews and to Roger Pau Monne for bug reports and fixes!

Ian.

Xen devel - Apr 2012 - [PATCH v5 00/19] libxl: improvements, prep for subprocess handling

[PATCH v5 00/19] libxl: improvements, prep for subprocess handling

[PATCH 01/19] .gitignore: Add a missing file

[PATCH 02/19] libxl: ao: allow immediate completion

[PATCH 03/19] libxl: fix hang due to libxl__initiate_device_remove

[PATCH 04/19] libxl: Fix eventloop_iteration over-locking

[PATCH 05/19] libxl: remove poller from list in libxl__poller_get

[PATCH 06/19] libxl: Fix leak of ctx->lock

[PATCH 07/19] tools: Correct PTHREAD options in config/StdGNU.mk

[PATCH 08/19] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS

[PATCH 09/19] tools: Use PTHREAD_CFLAGS, _LDFLAGS, _LIBS

[PATCH 10/19] libxl: Crash (more sensibly) on malloc failure

[PATCH 11/19] libxl: Make libxl__zalloc et al tolerate a NULL gc

[PATCH 12/19] libxl: Introduce some convenience macros

[PATCH 13/19] libxl: include <ctype.h> and introduce CTYPE helper macro

[PATCH 14/19] libxl: Provide libxl_string_list_length

[PATCH 15/19] libxl: include <_libxl_paths.h> in libxl_internal.h

[PATCH 16/19] libxl: abolish libxl_ctx_postfork

[PATCH 17/19] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS

[PATCH 18/19] libxl: Protect fds with CLOEXEC even with forking threads

[PATCH 19/19] libxl: provide STATE_AO_GC

Re: [PATCH v5 00/19] libxl: improvements, prep for subprocess handling