Hi John,

Thanks for your review!

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

After finishing the above steps, we can get something wrong:

# virsh list --all

 Id    Name                           State

----------------------------------------------------

 -     win7                           shut off

 

# virsh undefine win7

error: Failed to undefine domain win7

error: Requested operation is not valid: cannot undefine transient domain

 

> There's two reasons to get to the endjob: label...

> #1 virDomainObjIsActive (domain already active)

> #2 qemuDomainObjStart fails

> So this certainly wouldn't be right for the IsActive condition, under

> "normal" circumstances. And again, I don't think requiring a subsequent

> call to Start would be "correct" either.  I am curious though if when in

> this condition, is the vm->def->id set?  IOW: What does 'virsh list'

> indicate or even virsh dominfo $domain?

What you said sounds reasonable, I can call qemuDomainObjStart at once after qemuDomainObjStart fails like libvirt does in qemuDomainCreateXML, but end job should be called before qemuDomainRemoveInactive :)