[libvirt] [PATCH] qemu: enable direct migration over IPv6

[Eric Blake]

On 02/25/2013 07:15 AM, Ján Tomko wrote:
> Use virURIParse in qemuMigrationPrepareDirect to allow parsing
> IPv6 addresses, which would cause an 'incorrect :port' error message
> before.
> 
> To be able to migrate over IPv6, QEMU needs to listen on [::] instead
> of 0.0.0.0. This patch adds a call to getaddrinfo and sets the listen
> address based on the result.
> 
> It also uses the same listen address for the NBD server.
> 
> This will break migration if a hostname does not resolve to the same
> address family on both sides.
> 
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=846013
> ---
> 
> Diff to V1:
> * initialize uri_str
> * reuse STRSKIP("tcp:") result instead of doing strlen on it
> * print a warning instead of failing when the hostname can't be resolved
> 
> Diff to V2:
> * freeaddrinfo
> * separate the listen address to allow reuse in qemuMigrationStartNBDServer
> 
>  src/qemu/qemu_migration.c | 77 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index cae58fa..ff9b959 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -22,7 +22,10 @@
>  
>  #include <config.h>
>  
> +#include <netdb.h>
> +#include <sys/socket.h>
>  #include <sys/time.h>
> +#include <sys/types.h>

You rarely ever need <sys/types.h> - most types in that header are
guaranteed to be provided by any other standard header that has an
interface using that type; or put another way, POSIX says that inclusion
of other headers may drag in <sys/types.h>, and we have already been
relying on that reliably happening.  The other two additions are
important, though.

>  
> @@ -2292,9 +2296,14 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>      static int port = 0;
>      int this_port;
>      char *hostname = NULL;
> +    char listenAddr[8];
>      char migrateFrom [64];

As long as you are touching this, remove the space before [64].  Then
again, 64 was a magic number; a much safer approach would have been

char migrateFrom[sizeof("tcp:0.0.0.0:") + INT_BUFSIZE_BOUND(int)];

(using gnulib's "intprops.h"), or even getting rid of the stack-alloc
temporary (see below for thoughts about using virBufferPtr instead)

>  
> +    if (getaddrinfo(hostname, NULL, NULL, &info)) {
> +        VIR_WARN("unable to get address info for %s, defaulting to IPv4",
> +                 hostname);

Would it be nice to include the gai_strerror() reason for why
getaddrinfo() failed?

> +    } else {
> +        ipv6 = info->ai_family == AF_INET6;
> +        freeaddrinfo(info);
> +    }
> +
>      if (*uri_out)
>          VIR_DEBUG("Generated uri_out=%s", *uri_out);
>  
> -    /* QEMU will be started with -incoming tcp:0.0.0.0:port */
> -    snprintf(migrateFrom, sizeof(migrateFrom), "tcp:0.0.0.0:%d", this_port);
> +    /* QEMU will be started with -incoming tcp:0.0.0.0:port
> +     * or -incoming tcp:[::]:port for IPv6 */
> +    if (ipv6)
> +        snprintf(listenAddr, sizeof(listenAddr), "[::]");
> +    else
> +        snprintf(listenAddr, sizeof(listenAddr), "0.0.0.0");

Eww.  Even though this is safe (because you sized listenAddr big
enough), I'd rather see:

const char *listenAddr;

if (ipv6)
    listenAddr = "[::]";
else
    listenAddr = "0.0.0.0";

> +
> +    snprintf(migrateFrom, sizeof(migrateFrom),
> +             "tcp:%s:%d", listenAddr, this_port);

And while this snprintf was pre-existing and also happens to be large
enough, I can't help but think it would be nicer to pass a virBufferPtr
instead of a const char * to qemuMigrationPrepareAny, although that
would mean more refactoring done in an independent patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130225/86bc634f/attachment-0001.sig>