From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vincent Hanquez Subject: Re: Re: [PATCH] libxl: check for early failures of qemu-dm (v2) Date: Mon, 16 Nov 2009 14:52:56 +0000 Message-ID: <4B016748.8050904@eu.citrix.com> References: <19201.16668.970391.316245@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <19201.16668.970391.316245@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Jackson Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 > > (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