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

[John Ferlan]

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