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(). > > With that, could I take your r-b? Yes. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
