[libvirt] dname parameter ignored in qemudDomainMigratePrepareTunnel{, 2}

[Cole Robinson]

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 at 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