qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init
Date: Tue, 9 Jan 2018 17:13:40 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0

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 <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---

> +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 <address@hidden>

> +++ 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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]