On 07/12/2017 07:34 AM, wang.yi59@xxxxxxxxxx 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. John > > 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 :) > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
