[libvirt] [PATCH] qemu: Remove double unlock for domains

[Martin Kletzander]

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):

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150803/bef139e1/attachment-0001.sig>