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=0d77d572a49e1c86f86911bce54d93fdb4a38feb;hb=HEAD#l7436 > /* 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=c26cb9234f4b9fa46d7caa3385ae36704167c53f > 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@xxxxxxxxxx> > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list