[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init |
Date: |
Thu, 11 Jan 2018 16:18:18 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Wed, Jan 10, 2018 at 06:54:45AM -0600, Eric Blake wrote:
> On 01/10/2018 02:26 AM, Peter Xu wrote:
>
> >> 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:
>
>
> > 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.
>
> My gdb testing and your analysis match; we're safe. So all that's
> needed is the paragraph documenting that we thought about the issue:
>
> >
> >>
> >> Only if you improve the commit message, you may add:
> >> Reviewed-by: Eric Blake <address@hidden>
> >
> > 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.
>
> Or even:
>
> monitor_lock is not used before monitor_init() (as confirmed by code
> analysis and gdb watchpoints); so we are safe delaying what was a
> constructor-time initialization of the mutex into the later first call
> to monitor_init().
Will take away this.
>
> >
> > With that, could I take your r-b?
>
> Yes.
Thank you.
--
Peter Xu