On Tue, Jan 09, 2018 at 05:13:40PM -0600, Eric Blake wrote:
> 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,...
Fixed.
>
> >
> > - 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
Sorry for that! I thought you helped proved that somehow (which I
really appreciate)...
>
> 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.
I did that by observing all users of the lock in current repository:
*** monitor.c:
monitor_qmp_response_pop_one[457] qemu_mutex_lock(&monitor_lock);
monitor_qmp_response_pop_one[466] qemu_mutex_unlock(&monitor_lock);
monitor_qapi_event_queue[529] qemu_mutex_lock(&monitor_lock);
monitor_qapi_event_queue[574] qemu_mutex_unlock(&monitor_lock);
monitor_qapi_event_handler[588] qemu_mutex_lock(&monitor_lock);
monitor_qapi_event_handler[604] qemu_mutex_unlock(&monitor_lock);
monitor_qmp_requests_pop_one[4116] qemu_mutex_lock(&monitor_lock);
monitor_qmp_requests_pop_one[4136] qemu_mutex_unlock(&monitor_lock);
monitor_init[4559] qemu_mutex_lock(&monitor_lock);
monitor_init[4561] qemu_mutex_unlock(&monitor_lock);
monitor_cleanup[4596] qemu_mutex_lock(&monitor_lock);
monitor_cleanup[4603] qemu_mutex_unlock(&monitor_lock);
monitor_init() and monitor_cleanup() are called after global init, so
it should be fine (monitor_init() is called right after the global
init).
For the rest of the functions:
monitor_qmp_response_pop_one
monitor_qapi_event_queue
monitor_qapi_event_handler
monitor_qmp_requests_pop_one
AFAIK all of them are called even after monitor_init(), in other
words, they are all after global init too.
As a conclusion, we should be safe here. Again, I may be wrong
somewhere, please correct me if so.
>
> Only if you improve the commit message, you may add:
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Besides the English fix, how about I add one more paragraph to talk
about monitor_lock in commit message:
monitor_lock won't be used before monitor_init(). So as long as we
initialize the monitor globals before the first call to
monitor_init(), we will be safe.
With that, could I take your r-b?
Thanks,
>
> > +++ 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
>
--
Peter Xu
