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

It failed in processing during ProcessLaunch, on which period qemu exited

unexpectedly.

>

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

Yes, your advice looks better. So it seems like that we should protect all

of qemuProcessStart, 'cause some functions such like qemuProcessFinishStartup

may also invoke Monitor.

If this works, I will send the V2 patch.

>

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