On 12/19/2017 02:45 AM, Peter Xu wrote: > There are many places for monitor init its globals, at least:
Reads awkwardly; maybe:
...many places where the monitor initializes its globals,...
>
> - 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.
>
> Reviewed-by: Fam Zheng <f...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> +void monitor_init_globals(void)
> +{
> + monitor_init_qmp_commands();
> + monitor_qapi_event_init();
> + sortcmdlist();
> + qemu_mutex_init(&monitor_lock);
> +}
> +
> /* These functions just adapt the readline interface in a typesafe way. We
> * could cast function pointers but that discards compiler checks.
> */
> @@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt,
> va_list ap)
> }
> }
>
> -static void __attribute__((constructor)) monitor_lock_init(void)
> -{
> - qemu_mutex_init(&monitor_lock);
> -}
The later initialization of the monitor_lock mutex is a potential
semantic change. Please beef up the commit message to document why it
is safe. In fact, I requested this back on my review of v1, but it
still hasn't been done. :(
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html
If my read of history is correct, I think it is sufficient to point to
commit 05875687 as a place where we no longer care about constructor
semantics because we are no longer dealing with module_call_init(). But
you may find a better place to point to. You already found that
d622cb587 was what introduced the constructor in the first place, but I
didn't spend time today auditing the state of qemu back at that time to
see if the constructor was really necessary back then or just a
convenience for lack of a better initialization point.
Alternatively, if you can't find a good commit message to point to, at
least document how you (and I) tested things, using gdb watchpoints, to
prove it is a safe delay.
Only if you improve the commit message, you may add:
Reviewed-by: Eric Blake <ebl...@redhat.com>
> +++ b/vl.c
> @@ -3099,7 +3099,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);
> @@ -4645,6 +4644,12 @@ 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 used in monitor_qapi_event_init())
s/which/which is/
> + * depend on configure_accelerator() above.
s/depend/depends/
> + */
> + monitor_init_globals();
> +
> if (qemu_opts_foreach(qemu_find_opts("mon"),
> mon_init_func, NULL, NULL)) {
> exit(1);
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
