* (no subject)
@ 2012-02-24 18:54 Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
` (15 more replies)
0 siblings, 16 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel
Once again, I have not executed the code in this series!
These three are intended to be bugfixes to sort out
the deadlock problem that Roger Pau Monne reported:
01/15 libxl: ao: allow immediate completion
02/15 libxl: fix hang due to libxl__initiate_device_remove
These are other bugfixes:
03/15 libxl: Fix eventloop_iteration over-locking
04/15 libxl: Fix leak of ctx->lock
05/15 libxl: abolish libxl_ctx_postfork
06/15 tools: Correct PTHREAD options in config/StdGNU.mk
07/15 libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS
08/15 libxl: Crash (more sensibly) on malloc failure
These are handy new bits for internal libxl users:
09/15 libxl: Make libxl__zalloc et al tolerate a NULL gc
10/15 libxl: Introduce some convenience macros
11/15 libxl: Protect fds with CLOEXEC even with forking threads
12/15 libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS
14/15 libxl: Provide libxl_string_list_length
15/15 libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds
This is the new child-handling machinery:
13/15 libxl: event API: new facilities for waiting for subprocesses
There are not any users for much of this code in this series. I have
a half-written rework of libxl_bootloader.c to make it event-driven,
but it's not suitable for review at this stage. It doesn't even
compile.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 01/15] libxl: ao: allow immediate completion
2012-02-24 18:54 (no subject) Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove Ian Jackson
` (14 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
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 46e131a..2dc820c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -337,7 +337,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;
@@ -1186,7 +1186,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:
*
@@ -1217,8 +1221,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
@@ -1232,12 +1235,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); \
})
@@ -1246,6 +1251,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove
2012-02-24 18:54 (no subject) Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 03/15] libxl: Fix eventloop_iteration over-locking Ian Jackson
` (13 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
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 68bba8f..7735223 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1362,7 +1362,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;
@@ -1815,7 +1815,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;
@@ -2159,7 +2159,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;
@@ -2286,7 +2286,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 2dc820c..0536f8c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -669,7 +669,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 03/15] libxl: Fix eventloop_iteration over-locking
2012-02-24 18:54 (no subject) Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
2012-02-24 18:54 ` [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 04/15] libxl: Fix leak of ctx->lock Ian Jackson
` (12 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
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 0536f8c..ac892ae 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1168,7 +1168,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: */
@@ -1264,7 +1264,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 04/15] libxl: Fix leak of ctx->lock
2012-02-24 18:54 (no subject) Ian Jackson
` (2 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 03/15] libxl: Fix eventloop_iteration over-locking Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 05/15] libxl: abolish libxl_ctx_postfork Ian Jackson
` (11 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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 7735223..fd890cf 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 05/15] libxl: abolish libxl_ctx_postfork
2012-02-24 18:54 (no subject) Ian Jackson
` (3 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 04/15] libxl: Fix leak of ctx->lock Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk Ian Jackson
` (10 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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 1bffa03..19ef1de 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -312,7 +312,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 */
int libxl_init_create_info(libxl_ctx *ctx, libxl_domain_create_info *c_info);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index cd819c8..9f9f91d 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -399,13 +399,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 df9b1e7..9fd67b4 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -95,6 +95,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 702b208..394a128 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -104,6 +104,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 c58e9f3..2018153 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1422,7 +1422,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);
@@ -1709,11 +1709,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk
2012-02-24 18:54 (no subject) Ian Jackson
` (4 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 05/15] libxl: abolish libxl_ctx_postfork Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS Ian Jackson
` (9 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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.
So abolish PTHREAD_LIBS in StdGNU.mk, and add PTHREAD_CFLAGS and
PTHREAD_LDFLAGS instead. Fix the one user (libxc).
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 version of this patch.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
config/StdGNU.mk | 4 +++-
tools/libxc/Makefile | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 2af2841..97445a7 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -68,10 +68,12 @@ XEN_SCRIPT_DIR = $(XEN_CONFIG_DIR)/scripts
SOCKET_LIBS =
CURSES_LIBS = -lncurses
-PTHREAD_LIBS = -lpthread
UTIL_LIBS = -lutil
DLOPEN_LIBS = -ldl
+PTHREAD_CFLAGS = -pthread
+PTHREAD_LDFLAGS = -pthread
+
SONAME_LDFLAG = -soname
SHLIB_LDFLAGS = -shared
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index b5e7022..09e2f5f 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -72,6 +72,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
@@ -156,7 +158,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
--
1.7.2.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS
2012-02-24 18:54 (no subject) Ian Jackson
` (5 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure Ian Jackson
` (8 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
tools/libxl/Makefile | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 06764f2..347f192 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -26,6 +26,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure
2012-02-24 18:54 (no subject) Ian Jackson
` (6 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc Ian Jackson
` (7 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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:
- Provide versions of malloc, realloc and free which do the
checking but which do not enroll the pointer in the gc.
- Remove a lot of now-spurious error handling.
- Remove the ERROR_NOMEM error code.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.h | 4 ++
tools/libxl/libxl_internal.c | 79 ++++++++++++++++++------------------------
tools/libxl/libxl_internal.h | 5 ++-
3 files changed, 42 insertions(+), 46 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 19ef1de..65961d2 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -123,6 +123,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.
*/
#ifndef LIBXL_H
#define LIBXL_H
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 12c32dc..dfa2153 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -17,40 +17,40 @@
#include "libxl_internal.h"
-int libxl__error_set(libxl__gc *gc, int code)
-{
- return 0;
+void libxl__alloc_failed(const char *func, size_t nmemb, size_t size) {
+ fprintf(stderr,
+ "libxl: FATAL ERROR: memory allocation failure (%s, %lu x %lu)\n",
+ func, (unsigned long)nmemb, (unsigned long)size);
+ fflush(stderr);
+ _exit(-1);
}
-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(__func__, sizeof(void*), new_maxsize);
- /* 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 +71,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(__func__, bytes, 1);
libxl__ptr_add(gc, ptr);
return ptr;
@@ -83,10 +80,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(__func__, nmemb, size);
libxl__ptr_add(gc, ptr);
return ptr;
@@ -97,9 +91,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(__func__, new_size, 1);
if (ptr == NULL) {
libxl__ptr_add(gc, new_ptr);
@@ -112,7 +105,6 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
}
}
-
return new_ptr;
}
@@ -126,16 +118,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 +132,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(__func__, strlen(c), 1);
+
+ libxl__ptr_add(gc, s);
return s;
}
@@ -153,8 +143,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(__func__, n, 1);
return s;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ac892ae..ab2898e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -116,6 +116,9 @@ typedef struct libxl__gc libxl__gc;
typedef struct libxl__egc libxl__egc;
typedef struct libxl__ao libxl__ao;
+void libxl__alloc_failed(const char *func, size_t nmemb, size_t size)
+ __attribute__((noreturn));
+
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);
@@ -370,7 +373,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc
2012-02-24 18:54 (no subject) Ian Jackson
` (7 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 10/15] libxl: Introduce some convenience macros Ian Jackson
` (6 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
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 dfa2153..de7c3a8 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -29,6 +29,9 @@ void libxl__ptr_add(libxl__gc *gc, void *ptr)
{
int i;
+ if (!gc)
+ return;
+
if (!ptr)
return;
@@ -96,7 +99,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 ab2898e..33b588a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -371,30 +371,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 10/15] libxl: Introduce some convenience macros
2012-02-24 18:54 (no subject) Ian Jackson
` (8 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:54 ` [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads Ian Jackson
` (5 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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;
Signed-off-by: Ian Jackson <ian.jackson@eu.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 33b588a..b2f9092 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1323,6 +1323,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads
2012-02-24 18:54 (no subject) Ian Jackson
` (9 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 10/15] libxl: Introduce some convenience macros Ian Jackson
@ 2012-02-24 18:54 ` Ian Jackson
2012-02-24 18:55 ` [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS Ian Jackson
` (4 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:54 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 3 +
tools/libxl/libxl_event.h | 12 ++++
tools/libxl/libxl_fork.c | 146 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 63 ++++++++++++++++++
tools/libxl/xl.c | 3 +
6 files changed, 228 insertions(+), 1 deletions(-)
create mode 100644 tools/libxl/libxl_fork.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 347f192..a041294 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -57,7 +57,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)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index fd890cf..716163a 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 f889115..f8bc22f 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -369,6 +369,18 @@ 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.
+ */
+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..4aaa0b5
--- /dev/null
+++ b/tools/libxl/libxl_fork.c
@@ -0,0 +1,146 @@
+/*
+ * 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) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "pthread_atfork failed");
+ rc = ERROR_NOMEM;
+ goto out;
+ }
+
+ 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);
+ 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;
+ LIBXL_LIST_REMOVE(cf, entry);
+ 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 b2f9092..3d0379e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -596,6 +596,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);
@@ -1279,6 +1282,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 9fd67b4..d806077 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -97,6 +97,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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS
2012-02-24 18:54 (no subject) Ian Jackson
` (10 preceding siblings ...)
2012-02-24 18:54 ` [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads Ian Jackson
@ 2012-02-24 18:55 ` Ian Jackson
2012-02-24 18:55 ` [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
` (3 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:55 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
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>
---
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 5ac6334..5405299 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses
2012-02-24 18:54 (no subject) Ian Jackson
` (11 preceding siblings ...)
2012-02-24 18:55 ` [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS Ian Jackson
@ 2012-02-24 18:55 ` Ian Jackson
2012-02-24 18:55 ` [PATCH 14/15] libxl: Provide libxl_string_list_length Ian Jackson
` (2 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:55 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
The current arrangements in libxl for spawning subprocesses have two
key problems: (i) they make unwarranted (and largely undocumented)
assumptions about the caller's use of subprocesses, (ii) they aren't
integrated into the event system and can't be made asynchronous etc.
So replace them with a new set of facilities.
Primarily, from the point of view of code inside libxl, this is
libxl__ev_child_fork which is both (a) a version of fork() and (b) an
event source which generates a callback when the child dies.
>From the point of view of the application, we fully document our use
of SIGCHLD. The application can tell us whether it wants to own
SIGCHLD or not; if it does, it has to tell us about deaths of our
children.
Currently there are no callers in libxl which use these facilities.
All code in libxl which forks needs to be converted and libxl_fork
needse to be be abolished.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl.c | 17 +++-
tools/libxl/libxl_event.c | 52 +++++++--
tools/libxl/libxl_event.h | 142 ++++++++++++++++++++++++-
tools/libxl/libxl_fork.c | 243 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 61 ++++++++++-
5 files changed, 495 insertions(+), 20 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 716163a..06ac064 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -39,7 +39,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
memset(ctx, 0, sizeof(libxl_ctx));
ctx->lg = lg;
- /* First initialise pointers (cannot fail) */
+ /* First initialise pointers etc. (cannot fail) */
LIBXL_TAILQ_INIT(&ctx->occurred);
@@ -58,6 +58,11 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
LIBXL_TAILQ_INIT(&ctx->death_list);
libxl__ev_xswatch_init(&ctx->death_watch);
+ ctx->childproc_hooks = &libxl__childproc_default_hooks;
+ ctx->childproc_user = 0;
+
+ ctx->sigchld_selfpipe[0] = -1;
+
/* The mutex is special because we can't idempotently destroy it */
if (libxl__init_recursive_mutex(ctx, &ctx->lock) < 0) {
@@ -160,6 +165,16 @@ int libxl_ctx_free(libxl_ctx *ctx)
discard_events(&ctx->occurred);
+ /* If we have outstanding children, then the application inherits
+ * them; we wish the application good luck with understanding
+ * this if and when it reaps them. */
+ libxl__sigchld_removehandler(ctx);
+
+ if (ctx->sigchld_selfpipe[0] >= 0) {
+ close(ctx->sigchld_selfpipe[0]);
+ close(ctx->sigchld_selfpipe[1]);
+ }
+
pthread_mutex_destroy(&ctx->lock);
GC_FREE;
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 5405299..dcd5802 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -620,6 +620,10 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
\
REQUIRE_FD(poller->wakeup_pipe[0], POLLIN, BODY); \
\
+ int selfpipe = libxl__fork_selfpipe_active(CTX); \
+ if (selfpipe >= 0) \
+ REQUIRE_FD(selfpipe, POLLIN, BODY); \
+ \
}while(0)
#define REQUIRE_FD(req_fd_, req_events_, BODY) do{ \
@@ -749,10 +753,11 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
int nfds, const struct pollfd *fds,
struct timeval now)
{
+ /* May make callbacks into the appliction for child processes.
+ * ctx must be locked exactly once */
EGC_GC;
libxl__ev_fd *efd;
-
LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
if (!efd->events)
continue;
@@ -763,11 +768,16 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
}
if (afterpoll_check_fd(poller,fds,nfds, poller->wakeup_pipe[0],POLLIN)) {
- char buf[256];
- int r = read(poller->wakeup_pipe[0], buf, sizeof(buf));
- if (r < 0)
- if (errno != EINTR && errno != EWOULDBLOCK)
- LIBXL__EVENT_DISASTER(egc, "read wakeup", errno, 0);
+ int e = libxl__self_pipe_eatall(poller->wakeup_pipe[0]);
+ if (e) LIBXL__EVENT_DISASTER(egc, "read wakeup", e, 0);
+ }
+
+ int selfpipe = libxl__fork_selfpipe_active(CTX);
+ if (selfpipe >= 0 &&
+ afterpoll_check_fd(poller,fds,nfds, selfpipe, POLLIN)) {
+ int e = libxl__self_pipe_eatall(selfpipe);
+ if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
+ libxl__fork_selfpipe_woken(egc);
}
for (;;) {
@@ -1063,16 +1073,36 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p)
void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p)
{
+ int e = libxl__self_pipe_wakeup(p->wakeup_pipe[1]);
+ if (e) LIBXL__EVENT_DISASTER(egc, "cannot poke watch pipe", e, 0);
+}
+
+int libxl__self_pipe_wakeup(int fd)
+{
static const char buf[1] = "";
for (;;) {
- int r = write(p->wakeup_pipe[1], buf, 1);
- if (r==1) return;
+ int r = write(fd, buf, 1);
+ if (r==1) return 0;
assert(r==-1);
if (errno == EINTR) continue;
- if (errno == EWOULDBLOCK) return;
- LIBXL__EVENT_DISASTER(egc, "cannot poke watch pipe", errno, 0);
- return;
+ if (errno == EWOULDBLOCK) return 0;
+ assert(errno);
+ return errno;
+ }
+}
+
+int libxl__self_pipe_eatall(int fd)
+{
+ char buf[256];
+ for (;;) {
+ int r = read(fd, buf, sizeof(buf));
+ if (r >= 0) return 0;
+ assert(r == -1);
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) return 0;
+ assert(errno);
+ return errno;
}
}
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index f8bc22f..01e970a 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -161,11 +161,6 @@ void libxl_event_register_callbacks(libxl_ctx *ctx,
* After libxl_ctx_free, all corresponding evgen handles become
* invalid and must no longer be passed to evdisable.
*
- * Events enabled with evenable prior to a fork and libxl_ctx_postfork
- * are no longer generated after the fork/postfork; however the evgen
- * structures are still valid and must be passed to evdisable if the
- * memory they use should not be leaked.
- *
* Applications should ensure that they eventually retrieve every
* event using libxl_event_check or libxl_event_wait, since events
* which occur but are not retreived by the application will be queued
@@ -370,6 +365,142 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+/*======================================================================*/
+
+/*
+ * Subprocess handling.
+ *
+ * Unfortunately the POSIX interface makes this very awkward.
+ *
+ * There are two possible arrangements for collecting statuses from
+ * wait/waitpid.
+ *
+ * For naive programs:
+ *
+ * libxl will keep a SIGCHLD handler installed whenever it has an
+ * active (unreaped) child. It will reap all children with
+ * wait(); any children it does not recognise will be passed to
+ * the application via an optional callback (and will result in
+ * logged warnings if no callback is provided or the callback
+ * denies responsibility for the child).
+ *
+ * libxl may have children whenever:
+ *
+ * - libxl is performing an operation which can be made
+ * asynchronous; ie one taking a libxl_asyncop_how, even
+ * if NULL is passed indicating that the operation is
+ * synchronous; or
+ *
+ * - events of any kind are being generated, as requested
+ * by libxl_evenable_....
+ *
+ * A multithreaded application which is naive in this sense may
+ * block SIGCHLD on some of its threads, but there must be at
+ * least one thread that has SIGCHLD unblocked. libxl will not
+ * modify the blocking flag for SIGCHLD (except that it may create
+ * internal service threads with all signals blocked).
+ *
+ * A naive program must only have at any one time only
+ * one libxl context which might have children.
+ *
+ * For programs which run their own children alongside libxl's:
+ *
+ * The application must install a SIGCHLD handler and reap (at
+ * least) all of libxl's children and pass their exit status
+ * to libxl by calling libxl_childproc_exited.
+ *
+ * An application which does this must call
+ * libxl_childproc_setmode.
+ *
+ * An application which fails to call setmode, or which passes 0 for
+ * hooks, while it uses any libxl operation which might
+ * create or use child processes (see above):
+ * - Must not have any child processes running.
+ * - Must not install a SIGCHLD handler.
+ * - Must not reap any children.
+ */
+
+
+typedef enum {
+ /* libxl owns SIGCHLD whenever it has a child. */
+ libxl_sigchld_owner_libxl,
+
+ /* Application promises to call libxl_childproc_exited but NOT
+ * from within a signal handler. libxl will not itself arrange to
+ * (un)block or catch SIGCHLD. */
+ libxl_sigchld_owner_mainloop,
+
+ /* libxl owns SIGCHLD all the time, and the application is
+ * relying on libxl's event loop for reaping its own children. */
+ libxl_sigchld_owner_libxl_always,
+} libxl_sigchld_owner;
+
+typedef struct {
+ libxl_sigchld_owner chldowner;
+
+ /* All of these are optional: */
+
+ /* Called by libxl instead of fork. Should behave exactly like
+ * fork, including setting errno etc. May NOT reenter into libxl.
+ * Application may use this to discover pids of libxl's children,
+ * for example.
+ */
+ pid_t (*fork_replacement)(void *user);
+
+ /* With libxl_sigchld_owner_libxl, called by libxl when it has
+ * reaped a pid. (Not permitted with _owner_mainloop.)
+ *
+ * Should return 0 if the child was recognised by the application
+ * (or if the application does not keep those kind of records),
+ * ERROR_NOT_READY if the application knows that the child is not
+ * the application's; if it returns another error code it is a
+ * disaster as described for libxl_event_register_callbacks.
+ * (libxl will report unexpected children to its error log.)
+ *
+ * If not supplied, the application is assumed not to start
+ * any children of its own.
+ *
+ * This function is NOT called from within the signal handler.
+ * Rather it will be called from inside a libxl's event handling
+ * code and thus only when libxl is running, for example from
+ * within libxl_event_wait. (libxl uses the self-pipe trick
+ * to implement this.)
+ *
+ * childproc_exited_callback may call back into libxl, but it
+ * is best to avoid making long-running libxl calls as that might
+ * stall the calling event loop while the nested operation
+ * completes.
+ */
+ int (*reaped_callback)(pid_t, int status, void *user);
+} libxl_childproc_hooks;
+
+/* hooks may be 0 in which is equivalent to &{ libxl_sigchld_owner_libxl, 0, 0 }
+ *
+ * May not be called when libxl might have any child processes, or the
+ * behaviour is undefined. So it is best to call this at
+ * initialisation.
+ */
+void libxl_childproc_setmode(libxl_ctx *ctx, const libxl_childproc_hooks *hooks,
+ void *user);
+
+/*
+ * This function is for an application which owns SIGCHLD and which
+ * therefore reaps all of the process's children.
+ *
+ * May be called only by an application which has called setmode with
+ * chldowner == libxl_sigchld_owner_mainloop. If pid was a process started
+ * by this instance of libxl, returns 0 after doing whatever
+ * processing is appropriate. Otherwise silently returns
+ * ERROR_NOT_READY. No other error returns are possible.
+ *
+ * May NOT be called from within a signal handler which might
+ * interrupt any libxl operation. The application will almost
+ * certainly need to use the self-pipe trick (or a working pselect or
+ * ppoll) to implement this.
+ */
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status);
+
+
/*
* An application which initialises a libxl_ctx in a parent process
* and then forks a child which does not quickly exec, must
@@ -381,6 +512,7 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
void libxl_postfork_child_noexec(libxl_ctx *ctx);
+
#endif
/*
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 4aaa0b5..ee62b61 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -46,6 +46,12 @@ static int atfork_registered;
static LIBXL_LIST_HEAD(, libxl__carefd) carefds =
LIBXL_LIST_HEAD_INITIALIZER(carefds);
+/* non-null iff installed, protected by no_forking */
+static libxl_ctx *sigchld_owner;
+static struct sigaction sigchld_saved_action;
+
+static void sigchld_removehandler_core(void);
+
static void atfork_lock(void)
{
int r = pthread_mutex_lock(&no_forking);
@@ -108,6 +114,7 @@ void libxl_postfork_child_noexec(libxl_ctx *ctx)
int r;
atfork_lock();
+
LIBXL_LIST_FOREACH_SAFE(cf, &carefds, entry, cf_tmp) {
if (cf->fd >= 0) {
r = close(cf->fd);
@@ -117,6 +124,10 @@ void libxl_postfork_child_noexec(libxl_ctx *ctx)
free(cf);
}
LIBXL_LIST_INIT(&carefds);
+
+ if (sigchld_owner)
+ sigchld_removehandler_core();
+
atfork_unlock();
}
@@ -138,6 +149,238 @@ int libxl__carefd_fd(const libxl__carefd *cf)
}
/*
+ * Actual child process handling
+ */
+
+static void sigchld_handler(int signo)
+{
+ int e = libxl__self_pipe_wakeup(sigchld_owner->sigchld_selfpipe[1]);
+ assert(!e); /* errors are probably EBADF, very bad */
+}
+
+static void sigchld_removehandler_core(void)
+{
+ struct sigaction was;
+ int r;
+
+ r = sigaction(SIGCHLD, &sigchld_saved_action, &was);
+ assert(!r);
+ assert(!(was.sa_flags & SA_SIGINFO));
+ assert(was.sa_handler == sigchld_handler);
+ sigchld_owner = 0;
+}
+
+void libxl__sigchld_removehandler(libxl_ctx *ctx) /* non-reentrant */
+{
+ atfork_lock();
+ if (sigchld_owner == ctx)
+ sigchld_removehandler_core();
+ atfork_unlock();
+}
+
+int libxl__sigchld_installhandler(libxl_ctx *ctx) /* non-reentrant */
+{
+ int r, rc;
+
+ if (ctx->sigchld_selfpipe[0] < 0) {
+ r = pipe(ctx->sigchld_selfpipe);
+ if (r) {
+ ctx->sigchld_selfpipe[0] = -1;
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "failed to create sigchld pipe");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ atfork_lock();
+ if (sigchld_owner != ctx) {
+ struct sigaction ours;
+
+ assert(!sigchld_owner);
+ sigchld_owner = ctx;
+
+ memset(&ours,0,sizeof(ours));
+ ours.sa_handler = sigchld_handler;
+ sigemptyset(&ours.sa_mask);
+ ours.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+ r = sigaction(SIGCHLD, &ours, &sigchld_saved_action);
+ assert(!r);
+
+ assert(((void)"application must negotiate with libxl about SIGCHLD",
+ !(sigchld_saved_action.sa_flags & SA_SIGINFO) &&
+ (sigchld_saved_action.sa_handler == SIG_DFL ||
+ sigchld_saved_action.sa_handler == SIG_IGN)));
+ }
+ atfork_unlock();
+
+ rc = 0;
+ out:
+ return rc;
+}
+
+static int chldmode_ours(libxl_ctx *ctx)
+{
+ return ctx->childproc_hooks->chldowner == libxl_sigchld_owner_libxl;
+}
+
+int libxl__fork_selfpipe_active(libxl_ctx *ctx)
+{
+ /* Returns the fd to read, or -1 */
+ if (!chldmode_ours(ctx))
+ return -1;
+
+ if (LIBXL_LIST_EMPTY(&ctx->children))
+ return -1;
+
+ return ctx->sigchld_selfpipe[0];
+}
+
+static void perhaps_removehandler(libxl_ctx *ctx)
+{
+ if (LIBXL_LIST_EMPTY(&ctx->children) &&
+ ctx->childproc_hooks->chldowner != libxl_sigchld_owner_libxl_always)
+ libxl__sigchld_removehandler(ctx);
+}
+
+static int childproc_reaped(libxl__egc *egc, pid_t pid, int status)
+{
+ EGC_GC;
+ libxl__ev_child *ch;
+
+ LIBXL_LIST_FOREACH(ch, &CTX->children, entry)
+ if (ch->pid == pid)
+ goto found;
+
+ /* not found */
+ return ERROR_NOT_READY;
+
+ found:
+ LIBXL_LIST_REMOVE(ch, entry);
+ ch->pid = -1;
+ ch->callback(egc, ch, pid, status);
+
+ perhaps_removehandler(CTX);
+
+ return 0;
+}
+
+int libxl_childproc_reaped(libxl_ctx *ctx, pid_t pid, int status)
+{
+ EGC_INIT(ctx);
+ CTX_LOCK;
+ int rc = childproc_reaped(egc, pid, status);
+ CTX_UNLOCK;
+ EGC_FREE;
+ return rc;
+}
+
+void libxl__fork_selfpipe_woken(libxl__egc *egc) {
+ /* May make callbacks into the appliction for child processes.
+ * ctx must be locked EXACTLY ONCE */
+ EGC_GC;
+
+ while (chldmode_ours(CTX) /* in case the app changes the mode */) {
+ int status;
+ pid_t pid = waitpid(-1, &status, WNOHANG);
+
+ if (pid == 0) return;
+
+ if (pid == -1) {
+ if (errno == ECHILD) return;
+ if (errno == EINTR) continue;
+ LIBXL__EVENT_DISASTER(egc, "waitpid() failed", errno, 0);
+ return;
+ }
+
+ int rc = childproc_reaped(egc, pid, status);
+
+ if (rc) {
+ if (CTX->childproc_hooks->reaped_callback) {
+ CTX_UNLOCK;
+ CTX->childproc_hooks->reaped_callback
+ (pid, status, CTX->childproc_user);
+ CTX_LOCK;
+ } else {
+ libxl_report_child_exitstatus(CTX, XTL_WARN,
+ "unknown child", (long)pid, status);
+ }
+ }
+ }
+}
+
+pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *ch,
+ libxl__ev_child_callback *death)
+{
+ CTX_LOCK;
+ int rc;
+
+ if (chldmode_ours(CTX)) {
+ rc = libxl__sigchld_installhandler(CTX);
+ if (rc) goto out;
+ }
+
+ pid_t pid =
+ CTX->childproc_hooks->fork_replacement
+ ? CTX->childproc_hooks->fork_replacement(CTX->childproc_user)
+ : fork();
+ if (pid == -1) {
+ LOGE(ERROR, "fork failed");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if (!pid) {
+ /* woohoo! */
+ return 0; /* Yes, CTX is left locked in the child. */
+ }
+
+ ch->pid = pid;
+ ch->callback = death;
+ LIBXL_LIST_INSERT_HEAD(&CTX->children, ch, entry);
+ rc = pid;
+
+ out:
+ perhaps_removehandler(CTX);
+ CTX_UNLOCK;
+ return rc;
+}
+
+void libxl_childproc_setmode(libxl_ctx *ctx, const libxl_childproc_hooks *hooks,
+ void *user)
+{
+ GC_INIT(ctx);
+ CTX_LOCK;
+
+ assert(LIBXL_LIST_EMPTY(&CTX->children));
+
+ if (!hooks)
+ hooks = &libxl__childproc_default_hooks;
+
+ ctx->childproc_hooks = hooks;
+ ctx->childproc_user = user;
+
+ switch (ctx->childproc_hooks->chldowner) {
+ case libxl_sigchld_owner_mainloop:
+ case libxl_sigchld_owner_libxl:
+ libxl__sigchld_removehandler(ctx);
+ break;
+ case libxl_sigchld_owner_libxl_always:
+ libxl__sigchld_installhandler(ctx);
+ break;
+ default:
+ abort();
+ }
+
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+const libxl_childproc_hooks libxl__childproc_default_hooks = {
+ libxl_sigchld_owner_libxl, 0, 0
+};
+
+/*
* Local variables:
* mode: C
* c-basic-offset: 4
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3d0379e..09bfd27 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -182,6 +182,19 @@ typedef struct libxl__ev_watch_slot {
libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum);
+typedef struct libxl__ev_child libxl__ev_child;
+typedef void libxl__ev_child_callback(libxl__egc *egc, libxl__ev_child*,
+ pid_t pid, int status);
+struct libxl__ev_child {
+ /* caller should include this in their own struct */
+ /* read-only for caller: */
+ pid_t pid; /* -1 means unused ("unregistered", ie Idle) */
+ libxl__ev_child_callback *callback;
+ /* remainder is private for libxl__ev_... */
+ LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
+};
+
+
/*
* evgen structures, which are the state we use for generating
* events for the caller.
@@ -251,6 +264,8 @@ struct libxl__ctx {
const libxl_event_hooks *event_hooks;
void *event_hooks_user;
+ LIBXL_LIST_ENTRY(struct libxl__ctx);
+
pthread_mutex_t lock; /* protects data structures hanging off the ctx */
/* Always use libxl__ctx_lock and _unlock (or the convenience
* macors CTX_LOCK and CTX_UNLOCK) to manipulate this.
@@ -291,10 +306,14 @@ struct libxl__ctx {
LIBXL_LIST_HEAD(, libxl_evgen_disk_eject) disk_eject_evgens;
- /* 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 */
+ const libxl_childproc_hooks *childproc_hooks;
+ void *childproc_user;
+ int sigchld_selfpipe[2]; /* [0]==-1 means handler not installed */
+ LIBXL_LIST_HEAD(, libxl__ev_child) children;
+
+ /* This is obsolete and must be removed: */
int (*waitpid_instead)(pid_t pid, int *status, int flags);
+
libxl_version_info version_info;
};
@@ -542,6 +561,33 @@ static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
/*
+ * For making subprocesses. This is the only permitted mechanism for
+ * code in libxl to do so.
+ *
+ * In the parent, returns the pid, filling in childw_out.
+ * In the child, returns 0.
+ * If it fails, returns a libxl error (all of which are -ve).
+ *
+ * The child should go on to exec (or exit) soon, and should not make
+ * any further libxl event calls in the meantime.
+ *
+ * The parent may signal the child but it must not reap it. That will
+ * be done by the event machinery. Either death or cldstop may be
+ * NULL in which case that kind of event is ignored.
+ *
+ * It is not possible to "deregister" the child death event source.
+ * It will generate exactly one event callback; until then the childw
+ * is Active and may not be reused.
+ */
+_hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out,
+ libxl__ev_child_callback *death);
+static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
+ { childw_out->pid = -1; }
+static inline int libxl__ev_child_inuse(libxl__ev_child *childw_out)
+ { return childw_out->pid >= 0; }
+
+
+/*
* Other event-handling support provided by the libxl event core to
* the rest of libxl.
*/
@@ -595,6 +641,15 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p);
* ctx must be locked. */
void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p);
+/* Internal to fork and child reaping machinery */
+extern const libxl_childproc_hooks libxl__childproc_default_hooks;
+int libxl__sigchld_installhandler(libxl_ctx *ctx); /* non-reentrant;logs errs */
+void libxl__sigchld_removehandler(libxl_ctx *ctx); /* non-reentrant */
+int libxl__fork_selfpipe_active(libxl_ctx *ctx); /* returns read fd or -1 */
+void libxl__fork_selfpipe_woken(libxl__egc *egc);
+int libxl__self_pipe_wakeup(int fd); /* returns 0 or -1 setting errno */
+int libxl__self_pipe_eatall(int fd); /* returns 0 or -1 setting errno */
+
int libxl__atfork_init(libxl_ctx *ctx);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 14/15] libxl: Provide libxl_string_list_length
2012-02-24 18:54 (no subject) Ian Jackson
` (12 preceding siblings ...)
2012-02-24 18:55 ` [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
@ 2012-02-24 18:55 ` Ian Jackson
2012-02-24 18:55 ` [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
2012-02-24 18:57 ` [PATCH v2 00/15] libxl: child process handling Ian Jackson
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:55 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Signed-off-by: Ian Jackson <ian.jackson@eu.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 06ac064..42bc7fc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -195,6 +195,14 @@ void libxl_string_list_dispose(libxl_string_list *psl)
free(sl);
}
+int libxl_string_list_length(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 65961d2..dcfbd32 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -154,6 +154,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(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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds
2012-02-24 18:54 (no subject) Ian Jackson
` (13 preceding siblings ...)
2012-02-24 18:55 ` [PATCH 14/15] libxl: Provide libxl_string_list_length Ian Jackson
@ 2012-02-24 18:55 ` Ian Jackson
2012-02-24 18:57 ` [PATCH v2 00/15] libxl: child process handling Ian Jackson
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:55 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
We will want to reuse the fd-sending code, so break it out into its
own function, and provide the corresponding sending function.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_internal.h | 11 ++++
tools/libxl/libxl_qmp.c | 31 ++-----------
tools/libxl/libxl_utils.c | 104 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+), 27 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 09bfd27..1762bf8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1083,6 +1083,17 @@ _hidden void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid);
_hidden int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
const libxl_domain_config *guest_config);
+/* on failure, logs */
+int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
+ const void *data, size_t datalen,
+ int nfds, const int fds[], const char *what);
+
+/* Insists on receiving exactly nfds and datalen. On failure, logs
+ * and leaves *fds untouched. */
+int libxl__recvmsg_fds(libxl__gc *gc, int carrier,
+ void *databuf, size_t datalen,
+ int nfds, int fds[], const char *what);
+
/* from libxl_json */
#include <yajl/yajl_gen.h>
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 43fd134..e346e05 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -544,38 +544,15 @@ static int qmp_send_fd(libxl__gc *gc, libxl__qmp_handler *qmp,
qmp_request_context *context,
int fd)
{
- struct msghdr msg = { 0 };
- struct cmsghdr *cmsg;
- char control[CMSG_SPACE(sizeof (fd))];
- struct iovec iov;
char *buf = NULL;
+ int rc;
buf = qmp_send_prepare(gc, qmp, "getfd", args, callback, opaque, context);
- /* Response data */
- iov.iov_base = buf;
- iov.iov_len = strlen(buf);
-
- /* compose the message */
- msg.msg_iov = &iov;
- msg.msg_iovlen = 1;
- msg.msg_control = control;
- msg.msg_controllen = sizeof (control);
-
- /* attach open fd */
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof (fd));
- *(int *)CMSG_DATA(cmsg) = fd;
-
- msg.msg_controllen = cmsg->cmsg_len;
+ rc = libxl__sendmsg_fds(gc, qmp->qmp_fd, buf, strlen(buf), 1, &fd,
+ "QMP message to QEMU");
+ if (rc) return rc;
- if (sendmsg(qmp->qmp_fd, &msg, 0) < 0) {
- LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
- "Failed to send a QMP message to QEMU.");
- return ERROR_FAIL;
- }
if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
"CRLF", "QMP socket")) {
return ERROR_FAIL;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 9f9f91d..d2efa5c 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -534,6 +534,110 @@ void libxl_cputopology_list_free(libxl_cputopology *list, int nr)
free(list);
}
+int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
+ const void *data, size_t datalen,
+ int nfds, const int fds[], const char *what) {
+ struct msghdr msg = { 0 };
+ struct cmsghdr *cmsg;
+ size_t spaceneeded = nfds * sizeof(fds[0]);
+ char control[CMSG_SPACE(spaceneeded)];
+ struct iovec iov;
+ int r;
+
+ iov.iov_base = (void*)data;
+ iov.iov_len = datalen;
+
+ /* compose the message */
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ /* attach open fd */
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(spaceneeded);
+ memcpy(CMSG_DATA(cmsg), fds, spaceneeded);
+
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ r = sendmsg(carrier, &msg, 0);
+ if (r < 0) {
+ LOGE(ERROR, "failed to send fd-carrying message (%s)", what);
+ return ERROR_FAIL;
+ }
+
+ return 0;
+}
+
+int libxl__recvmsg_fds(libxl__gc *gc, int carrier,
+ void *databuf, size_t datalen,
+ int nfds, int fds[], const char *what)
+{
+ struct msghdr msg = { 0 };
+ struct cmsghdr *cmsg;
+ size_t spaceneeded = nfds * sizeof(fds[0]);
+ char control[CMSG_SPACE(spaceneeded)];
+ struct iovec iov;
+ int r;
+
+ iov.iov_base = databuf;
+ iov.iov_len = datalen;
+
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ for (;;) {
+ r = recvmsg(carrier, &msg, 0);
+ if (r < 0) {
+ if (errno == EINTR) continue;
+ if (errno == EWOULDBLOCK) return -1;
+ LOGE(ERROR,"recvmsg failed (%s)", what);
+ return ERROR_FAIL;
+ }
+ if (r == 0) {
+ LOG(ERROR,"recvmsg got EOF (%s)", what);
+ return ERROR_FAIL;
+ }
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg->cmsg_len <= CMSG_LEN(0)) {
+ LOG(ERROR,"recvmsg got no control msg"
+ " when expecting fds (%s)", what);
+ return ERROR_FAIL;
+ }
+ if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) {
+ LOG(ERROR, "recvmsg got unexpected"
+ " cmsg_level %d (!=%d) or _type %d (!=%d) (%s)",
+ cmsg->cmsg_level, SOL_SOCKET,
+ cmsg->cmsg_type, SCM_RIGHTS,
+ what);
+ return ERROR_FAIL;
+ }
+ if (cmsg->cmsg_len != CMSG_LEN(spaceneeded) ||
+ msg.msg_controllen != cmsg->cmsg_len) {
+ LOG(ERROR, "recvmsg got unexpected"
+ " number of fds or extra control data"
+ " (%ld bytes' worth, expected %ld) (%s)",
+ (long)CMSG_LEN(spaceneeded), (long)cmsg->cmsg_len,
+ what);
+ int i, fd;
+ unsigned char *p;
+ for (i=0, p=CMSG_DATA(cmsg);
+ CMSG_SPACE(i * sizeof(fds[0]));
+ i++, i+=sizeof(fd)) {
+ memcpy(&fd, p, sizeof(fd));
+ close(fd);
+ }
+ return ERROR_FAIL;
+ }
+ memcpy(fds, CMSG_DATA(cmsg), spaceneeded);
+ return 0;
+ }
+}
+
/*
* Local variables:
* mode: C
--
1.7.2.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 00/15] libxl: child process handling
2012-02-24 18:54 (no subject) Ian Jackson
` (14 preceding siblings ...)
2012-02-24 18:55 ` [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
@ 2012-02-24 18:57 ` Ian Jackson
15 siblings, 0 replies; 17+ messages in thread
From: Ian Jackson @ 2012-02-24 18:57 UTC (permalink / raw)
To: xen-devel
Ian Jackson writes (""):
> Once again, I have not executed the code in this series!
The subject line in my original 00/15 mail seems to have been lost.
Sorry.
Also, it seems that a glitch in my patch management system has lost
some acks that I believe may have been posted. Sorry; I didn't
intentionally remove any acks due to code change, so you can feel
confident just to re-ack without re-reading.
Ian.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-02-24 18:57 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-24 18:54 (no subject) Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
2012-02-24 18:54 ` [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove Ian Jackson
2012-02-24 18:54 ` [PATCH 03/15] libxl: Fix eventloop_iteration over-locking Ian Jackson
2012-02-24 18:54 ` [PATCH 04/15] libxl: Fix leak of ctx->lock Ian Jackson
2012-02-24 18:54 ` [PATCH 05/15] libxl: abolish libxl_ctx_postfork Ian Jackson
2012-02-24 18:54 ` [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk Ian Jackson
2012-02-24 18:54 ` [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS Ian Jackson
2012-02-24 18:54 ` [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure Ian Jackson
2012-02-24 18:54 ` [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc Ian Jackson
2012-02-24 18:54 ` [PATCH 10/15] libxl: Introduce some convenience macros Ian Jackson
2012-02-24 18:54 ` [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads Ian Jackson
2012-02-24 18:55 ` [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS Ian Jackson
2012-02-24 18:55 ` [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
2012-02-24 18:55 ` [PATCH 14/15] libxl: Provide libxl_string_list_length Ian Jackson
2012-02-24 18:55 ` [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
2012-02-24 18:57 ` [PATCH v2 00/15] libxl: child process handling Ian Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).