Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
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
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
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?
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..ddce248635b8 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -323,10 +323,8 @@ 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);
--
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
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
> ---
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Remove double unlock for domains
On Wed, Jul 15, 2015 at 09:17:46AM +0200, 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().
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
---
Post-release ping
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;
}
--
2.4.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Remove double unlock for domains
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().
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
---
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;
}
--
2.4.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
