Re: [Qemu-devel] [RFC 07/15] monitor: unify global init

[Eric Blake]

On 09/14/2017 02:50 AM, Peter Xu wrote:
> There are many places for monitor init its globals, at least:
> 
> - monitor_init_qmp_commands() at the very beginning
> - single function to init monitor_lock
> - in the first entry of monitor_init() using "is_first_init"
> 
> Unify them a bit.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  include/monitor/monitor.h |  2 +-
>  monitor.c                 | 25 ++++++++++---------------
>  vl.c                      |  3 ++-
>  3 files changed, 13 insertions(+), 17 deletions(-)
> 

>  
> +void monitor_init_globals(void)
> +{
> +    monitor_init_qmp_commands();
> +    monitor_qapi_event_init();
> +    sortcmdlist();
> +    qemu_mutex_init(&monitor_lock);
> +}

Are we sure that this new function is called sooner than any access to
monitor_lock,

> -static void __attribute__((constructor)) monitor_lock_init(void)
> -{
> -    qemu_mutex_init(&monitor_lock);
> -}

especially since the old code initialized the lock REALLY early?

> diff --git a/vl.c b/vl.c
> index fb1f05b..850cf55 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3049,7 +3049,6 @@ int main(int argc, char **argv, char **envp)
>      qemu_init_exec_dir(argv[0]);
>  
>      module_call_init(MODULE_INIT_QOM);
> -    monitor_init_qmp_commands();
>  
>      qemu_add_opts(&qemu_drive_opts);
>      qemu_add_drive_opts(&qemu_legacy_drive_opts);
> @@ -4587,6 +4586,8 @@ int main(int argc, char **argv, char **envp)
>  
>      parse_numa_opts(current_machine);
>  
> +    monitor_init_globals();
> +

Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on
monitor_lock_init() first, prior to main.

Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089
4089        qemu_mutex_init(&monitor_lock);
(gdb) c
Continuing.
[New Thread 0x7fffce225700 (LWP 26380)]

Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5,
    argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077
3077    {

Post-patch, the mutex is not initialized until well after main().  So
the real question is what (if anything) is using the lock in between
those two points?

Hmm - it may be that we needed it back before commit 05875687, when we
really did depend on MODULE_INIT_QAPI, but it is something we forgot to
cleanup in the meantime?

If nothing else, the commit message should call out that dropping
__attribute__((constructor)) nonsense is intentional (if it was indeed
nonsense).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature