Hi John,

I have reworked this patch on the other new thread[1], please review that

patch.

Thank you :-)

[1] https://www.redhat.com/archives/libvir-list/2017-July/msg00921.html

>On 07/12/2017 07:34 AM, wang.yi59@zte.com.cn wrote:

>> Hi John,

>> 

>> Thanks for your review!

>> 

>

>Somehow your response is out of synch with the rest of the series -

>things like this get lost very quickly.

>

>>> This seems to be a strange sequence of operations, but the claim is that

>> 

>>> by adding this logic to CreateWithFlags, then the problem you're facing

>> 

>>> is resolved. However, is adding this to the Create logic the right thing

>> 

>>> to do?

>> 

>>> IIUC:

>> 

>> This condition is rare, so I designed some operations which can help us

>> to understand what happened:

>> 

>> # virsh define win7 -> successful

>> 

>> # virsh start win7 &; sleep 0.2; virsh undefine win7 -> start failed and

>> undefine successful

>> 

>>  

>> 

>> and we may see libvirtd output such logs like this:

>> 

>> qemuDomainDefineXMLFlags:7386 : Creating domain win7

>> 

>> qemuProcessStart:6086 : conn=....

>> 

>> qemuDomainUndefineFlags:7501 : Undefining domain win7

>> 

>> error : qemuMonitorOpenUnix:379 : failed to connect to monitor socket

>> 

>>  

>> 

>> Libvirtd unlocked vm during qemuProcessStart, so qemuDomainUndefineFlags

>> can get the lock and undefine vm successfully.

>

>Can you be a bit more specific. Do you mean during qemuConnectMonitor

>processing during ProcessLaunch? Or perhaps Agent processing?

>

>In any case, perhaps then the better fix is to prohibit undefine whilst

>unlocked for Monitor or Agent start. The perhaps interesting question is

>where to set flag. Setting/clearing just around Monitor/Agent startup

>would be a seemingly short window and perhaps where you're seeing the

>virObjectUnlock; however, I wonder if perhaps all of qemuProcessLaunch

>should be "protected" as there's some really critical things happening

>in there. Thus at the top one sets a boolean "obj->starting = true" and

>at cleanup "obj->starting = false".

>

>In qemuDomainUndefineFlags, there'd need to be a similar check to the if

>(!persistent) along the lines of "if (starting)" then an error message

>indicating that the domain is starting up and cannot be undefined.

>Perhaps similarly for Destroy just to be safe.

>

>I didn't think all that long about it, but hopefully it's enough to

>perhaps have to generate more patches...  I don't believe with that it'd

>be possible to have the need to qemuDomainRemoveInactive in the

>CreateWithFlags, but I could be wrong.