9:45 a.m.
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(a)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
11:19 a.m.
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(a)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
11:21 a.m.
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
6:33 a.m.
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
7:24 a.m.
On 01/18/2013 06:21 PM, Eric Blake 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
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
4 p.m.
On 01/23/2013 06:24 AM, Viktor Mihajlovski wrote:
On 01/18/2013 06:21 PM, Eric Blake 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
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
4321
days inactive
4326
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Viktor Mihajlovski