Re: Re: [PATCH] libxl: check for early failures of qemu-dm (v2)

[Vincent Hanquez <vincent.hanquez@eu.citrix.com>]

Ian Jackson wrote:
> This patch makes xl create check whether qemu-dm has started
> correctly, and causes it to fail immediately with appropriate errors
> if not.  There are other bugfixes too.
>
> More specifically:
>
>  * libxl_create_device_model forks twice rather than once so that the
>    process which calls libxl does not end up being the actual parent
>    of qemu.  That avoids the need for the qemu-dm process to be reaped
>    at some indefinite time in the future.
>
>  * The first fork generates an intermediate process which is
>    responsible for writing the qemu-dm pid to xenstore and then merely
>    waits to collect and report on qemu-dm's exit status during
>    startup.  New arguments to libxl_create_device_model allow the
>    preservation of its pid so that a later call can check whether the
>    startup is successful.
>   
Did you have the qemu-dm ready patch in qemu ?
>  * xl.c's create_domain checks for errors in its libxl calls.
>
> Consequential changes:
>
>  * libxl_wait_for_device_model now takes a callback function parameter
>    which is called repeatedly in the loop iteration and allows the
>    caller to abort the wait.
>
>  * libxl_exec no longer calls fork; there is a new libxl_fork.
>   
this is not libxl goal to provide wrapper for syscalls.
the exec should do the double fork+exec itself, no need to have a fork 
function.
>  * osdep.[ch] new use #define _GNU_SOURCE.  The provided asprintf
>    declarations are suppressed when not needed (currently, always).
>
>  * There is a hook to override waitpid, which will be necessary for
>    some callers.
>   
why ?

> Remaining problems and other issues I noticed or we found:
>
>  * The error handling is rather inconsistent still and lacking in
>    places.
>   
>  * xl_logv is declared but not defined.
>  * _GNU_SOURCE should be used throughout.  The asprintf implementation
>    should be disabled in favour of the system one.
>   
osdeps was just suppose to be available for netbsd. not sure why the 
contrary happens on christoph's patch.

>  * XL_LOG_ERROR_ERRNO needs to actually print the errno value.
>   
there shouldn't be a LOG_ERROR_ERRNO value. what you want is the 
different log function
that will embedd an errno automatically in the fmt, not a different 
logging level. not sure what you meant later by "remove new error level, 
should be in future patch".

>  * destroy_device_model can kill random dom0 processes (!)
>   
>  * struct libxl_ctx should be defined in libxl_internal.h.
>   
no, otherwise the init ctx need to allocate itself the memory of the 
context, instead of having
the caller "allocate it" itself on the fct stack.
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> (Changes since v1:
>  * Remove new error log level; should be in a future patch
>  * Properly fixed the asprintf problem
>  * Indentation no uses literal tabs

-- 
Vincent