On 09/10/2011 11:43 PM, Guannan Ren wrote:
Your subject line is rather long; I trimmed it to:
snapshot: fix double free of qemuImgBinary
*src/qemu/qemu_driver.c: In qemuDomainSnapshotForEachQcow()
it free up the memory of qemu_driver->qemuImgBinary in the
cleanup tag which leads to the garbage value of qemuImgBinary
in qemu_driver struct and libvirtd crash when running
"virsh snapshot-create" command at second time.
---
src/qemu/qemu_driver.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)
Thanks for finding this - I had encountered this bug last Thursday, but
then was unable to investigate root cause. Your patch is almost
perfect. I added mention of the commit id where the bug was introduced
(3881a470 added qemu-img caching incorrectly). The regression stems
from the fact that I wrote my patch for qemu-img caching first, then
wrote my patch to refactor things to add qemuDomainSnapshotForEachQcow2
(commit 25fb3ef), but then rebased things to apply the patches in the
opposite order, failing to notice that my rebase left behind a spurious
VIR_FREE in the new function.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b94d1c4..d5a2bc0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1681,14 +1681,13 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
bool try_all)
{
const char *qemuimgarg[] = { NULL, "snapshot", NULL, NULL, NULL, NULL };
- int ret = -1;
int i;
bool skipped = false;
qemuimgarg[0] = qemuFindQemuImgBinary(driver);
if (qemuimgarg[0] == NULL) {
/* qemuFindQemuImgBinary set the error */
- goto cleanup;
+ return -1;
}
qemuimgarg[2] = op;
@@ -1715,7 +1714,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
_("Disk device '%s' does not support"
" snapshotting"),
vm->def->disks[i]->info.alias);
Except that it is missing one additional improvement - here,
vm->def->disks[i]->info.alias can be NULL (I first noticed the log had a
literal "(null)", thanks to glibc, but other platforms would crash on a
NULL deref). But we are guaranteed that disks[i]->dst is always
non-NULL, and still a reasonably useful string to log. Besides, the
info.alias field is internal to libvirt (no way for the user to set it
in their xml), while the dst field comes straight from the user.
I'm pushing with this squashed in:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index d5a2bc0..321b07b 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -1706,14 +1706,14 @@ qemuDomainSnapshotForEachQcow2(struct
qemud_driver *driver,
* disks in this VM may have the same snapshot name.
*/
VIR_WARN("skipping snapshot action on %s",
- vm->def->disks[i]->info.alias);
+ vm->def->disks[i]->dst);
skipped = true;
continue;
}
qemuReportError(VIR_ERR_OPERATION_INVALID,
_("Disk device '%s' does not support"
" snapshotting"),
- vm->def->disks[i]->info.alias);
+ vm->def->disks[i]->dst);
return -1;
}
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list