On 08/03/2015 10:04 AM, Martin Kletzander wrote:
On Mon, Aug 03, 2015 at 04:00:32PM +0200, Martin Kletzander wrote:
> On Mon, Aug 03, 2015 at 09:21:51AM -0400, John Ferlan wrote:
>>
>>
>> On 07/15/2015 03:17 AM, Martin Kletzander wrote:
>>> The virDomainObjListRemove() function unlocks a domain that it's given
>>> due to legacy code. And because of that code, which should be
>>> refactored, that last virObjectUnlock() cannot be just removed. So
>>> instead, lock it right back for qemu for now. All calls to
>>> qemuDomainRemoveInactive() are followed by code that unlocks the domain
>>> again, plus the domain should be locked during
>>> qemuDomainObjEndJob(), so
>>> the right place to lock it is right after virDomainObjListRemove().
>>>
>>
>> Looked through callers - seems qemuProcessHandleMonitorEOF may need a
>> tweak too as it skips around the virObjectUnlock(vm)
>>
>> My only other comment would be perhaps the entry comment into
>> qemuDomainRemoveInactive could use a tweak on the wording and some
>> additional text to indicate on exit it should still hold the lock (or
>> just move the blob you added... Perhaps an XXX to force a revisit in the
>> future based on your note above "which should be refactored"
>>
>> Not that anyone reads those anyway ;-)
>>
>> ACK with the double check on the *EOF function...
>>
>
> Would it be enough if I squashed this in?
>
Sorry, incomplete diff, here's the proper one, it's cleaner and looks
better (and compiles):
Sure, this works for me
ACK
John
diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index d6d723112638..6ba8087c108b 100644
--- i/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -2597,7 +2597,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
* virDomainObjListRemove() leaves the domain unlocked so it can
* be unref'd for other drivers that depend on that, but we still
* need to reset a job and we have a reference from the API that
- * called this function. So we need to lock it back.
+ * called this function. So we need to lock it back. This is
+ * just a workaround for the qemu driver.
+ *
+ * XXX: Ideally, the global handling of domain objects and object
+ * lists would be refactored so we don't need hacks like
+ * this, but since that requires refactor of all drivers,
+ * it's a work for another day.
*/
virObjectLock(vm);
virObjectUnref(cfg);
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 5a9f97bc8921..505778ec2f05 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -295,12 +295,12 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
if (priv->beingDestroyed) {
VIR_DEBUG("Domain is being destroyed, EOF is expected");
- goto unlock;
+ goto cleanup;
}
if (!virDomainObjIsActive(vm)) {
VIR_DEBUG("Domain %p is not active, ignoring EOF", vm);
- goto unlock;
+ goto cleanup;
}
if (priv->monJSON && !priv->gotShutdown) {
@@ -323,15 +323,11 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
qemuProcessStop(driver, vm, stopReason, stopFlags);
virDomainAuditStop(vm, auditReason);
- if (!vm->persistent) {
+ if (!vm->persistent)
qemuDomainRemoveInactive(driver, vm);
- goto cleanup;
- }
-
- unlock:
- virObjectUnlock(vm);
cleanup:
+ virObjectUnlock(vm);
if (event)
qemuDomainEventQueue(driver, event);
}
--
Martin