On 02/11/2010 10:52 AM, Jim Meyering wrote:

clang warned about a dead-store in src/qemu/qemu_driver.c, since dname is never used thereafter:

http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_driver.c;h=0d77... /* Target domain name, maybe renamed. */ dname = dname ? dname : def->name;

That stmt was initially added with a following use,

if (!vm) vm = virDomainFindByName(&driver->domains, dname);

But the use was removed by this commit, rendering the store "dead", and the dname parameter effectively unused:

http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=c26cb9234f4b9fa46d7caa3...

Whoops, yeah, I stared at that patch for a few minutes before I saw the issue. Must have dname blindness.

The same thing happened in both qemudDomainMigratePrepareTunnel and qemudDomainMigratePrepareTunnel2

Shouldn't they be doing something like this instead?

if (dname) def->name = dname;

But def->name is already allocated. Better free it first. And is dname "known" to be allocated from the heap? That's not clear from the little documentation I've read so far, so I've strdup'd it below:

Here's a possible patch:

...

From b51a39aed62ee5c8db733cecfe6eef0c34a7f989 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 11 Feb 2010 16:46:21 +0100 Subject: [PATCH] qemu_driver.c: honor dname parameter once again

Since c26cb9234f4b9fa46d7caa3385ae36704167c53f, the dname parameter has been ignored by these two functions. Use it. * src/qemu/qemu_driver.c (qemudDomainMigratePrepareTunnel): Honor dname parameter once again. (qemudDomainMigratePrepare2): Likewise. --- src/qemu/qemu_driver.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d77d57..9ff712c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7434,7 +7434,12 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, }

/* Target domain name, maybe renamed. */ - dname = dname ? dname : def->name; + if (dname) { + VIR_FREE(def->name); + def->name = strdup(dname); + if (def->name == NULL) + goto cleanup; + }

if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) goto cleanup; @@ -7660,7 +7665,12 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, }

/* Target domain name, maybe renamed. */ - dname = dname ? dname : def->name; + if (dname) { + VIR_FREE(def->name); + def->name = strdup(dname); + if (def->name == NULL) + goto cleanup; + }

if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) goto cleanup;

Yes, I think this works. The 'def' here is actually from XML passed to the destination migration host: its _not_ the def of an existing VM, so just swapping out name like that should be okay. ACK. Thanks, Cole