[libvirt] [PATCH] qemu: Move one misplaced driver unlock to the right function
This should have been commit 56fd513 already, but was missed by initially. The driver unlock call in the cleanup section of DomainManagedSave does actually belong to DomainSendKey. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c28c223..853e35d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2434,6 +2434,7 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); + qemuDriverUnlock(driver); return ret; } @@ -3174,7 +3175,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); VIR_FREE(name); return ret; -- 1.7.9.5
On 01/18/2013 08:45 AM, Viktor Mihajlovski wrote:
This should have been commit 56fd513 already, but was missed by initially. The driver unlock call in the cleanup section of
s/by //
DomainManagedSave does actually belong to DomainSendKey.
Please also call out commit 8c5d2ba as the source of the original problem.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c28c223..853e35d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2434,6 +2434,7 @@ endjob: cleanup: if (vm) virObjectUnlock(vm); + qemuDriverUnlock(driver); return ret;
This is part of qemuDomainSendKey, and undoes the mistake in 8c5d2ba; good.
}
@@ -3174,7 +3175,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); VIR_FREE(name);
This is in qemuDomainManagedSave, which was not mentioned in the commit message of 8c5d2ba, was not touched in 56fd513, but does fix a bug (unlocking without owning the lock). Now I have to go dig which commit introduced _this_ problem... Found it - commit 2745177 was the culprit here. Yuck. You _still_ haven't fixed DomainHasManagedSaveImage (which 8c5d2ba claimed to fix), and we've found yet another bogus commit. We need a v2 of this patch that scrubs ALL of the bugs at once. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/18/2013 10:19 AM, Eric Blake wrote:
On 01/18/2013 08:45 AM, Viktor Mihajlovski wrote:
This should have been commit 56fd513 already, but was missed by initially. The driver unlock call in the cleanup section of
s/by //
DomainManagedSave does actually belong to DomainSendKey.
Please also call out commit 8c5d2ba as the source of the original problem.
@@ -3174,7 +3175,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) cleanup: if (vm) virObjectUnlock(vm); - qemuDriverUnlock(driver); VIR_FREE(name);
This is in qemuDomainManagedSave, which was not mentioned in the commit message of 8c5d2ba, was not touched in 56fd513, but does fix a bug (unlocking without owning the lock). Now I have to go dig which commit introduced _this_ problem...
Found it - commit 2745177 was the culprit here.
Scratch that; I just reread the function. qemuDomainManagedSave used qemuDomObjFromDomainDriver(), which returns driver locked; so commit 2745177 is correct, and there is no locking bug here, and _your_ hunk would make it wrong.
Yuck. You _still_ haven't fixed DomainHasManagedSaveImage (which 8c5d2ba claimed to fix), and we've found yet another bogus commit. We need a v2 of this patch that scrubs ALL of the bugs at once.
This is still true - we need a v2 of the patch that fixes all of the bugs, it's just that there are fewer bugs than I was worried about, because right now it looks like there is only one bogus commit, not two. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/18/2013 06:21 PM, Eric Blake wrote:
Scratch that; I just reread the function. qemuDomainManagedSave used qemuDomObjFromDomainDriver(), which returns driver locked; so commit 2745177 is correct, and there is no locking bug here, and _your_ hunk would make it wrong.
right, my apologies, I was mislead by my typo-in-commit-message theory, see below.
Yuck. You _still_ haven't fixed DomainHasManagedSaveImage (which 8c5d2ba claimed to fix), and we've found yet another bogus commit. We need a v2 of this patch that scrubs ALL of the bugs at once.
This is still true - we need a v2 of the patch that fixes all of the bugs, it's just that there are fewer bugs than I was worried about, because right now it looks like there is only one bogus commit, not two.
This exactly is the crux: I have no clue what the intended fix would be. This function didn't hold a driver lock before the commit, so how can it be relaxed then? This was what lead me to the wrong conclusion hat qemuManagedSave was meant. So, before I send out a revised patch: do you think there's anything to do for qemuDomainHasManagedSaveImage and if so, what? -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Yuck. You _still_ haven't fixed DomainHasManagedSaveImage (which 8c5d2ba claimed to fix), and we've found yet another bogus commit. We need a v2 of this patch that scrubs ALL of the bugs at once. Stepping thru the history of qemu_driver.c I saw that commit 2745177b has removed the driver lock. If 8c5d2ba was trying to do the same
On 01/18/2013 06:21 PM, Eric Blake wrote: this has probably caused a rebase conflict, which seems to have been not resolved correctly, resulting in the broken commit. So there's nothing to do for DomainHasManagedSaveImage. Look out for the V2 version... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 01/23/2013 06:24 AM, Viktor Mihajlovski wrote:
Yuck. You _still_ haven't fixed DomainHasManagedSaveImage (which 8c5d2ba claimed to fix), and we've found yet another bogus commit. We need a v2 of this patch that scrubs ALL of the bugs at once. Stepping thru the history of qemu_driver.c I saw that commit 2745177b has removed the driver lock. If 8c5d2ba was trying to do the same
On 01/18/2013 06:21 PM, Eric Blake wrote: this has probably caused a rebase conflict, which seems to have been not resolved correctly, resulting in the broken commit. So there's nothing to do for DomainHasManagedSaveImage.
Aha, that explains it. Thanks for doing the legwork on this.
Look out for the V2 version...
Yep, and I'll push it shortly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Viktor Mihajlovski