Re: [Qemu-devel] [PULL 6/7] monitor: move init global earlier

[Wolfgang Bumiller]

On Mon, Jun 18, 2018 at 04:08:53PM +0200, Markus Armbruster wrote:
> From: Peter Xu <address@hidden>
> 
> Before this patch, monitor fd helpers might be called even earlier than
> monitor_init_globals().  This can be problematic.
> 
> After previous work, now monitor_init_globals() does not depend on
> accelerator initialization any more.  Call it earlier (before CLI
> parsing; that's where the monitor APIs might be called) to make sure it
> is called before any of the monitor APIs.

It looks like this patch moves the creation of the 'IO mon_iothread' to
before the call to os_daemonize().
With qemu 3.0 I'm experiencing crashes when shutting down daemonized
qemu instances, at pthread_join() on the 'IO mon_iothread' thread.

monitor_init_globals() calls monitor_iothread_init(), which in turn uses
iothread_create() to create a joinable thread. Then os_daemonize() is
called after which the thread is no longer "ours".
Debug-writing the thread IDs at pthread_create() and pthread_join()
together with the 'detachable' flag revealed that the mon_iothread ID is
already being reused, in my trace for a detached thread, and the join
then ends with a coredump and ugly log output.

> Suggested-by: Markus Armbruster <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  vl.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 6e34fb348d..b3426e03d0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2978,6 +2978,7 @@ int main(int argc, char **argv, char **envp)
>  
>      runstate_init();
>      postcopy_infrastructure_init();
> +    monitor_init_globals();
>  
>      if (qcrypto_init(&err) < 0) {
>          error_reportf_err(err, "cannot initialize crypto: ");
> @@ -4412,12 +4413,6 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>  
> -    /*
> -     * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
> -     * depends on configure_accelerator() above.
> -     */
> -    monitor_init_globals();
> -
>      if (qemu_opts_foreach(qemu_find_opts("mon"),
>                            mon_init_func, NULL, NULL)) {
>          exit(1);
> -- 
> 2.17.1