Re: [Xen-devel] [PATCH 17/19] libxl: do not leak spawned middle children

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-06-08 at 18:34 +0100, Ian Jackson wrote:
> libxl__spawn_spawn would, when libxl__spawn_detach was called, make
> the spawn become idle immediately.  However it still has a child
> process which needs to be waited for: the `detachable' spawned
> child.
> 
> This is wrong because the ultimate in-libxl caller may return to the
> application, with a child process still forked but not reaped libxl
> contrary to the documented behaviour of libxl.
> 
> Instead, replace libxl__spawn_detach with libxl__spawn_initiate_detach
> which is asynchronous.  The detachable spawned children are abolished;
> instead, we defer calling back to the in-libxl user until the middle
> child has been reaped.
> 
> Also, remove erroneous comment suggesting that `death' callback
> parameter to libxl__ev_child_fork may be NULL.  It may not, and there
> are no callers which pass NULL.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

(couple of tiny queries below)

[...]
> diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
> index cfa379c..bb85682 100644
> --- a/tools/libxl/libxl_exec.c
> +++ b/tools/libxl/libxl_exec.c
> @@ -238,15 +238,16 @@ err:
>  /*
>   * Full set of possible states of a libxl__spawn_state and its _detachable:
>   *
> - *               ss->        ss->        ss->    | ssd->       ssd->
> - *               timeout     xswatch     ssd     |  mid         ss
> - *  - Undefined   undef       undef       no     |  -           -
> - *  - Idle        Idle        Idle        no     |  -           -
> - *  - Active      Active      Active      yes    |  Active      yes
> - *  - Partial     Active/Idle Active/Idle maybe  |  Active/Idle yes  (if 
> exists)
> - *  - Detached    -           -           -      |  Active      no
> + *                   detaching failed  mid     timeout      xswatch
> + *  - Undefined         undef   undef   -        undef        undef
> + *  - Idle              any     any     Idle     Idle         Idle
> + *  - Attached OK       0       0       Active   Active       Active
> + *  - Attached Failed   0       1       Active   Idle         Idle
> + *  - Detaching         1       maybe   Active   Idle         Idle
> + *  - Partial           any     any     Idle     Active/Idle  Active/Idle
>   *
> - * When in state Detached, the middle process has been sent a SIGKILL.
> + * When in states Detaching or Attached Failed, the middle process has
> + * been sent a SIGKILL.
>   */
> 
>  /* Event callbacks. */
> @@ -257,19 +258,18 @@ static void spawn_timeout(libxl__egc *egc, 
> libxl__ev_time *ev,
>  static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
>                                 pid_t pid, int status);
> 
> -/* Precondition: Partial.  Results: Detached. */
> +/* Precondition: Partial.  Results: Idle. */
>  static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss);
> 
> -/* Precondition: Partial; caller has logged failure reason.
> - * Results: Caller notified of failure;
> - *  after return, ss may be completely invalid as caller may reuse it */
> -static void spawn_failed(libxl__egc *egc, libxl__spawn_state *ss);
> +/* Precondition: Attached or Detaching; caller has logged failure reason.
> + * Results: Detaching, or Attached Failing */

Is it Failing or Failed? You use Failed elsewhere.

[...]
> @@ -998,8 +997,8 @@ _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, 
> uint32_t domid);
>   *
>   * Higher-level double-fork and separate detach eg as for device models
>   *
> - * Each libxl__spawn_state is in one of the conventional states
> - *    Undefined, Idle, Active
> + * Each libxl__spawn_state is in one of these states
> + *    Undefined, Idle, Attached, Detaching

"Attached OK" and "Attached Failed" as described above aren't really
full states, just sub-states?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel