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...
John
> The only place where this would cause a problem is the autodestroy
> callback, so we need to get another reference there and uref+unlock it
> afterwards. Luckily, returning NULL from that function doesn't mean an
> error, and only means that it doesn't need to be unlocked anymore.
>
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 7 +++++++
> src/qemu/qemu_process.c | 7 ++++---
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8b050a043995..d6d723112638 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2593,6 +2593,13 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
> virObjectRef(vm);
>
> virDomainObjListRemove(driver->domains, vm);
> + /*
> + * 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.
> + */
> + virObjectLock(vm);
> virObjectUnref(cfg);
>
> if (haveJob)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1c0c734c3811..df38dacdca92 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5696,6 +5696,8 @@ qemuProcessAutoDestroy(virDomainObjPtr dom,
>
> VIR_DEBUG("vm=%s, conn=%p", dom->def->name, conn);
>
> + virObjectRef(dom);
> +
> if (priv->job.asyncJob) {
> VIR_DEBUG("vm=%s has long-term job active, cancelling",
> dom->def->name);
> @@ -5718,15 +5720,14 @@ qemuProcessAutoDestroy(virDomainObjPtr dom,
>
> qemuDomainObjEndJob(driver, dom);
>
> - if (!dom->persistent) {
> + if (!dom->persistent)
> qemuDomainRemoveInactive(driver, dom);
> - dom = NULL;
> - }
>
> if (event)
> qemuDomainEventQueue(driver, event);
>
> cleanup:
> + virDomainObjEndAPI(&dom);
> return dom;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list