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

[Eric Blake]

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

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