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 <pet...@redhat.com> > --- > 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
