All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace
@ 2020-01-09 12:12 George Dunlap
  2020-01-09 12:12 ` [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode George Dunlap
  2020-01-10 17:57 ` [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace Ian Jackson
  0 siblings, 2 replies; 6+ messages in thread
From: George Dunlap @ 2020-01-09 12:12 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony Perard, Ian Jackson, George Dunlap, Wei Liu

No functional changes.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 tools/libxl/libxl_event.h    | 2 +-
 tools/libxl/libxl_fork.c     | 4 ++--
 tools/libxl/libxl_internal.h | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index d1517f7456..41082ef2f4 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -441,7 +441,7 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
  *
  *     A program which does this must call libxl_childproc_setmode.
  *     There are three options:
- * 
+ *
  *     libxl_sigchld_owner_libxl:
  *
  *       While any libxl operation which might use child processes
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 0f1b6b518c..c5b378c554 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -78,7 +78,7 @@ static void atfork_unlock(void)
 int libxl__atfork_init(libxl_ctx *ctx)
 {
     int r, rc;
-    
+
     atfork_lock();
     if (atfork_registered) { rc = 0; goto out; }
 
@@ -326,7 +326,7 @@ static void sigchld_removehandler_core(void) /* idempotent */
 {
     struct sigaction was;
     int r;
-    
+
     if (!sigchld_installed)
         return;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b5adbfe4b7..bfbee9451e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -161,7 +161,7 @@
 #endif
   /* all of these macros preserve errno (saving and restoring) */
 
-/* 
+/*
  * A macro to help retain the first failure in "do as much as you can"
  * situations.  Note the hard-coded use of the variable name `rc`.
  */
@@ -1367,7 +1367,7 @@ typedef struct {
     const char *shim_cmdline;
     const char *pv_cmdline;
 
-    /* 
+    /*
      * dm_runas: If set, pass qemu the `-runas` parameter with this
      *  string as an argument
      * dm_kill_uid: If set, the devicemodel should be killed by
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode
  2020-01-09 12:12 [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace George Dunlap
@ 2020-01-09 12:12 ` George Dunlap
  2020-01-09 17:07   ` George Dunlap
  2020-01-10 17:58   ` George Dunlap
  2020-01-10 17:57 ` [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace Ian Jackson
  1 sibling, 2 replies; 6+ messages in thread
From: George Dunlap @ 2020-01-09 12:12 UTC (permalink / raw)
  To: xen-devel
  Cc: Anthony Perard, Ian Jackson, George Dunlap, Wei Liu, Nick Rosbrook

libxl needs to be able to know when processes it forks have completed.

At the moment, libxl has two basic mode (with some variations).  In
one mode -- libxl_sigchld_owner_libxl* -- libxl sets up its own
SIGCHLD signal handler, and also handles the event loop that allows
libxl to safely block until the child in question is finished (using a
self-pipe for the SIGCHLD handler to notify the waiters that it's time
to look for reaped children).

In the other mode, libxl does not set up the SIGCHLD handler, nor does
it do anything with processing the event loop; it expects the library
caller to handle the event loop itself.

The golang runtime manages its own processes, and thus must use
SIGCHLD itself; and it has an easy way for other users to get SIGCHLD
notifications.  However, because its event loop is hidden away behind
abstractions, it's not easy to hook into; and there's no need -- the
golang runtime assumes that C function calls may block, and handles
everything behind the scenes.

Introduce a new mode, libxl_sigchld_owner_notify, in which libxl sets
up the SIGCHLD event handling machinery, but relies on the caller to
tell it when a SIGCHLD happens.

Call these two actions "notify" (for the self-pipe notification
machinery) and "handler" (for the actual SIGCHLD handler).

Provide a new external function, libxl_childproc_sigchld_notify(), for
library users to call when a SIGCHLD happens.  Have this call
sigchld_handler().

Rename chldmode_ours() to chldmode_notify(), and use it to determine
whether to set up the notification chain.

When setting up the notification chain, do everything except setting
up the signal handler in "notify-only" mode.

defer_sigchld() and release_sigchld() do two things: they modify the
signal handler, and grab and release locks.  Refactor these so that
they grab and release the locks correctly in "notify-only" mode, but
don't tweak the signal handler unless it's been set up.

With the golang bindings ported to use this change, domain creation
works.

NB an alternate approach would be to make libxl_sigchld_owner_mainloop
*always* set up and tear down the self-pipe notification mechanisms,
and then simply expose libxl_childproc_sigchld_notify().  However,
this would entail grabbing a libxl-wide global lock (across all libxl
ctx's) twice on every fork.  This should be avoided for callers which
don't need it.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- Make libxl_childproc_sigchld_notify() reentrant by simply calling
  sigchld_handler directly.  Mention this in the documentation to prompt
  people to think about saving and restoring errno themselves.
- Move whitespace fixes to another patch
- Make braces snuggly in line with libxl coding style.
- Remove XXX comment.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/libxl/libxl_event.h | 31 +++++++++++++++++++++++++++++
 tools/libxl/libxl_fork.c  | 42 +++++++++++++++++++++++++++------------
 2 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index 41082ef2f4..d6857d452e 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -466,6 +466,14 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl)
  *       has multiple libxl ctx's, it must call libxl_childproc_exited
  *       on each ctx.)
  *
+ *     libxl_sigchld_owner_mainloop_notify:
+ *
+ *       The application must install a SIGCHLD handler, but will
+ *       notify libxl when a sigchld happened by calling
+ *       libxl_childproc_sigchld_notify.  libxl will arrange to reap
+ *       only its own children.  This needs to be called only once,
+ *       even for applications which have multiple libxl ctx's.
+ *
  *     libxl_sigchld_owner_libxl_always:
  *
  *       The application expects this libxl ctx to reap all of the
@@ -494,6 +502,12 @@ typedef enum {
      * arrange to (un)block or catch SIGCHLD. */
     libxl_sigchld_owner_mainloop,
 
+    /* Application promises to discover when SIGCHLD occurs and call
+     * libxl_childproc_sigchld_notify (perhaps from within a signal
+     * handler).  libxl will not itself arrange to (un)block or catch
+     * SIGCHLD. */
+    libxl_sigchld_owner_mainloop_notify,
+
     /* libxl owns SIGCHLD all the time, and the application is
      * relying on libxl's event loop for reaping its children too. */
     libxl_sigchld_owner_libxl_always,
@@ -590,6 +604,23 @@ int libxl_childproc_reaped(libxl_ctx *ctx, pid_t, int status)
 void libxl_childproc_sigchld_occurred(libxl_ctx *ctx)
                            LIBXL_EXTERNAL_CALLERS_ONLY;
 
+/*
+ * This function is for an application which owns SIGCHLD but still
+ * expects libxl to handle its own event loops.
+ *
+ * Such an the application must notify libxl, by calling this
+ * function, that a SIGCHLD occurred.  libxl will then notify all
+ * appropriate event loops to check for reaped children..
+ *
+ * May be called only by an application which has called setmode with
+ * chldowner == libxl_sigchld_owner_mainloop_notify.
+ *
+ * MAY be called from within a signal handler.  This function is
+ * reentrang: it will save and restore errno.
+ */
+void libxl_childproc_sigchld_notify(void)
+                           LIBXL_EXTERNAL_CALLERS_ONLY;
+
 
 /*
  * An application which initialises a libxl_ctx in a parent process
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index c5b378c554..7f21bc9bb2 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -248,6 +248,11 @@ static void sigchld_handler(int signo)
     errno = esave;
 }
 
+void libxl_childproc_sigchld_notify(void)
+{
+    sigchld_handler(SIGCHLD);
+}
+
 static void sigchld_sethandler_raw(void (*handler)(int), struct sigaction *old)
 {
     struct sigaction ours;
@@ -288,9 +293,8 @@ static void sigchld_handler_when_deferred(int signo)
 
 static void defer_sigchld(void)
 {
-    assert(sigchld_installed);
-
-    sigchld_sethandler_raw(sigchld_handler_when_deferred, 0);
+    if (sigchld_installed)
+        sigchld_sethandler_raw(sigchld_handler_when_deferred, 0);
 
     /* Now _this thread_ cannot any longer be interrupted by the
      * signal, so we can take the mutex without risk of deadlock.  If
@@ -303,12 +307,12 @@ static void defer_sigchld(void)
 
 static void release_sigchld(void)
 {
-    assert(sigchld_installed);
-
     int r = pthread_mutex_unlock(&sigchld_defer_mutex);
     assert(!r);
 
-    sigchld_sethandler_raw(sigchld_handler, 0);
+    if (sigchld_installed)
+        sigchld_sethandler_raw(sigchld_handler, 0);
+
     if (sigchld_occurred_while_deferred) {
         sigchld_occurred_while_deferred = 0;
         /* We might get another SIGCHLD here, in which case
@@ -375,6 +379,11 @@ static void sigchld_user_remove(libxl_ctx *ctx) /* idempotent */
 
 void libxl__sigchld_notneeded(libxl__gc *gc) /* non-reentrant, idempotent */
 {
+    /*
+     * NB that we don't need to special-case
+     * libxl_sigchld_owner_mainloop_notify; sigchld_removehandler_core
+     * will DTRT if no signal handler has been set up.
+     */
     sigchld_user_remove(CTX);
     libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
 }
@@ -399,7 +408,9 @@ int libxl__sigchld_needed(libxl__gc *gc) /* non-reentrant, idempotent */
     if (!CTX->sigchld_user_registered) {
         atfork_lock();
 
-        sigchld_installhandler_core();
+        if (CTX->childproc_hooks->chldowner != libxl_sigchld_owner_mainloop_notify) {
+            sigchld_installhandler_core();
+        }
 
         defer_sigchld();
 
@@ -416,13 +427,15 @@ int libxl__sigchld_needed(libxl__gc *gc) /* non-reentrant, idempotent */
     return rc;
 }
 
-static bool chldmode_ours(libxl_ctx *ctx, bool creating)
+/* Do we need the sigchld notification machinery? */
+static bool chldmode_notify(libxl_ctx *ctx, bool creating)
 {
     switch (ctx->childproc_hooks->chldowner) {
     case libxl_sigchld_owner_libxl:
         return creating || !LIBXL_LIST_EMPTY(&ctx->children);
     case libxl_sigchld_owner_mainloop:
         return 0;
+    case libxl_sigchld_owner_mainloop_notify:
     case libxl_sigchld_owner_libxl_always:
     case libxl_sigchld_owner_libxl_always_selective_reap:
         return 1;
@@ -432,7 +445,7 @@ static bool chldmode_ours(libxl_ctx *ctx, bool creating)
 
 static void perhaps_sigchld_notneeded(libxl__gc *gc)
 {
-    if (!chldmode_ours(CTX, 0))
+    if (!chldmode_notify(CTX, 0))
         libxl__sigchld_notneeded(gc);
 }
 
@@ -440,7 +453,7 @@ static int perhaps_sigchld_needed(libxl__gc *gc, bool creating)
 {
     int rc;
 
-    if (chldmode_ours(CTX, creating)) {
+    if (chldmode_notify(CTX, creating)) {
         rc = libxl__sigchld_needed(gc);
         if (rc) return rc;
     }
@@ -556,13 +569,16 @@ static void sigchld_selfpipe_handler(libxl__egc *egc, libxl__ev_fd *ev,
     int e = libxl__self_pipe_eatall(selfpipe);
     if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
 
-    if (CTX->childproc_hooks->chldowner
-        == libxl_sigchld_owner_libxl_always_selective_reap) {
+    switch (CTX->childproc_hooks->chldowner) {
+    case libxl_sigchld_owner_libxl_always_selective_reap:
+    case libxl_sigchld_owner_mainloop_notify:
         childproc_checkall(egc);
         return;
+    default:
+        ;
     }
 
-    while (chldmode_ours(CTX, 0) /* in case the app changes the mode */) {
+    while (chldmode_notify(CTX, 0) /* in case the app changes the mode */) {
         int status;
         pid_t pid = checked_waitpid(egc, -1, &status);
 
-- 
2.24.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode
  2020-01-09 12:12 ` [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode George Dunlap
@ 2020-01-09 17:07   ` George Dunlap
  2020-01-10 17:58   ` George Dunlap
  1 sibling, 0 replies; 6+ messages in thread
From: George Dunlap @ 2020-01-09 17:07 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony Perard, Ian Jackson, Nick Rosbrook, Wei Liu

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On 1/9/20 12:12 PM, George Dunlap wrote:
> libxl needs to be able to know when processes it forks have completed.
> 
> At the moment, libxl has two basic mode (with some variations).  In
> one mode -- libxl_sigchld_owner_libxl* -- libxl sets up its own
> SIGCHLD signal handler, and also handles the event loop that allows
> libxl to safely block until the child in question is finished (using a
> self-pipe for the SIGCHLD handler to notify the waiters that it's time
> to look for reaped children).
> 
> In the other mode, libxl does not set up the SIGCHLD handler, nor does
> it do anything with processing the event loop; it expects the library
> caller to handle the event loop itself.
> 
> The golang runtime manages its own processes, and thus must use
> SIGCHLD itself; and it has an easy way for other users to get SIGCHLD
> notifications.  However, because its event loop is hidden away behind
> abstractions, it's not easy to hook into; and there's no need -- the
> golang runtime assumes that C function calls may block, and handles
> everything behind the scenes.

FWIW, attached is a C program that behaves as I think golang would
behave, which hangs in `mainloop` mode.

It's actually got two modes: Run it without any arguments, and it will
run in "default" mode (not setting up a SIGCHLD nor setting
childproc_mode);  run with at least one argument (whatever it is), it
will run in `mainloop` mode, and hang after the second SIGCHLD event.

 -George

[-- Attachment #2: xltest_sigchld_main.c --]
[-- Type: text/x-csrc, Size: 5002 bytes --]

#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <libxl.h>
#include <assert.h>
#include <pthread.h>
#include <poll.h>
#include <signal.h>
#include <fcntl.h>

char * malloc_copy(const char *src) {
  int len = strlen(src);
  char * dst = malloc(strlen(src)+1);

  strncpy(dst, src, len);

  return dst;
}

libxl_ctx *ctx;

int self_pipe_wakeup(int fd)
{
    /* Called from signal handlers, so needs to be async-signal-safe */
    static const char buf[1] = "";

    for (;;) {
        int r = write(fd, buf, 1);
        if (r==1) return 0;
        assert(r==-1);
        if (errno == EINTR) continue;
        if (errno == EWOULDBLOCK) return 0;
        if (!errno) abort();
        return errno;
    }
}

int self_pipe_eatall(int fd)
{
    char buf[256];
    for (;;) {
        int r = read(fd, buf, sizeof(buf));
        if (r == sizeof(buf)) continue;
        if (r >= 0) return 0;
        assert(r == -1);
        if (errno == EINTR) continue;
        if (errno == EWOULDBLOCK) return 0;
        assert(errno);
        return errno;
    }
}

int sigchld_selfpipe[2];

static void *sigchld_helper(void *fd) {
    struct pollfd pfd;
    int r;

    pfd.fd = (int)(unsigned long)fd;
    pfd.events = POLLIN | POLLHUP;

    while (true) {
        // Wait for a sigchld on [0]
        fprintf(stderr, "Waiting for self-pipe\n");
        r = poll(&pfd, 1, -1);
        if (r < 0) {
            if (errno == EINTR)
                continue;
            perror("poll");
            return NULL;
        }
        if (pfd.revents == POLLHUP) {
            fprintf(stderr, "Received POLLHUP, closing helper\n");
            return NULL;
        }
        fprintf(stderr, "Self-pipe received, calling libxl_childproc_sigchld_occurred\n");
        self_pipe_eatall(pfd.fd);
        libxl_childproc_sigchld_occurred(ctx);
    }

    return NULL;
}

static void sigchld_handler(int signo) {
    // Signal on pipe [1]
    fprintf(stderr, "SIGCHLD received, self-signaling\n");
    self_pipe_wakeup(sigchld_selfpipe[1]);
}

void setup_sigchld(void) {
    struct sigaction ours, old;
    pthread_t t;
    int i, flags, r;

    // Get self-signal pipe
    if (pipe(sigchld_selfpipe) < 0) {
        perror("Failed to create a pipe");
        exit(1);
    }

    r = pthread_create(&t, NULL, sigchld_helper, (void *)(unsigned long)sigchld_selfpipe[0]);
    if (r < 0) {
        perror("Creating helper thread");
        exit(1);
    }

    // Make non-blocking
    for (i = 0; i < 2; i++) {
        int fd = sigchld_selfpipe[i];
        flags = fcntl(fd, F_GETFL);
        if (flags == -1) {
            perror("Getting block flags");
            exit(1);
        }

        flags |= O_NONBLOCK;

        r = fcntl(fd, F_SETFL, flags);
        if (flags == -1) {
            perror("Setting pipe non-blocking");
            exit(1);
        }
    }

    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, &old);
    assert(!r);
}

int main(int argc, char * argv[]) {
    int rc;
    xentoollog_logger_stdiostream * xtl;
    libxl_domain_config cconfig;
    uint32_t domid;
    bool mainloop = false;

    if (argc > 1)
        mainloop=true;

    if (mainloop)
        setup_sigchld();

    xtl = xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0);
    rc = libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)xtl);
    if (rc) {
        fprintf(stderr, "Getting libxl_ctx: %d", rc);
        exit(1);
    }

    if (mainloop) {
        libxl_childproc_hooks *cp = malloc(sizeof(*cp));

        cp->chldowner = libxl_sigchld_owner_mainloop;
        libxl_childproc_setmode(ctx, cp, NULL);
    }

    libxl_domain_config_init(&cconfig);
    libxl_domain_build_info_init_type(&cconfig.b_info, LIBXL_DOMAIN_TYPE_PV);

    cconfig.c_info.type = LIBXL_DOMAIN_TYPE_PV;
    cconfig.c_info.name = malloc_copy("c6-01");
    cconfig.b_info.max_vcpus = 4;
    cconfig.b_info.max_memkb = 2048*1024;
    cconfig.b_info.target_memkb = 2048*1024;
    cconfig.on_crash = LIBXL_ACTION_ON_SHUTDOWN_DESTROY;
    cconfig.b_info.bootloader = malloc_copy("pygrub");

    cconfig.num_disks = 1;
    cconfig.disks = malloc(sizeof(libxl_device_disk));
    libxl_device_disk_init(cconfig.disks);
    cconfig.disks[0].vdev = malloc_copy("hda");
    cconfig.disks[0].format = LIBXL_DISK_FORMAT_RAW;
    cconfig.disks[0].pdev_path = malloc_copy("/images/c6-01.raw");
    cconfig.disks[0].readwrite = 1;

    cconfig.num_nics = 1;
    cconfig.nics = malloc(sizeof(libxl_device_nic));
    libxl_device_nic_init(cconfig.nics);

    rc = libxl_domain_create_new(ctx, &cconfig, &domid, NULL, NULL);
    if (rc) {
        fprintf(stderr, "Creating domain: %d", rc);
        exit(1);
    }

    printf("Created domain with id %d", domid);

    libxl_domain_config_dispose(&cconfig);

    if (mainloop) {
        close(sigchld_selfpipe[1]);
        close(sigchld_selfpipe[0]);
    }

    return 0;
}

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace
  2020-01-09 12:12 [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace George Dunlap
  2020-01-09 12:12 ` [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode George Dunlap
@ 2020-01-10 17:57 ` Ian Jackson
  1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2020-01-10 17:57 UTC (permalink / raw)
  To: George Dunlap; +Cc: Anthony Perard, xen-devel, Wei Liu

George Dunlap writes ("[PATCH v2 1/2] libxl: Get rid of some trailing whitespace"):
> No functional changes.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode
  2020-01-09 12:12 ` [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode George Dunlap
  2020-01-09 17:07   ` George Dunlap
@ 2020-01-10 17:58   ` George Dunlap
  2020-01-13 14:20     ` Ian Jackson
  1 sibling, 1 reply; 6+ messages in thread
From: George Dunlap @ 2020-01-10 17:58 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony Perard, Ian Jackson, Nick Rosbrook, Wei Liu

On 1/9/20 12:12 PM, George Dunlap wrote:
> libxl needs to be able to know when processes it forks have completed.
> 
> At the moment, libxl has two basic mode (with some variations).  In
> one mode -- libxl_sigchld_owner_libxl* -- libxl sets up its own
> SIGCHLD signal handler, and also handles the event loop that allows
> libxl to safely block until the child in question is finished (using a
> self-pipe for the SIGCHLD handler to notify the waiters that it's time
> to look for reaped children).
> 
> In the other mode, libxl does not set up the SIGCHLD handler, nor does
> it do anything with processing the event loop; it expects the library
> caller to handle the event loop itself.
> 
> The golang runtime manages its own processes, and thus must use
> SIGCHLD itself; and it has an easy way for other users to get SIGCHLD
> notifications.  However, because its event loop is hidden away behind
> abstractions, it's not easy to hook into; and there's no need -- the
> golang runtime assumes that C function calls may block, and handles
> everything behind the scenes.
> 
> Introduce a new mode, libxl_sigchld_owner_notify, in which libxl sets
> up the SIGCHLD event handling machinery, but relies on the caller to
> tell it when a SIGCHLD happens.
> 
> Call these two actions "notify" (for the self-pipe notification
> machinery) and "handler" (for the actual SIGCHLD handler).
> 
> Provide a new external function, libxl_childproc_sigchld_notify(), for
> library users to call when a SIGCHLD happens.  Have this call
> sigchld_handler().
> 
> Rename chldmode_ours() to chldmode_notify(), and use it to determine
> whether to set up the notification chain.
> 
> When setting up the notification chain, do everything except setting
> up the signal handler in "notify-only" mode.
> 
> defer_sigchld() and release_sigchld() do two things: they modify the
> signal handler, and grab and release locks.  Refactor these so that
> they grab and release the locks correctly in "notify-only" mode, but
> don't tweak the signal handler unless it's been set up.
> 
> With the golang bindings ported to use this change, domain creation
> works.
> 
> NB an alternate approach would be to make libxl_sigchld_owner_mainloop
> *always* set up and tear down the self-pipe notification mechanisms,
> and then simply expose libxl_childproc_sigchld_notify().  However,
> this would entail grabbing a libxl-wide global lock (across all libxl
> ctx's) twice on every fork.  This should be avoided for callers which
> don't need it.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

FAOD, with the fixes in your other series, I consider this patch to now
be moot.

 - George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode
  2020-01-10 17:58   ` George Dunlap
@ 2020-01-13 14:20     ` Ian Jackson
  0 siblings, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2020-01-13 14:20 UTC (permalink / raw)
  To: George Dunlap; +Cc: Anthony Perard, xen-devel, Nick Rosbrook, Wei Liu

George Dunlap writes ("Re: [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode"):
> FAOD, with the fixes in your other series, I consider this patch to now
> be moot.

Right.  FTOAD, I don't think there was a problem with this patch
principle; it's just not needed now.  If someone comes up with a use
for it in the future then it can be resurrected and we should do a
detailed code review of it then.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-01-13 14:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09 12:12 [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace George Dunlap
2020-01-09 12:12 ` [Xen-devel] [PATCH v2 2/2] libxl: Add new "notify-only" childproc mode George Dunlap
2020-01-09 17:07   ` George Dunlap
2020-01-10 17:58   ` George Dunlap
2020-01-13 14:20     ` Ian Jackson
2020-01-10 17:57 ` [Xen-devel] [PATCH v2 1/2] libxl: Get rid of some trailing whitespace Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.