Re: [PATCH 6/7] libxl: fork: Provide ..._always_selective_reap

[Jim Fehlig <jfehlig@suse.com>]

Ian Jackson wrote:
> Applications exist which want to use libxl in an event-driven mode but
> which do not integrate child termination into their event system, but
> instead reap all their own children synchronously.
>
> In such an application libxl must own SIGCHLD but avoid reaping any
> children that don't belong to libxl.
>
> Provide libxl_sigchld_owner_libxl_always_selective_reap which has this
> behaviour.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Jim Fehlig <jfehlig@suse.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  tools/libxl/libxl_event.h |    5 +++++
>  tools/libxl/libxl_fork.c  |    7 +++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index 12e3d1f..c09e3ed 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> @@ -480,6 +480,11 @@ typedef enum {
>      /* 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,
> +
> +    /* libxl owns SIGCHLD all the time, but it must only reap its own
> +     * children.  The application will reap its own children
> +     * synchronously with waitpid, without the assistance of SIGCHLD. */
> +    libxl_sigchld_owner_libxl_always_selective_reap,
>   

Should there be some documentation in the opening comments of
"Subprocess handling"?  E.g. an entry under "For programs which run
their own children alongside libxl's:"?

BTW, it is not clear to me how to use libxl_childproc_setmode() wrt
different libxl_ctx.  Currently in the libvirt libxl driver there's a
driver-wide ctx for more host-centric operations like
libxl_get_version_info(), libxl_get_free_memory(), etc., and a
per-domain ctx for domain-specific operations.  The current doc for
libxl_childproc_setmode() says:

 * 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.                                                                  


which makes me believe setmode() should be called during initialization
of the driver, using the driver-wide ctx.  But looking at the code in
libxl__ev_child_fork(), seems a domain-specific ctx would be used, since
a domain-specific operation resulted in calling libxl__ev_child_fork().

>  } libxl_sigchld_owner;
>  
>  typedef struct {
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> index b2325e0..16e17f6 100644
> --- a/tools/libxl/libxl_fork.c
> +++ b/tools/libxl/libxl_fork.c
> @@ -268,6 +268,7 @@ static bool chldmode_ours(libxl_ctx *ctx, bool creating)
>      case libxl_sigchld_owner_mainloop:
>          return 0;
>      case libxl_sigchld_owner_libxl_always:
> +    case libxl_sigchld_owner_libxl_always_selective_reap:
>   

When calling setmode() on the driver-wide or on each domain-specific
ctx, I get an assert with this hunk

libvirtd: libxl_fork.c:241: libxl__sigchld_installhandler: Assertion
`!sigchld_owner' failed.

Calling setmode() on the driver-wide ctx, I hit the assert when starting
a domain, which has its own ctx.  When calling setmode() on the
domain-specific ctx's, I don't see any problems with only one domain
(and hence one ctx), but hit the assert on a second domain, which has
its own ctx.  So e.g. libxl_domain_create_new(dom1_ctx, ...) works, but
I hit the assert when calling libxl_domain_create_new(dom2_ctx, ...).

I don't see the assert, regardless of how I call setmode(), when
changing this hunk to

@@ -264,11 +264,11 @@ static bool chldmode_ours(libxl_ctx *ctx, bool
creating)
 {
     switch (ctx->childproc_hooks->chldowner) {
     case libxl_sigchld_owner_libxl:
+    case libxl_sigchld_owner_libxl_always_selective_reap:
         return creating || !LIBXL_LIST_EMPTY(&ctx->children);
     case libxl_sigchld_owner_mainloop:
         return 0;
     case libxl_sigchld_owner_libxl_always:
-    case libxl_sigchld_owner_libxl_always_selective_reap:
         return 1;
     }
     abort();

I'm not familiar with this aspect of libxl, so this might be complete
nonsense.  But improving the setmode() docs wrt apps that have multiple
libxl_ctx's would be helpful.

Regards,
Jim

>          return 1;
>      }
>      abort();
> @@ -398,6 +399,12 @@ 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) {
> +        childproc_checkall(egc);
> +        return;
> +    }
> +
>      while (chldmode_ours(CTX, 0) /* in case the app changes the mode */) {
>          int status;
>          pid_t pid = checked_waitpid(egc, -1, &status);
>