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