Re: [PATCH 3/3] libxl: Domain destroy: fork

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-03-17 at 09:30 -0600, Jim Fehlig wrote:
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Call xc_domain_destroy in a subprocess.  That allows us to do so
> asynchronously, rather than blocking the whole process calling libxl.
> 
> The changes in detail:
> 
>  * Provide an libxl__ev_child in libxl__domain_destroy_state, and
>    initialise it in libxl__domain_destroy.  There is no possibility
>    to `clean up' a libxl__ev_child, but there need to clean it up, as
                                               ^is no ?

>    the control flow ensures that we only continue after the child has
>    exited.
> 
>  * Call libxl__ev_child_fork at the right point and put the call to
>    xc_domain_destroy and associated logging in the child.  (The child
>    opens a new xenctrl handle because we mustn't use the parent's.)

Do I read it correctly that this is arranging to use the same logger as
the parent was, IOW the log messages will still go somewhere the
application considers useful?

> 
>  * Consequently, the success return path from domain_destroy_domid_cb
>    no longer calls dis->callback.  Instead it simply returns.
> 
>  * We plumb the errorno value through the child's exit status, if it
>    fits.  This means we normally do the logging only in the parent.

(This makes my question above moot I think).

>  * Incidentally, we fix the bug that we were treating the return value
>    from xc_domain_destroy as an errno value when in fact it is a
>    return value from do_domctl (in this case, 0 or -1 setting errno).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>
> Tested-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  tools/libxl/libxl.c          | 57 ++++++++++++++++++++++++++++++++++++++++----
>  tools/libxl/libxl_internal.h |  1 +
>  2 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b6541d4..b43db1a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1481,6 +1481,10 @@ static void domain_destroy_callback(libxl__egc *egc,
>  static void destroy_finish_check(libxl__egc *egc,
>                                   libxl__domain_destroy_state *dds);
>  
> +static void domain_destroy_domid_cb(libxl__egc *egc,
> +                                    libxl__ev_child *destroyer,
> +                                    pid_t pid, int status);

This seems misplaced, doesn't it want to go before libxl__destroy_domid
just after the prototype of devices_destroy_cb, which is where it lives
in the sequence?

Ian.