if specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it. Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/libvirt_private.syms | 2 +- src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++------------------------ src/util/virsocketaddr.c | 19 +++++++++--------- src/util/virsocketaddr.h | 2 +- tests/sockettest.c | 2 +- 5 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cbc35b..8ab1394 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1911,11 +1911,11 @@ virSocketAddrGetIpPrefix; virSocketAddrGetPort; virSocketAddrGetRange; virSocketAddrIsNetmask; -virSocketAddrIsNumeric; virSocketAddrIsPrivate; virSocketAddrIsWildcard; virSocketAddrMask; virSocketAddrMaskByPrefix; +virSocketAddrNumericFamily; virSocketAddrParse; virSocketAddrParseIPv4; virSocketAddrParseIPv6; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 284cd5a..e135249 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,7 +2605,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (VIR_STRDUP(migrateFrom, "stdio") < 0) goto cleanup; } else { - virSocketAddr listenAddressSocket; bool encloseAddress = false; bool hostIPv6Capable = false; bool qemuIPv6Capable = false; @@ -2627,28 +2626,21 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virObjectUnref(qemuCaps); if (listenAddress) { - if (virSocketAddrIsNumeric(listenAddress)) { - /* listenAddress is numeric IPv4 or IPv6 */ - if (virSocketAddrParse(&listenAddressSocket, listenAddress, AF_UNSPEC) < 0) + if (virSocketAddrNumericFamily(listenAddress) == AF_INET6) { + if (!qemuIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("qemu isn't capable of IPv6")); goto cleanup; - - /* address parsed successfully */ - if (VIR_SOCKET_ADDR_IS_FAMILY(&listenAddressSocket, AF_INET6)) { - if (!qemuIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("qemu isn't capable of IPv6")); - goto cleanup; - } - if (!hostIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("host isn't capable of IPv6")); - goto cleanup; - } - /* IPv6 address must be escaped in brackets on the cmd line */ - encloseAddress = true; } + if (!hostIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("host isn't capable of IPv6")); + goto cleanup; + } + /* IPv6 address must be escaped in brackets on the cmd line */ + encloseAddress = true; } else { - /* listenAddress is a hostname */ + /* listenAddress is a hostname or IPv4 */ } } else if (qemuIPv6Capable && hostIPv6Capable) { /* Listen on :: instead of 0.0.0.0 if QEMU understands it @@ -2950,15 +2942,17 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { + bool encloseAddress = false; + const char *incFormat; + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) goto cleanup; if (migrateHost != NULL) { - if (virSocketAddrIsNumeric(migrateHost) && - virSocketAddrParse(NULL, migrateHost, AF_UNSPEC) < 0) - goto cleanup; + if (virSocketAddrNumericFamily(migrateHost) == AF_INET6) + encloseAddress = true; - if (VIR_STRDUP(hostname, migrateHost) < 0) + if (VIR_STRDUP(hostname, migrateHost) < 0) goto cleanup; } else { if ((hostname = virGetHostname()) == NULL) @@ -2977,7 +2971,12 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * compatibility with old targets. We at least make the * new targets accept both syntaxes though. */ - if (virAsprintf(uri_out, "tcp:%s:%d", hostname, port) < 0) + if (encloseAddress) + incFormat = "%s:[%s]:%d"; + else + incFormat = "%s:%s:%d"; + + if (virAsprintf(uri_out, incFormat, "tcp", hostname, port) < 0) goto cleanup; } else { bool well_formed_uri; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 7cc4bde..7fe7a15 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -856,26 +856,25 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, } /** - * virSocketAddrIsNumeric: + * virSocketAddrNumericFamily: * @address: address to check * - * Check if passed address is an IP address in numeric format. For - * instance, for 0.0.0.0 true is returned, for 'examplehost" - * false is returned. + * Check if passed address is an IP address in numeric format. and + * return the address family, otherwise return 0. * - * Returns: true if @address is an IP address, - * false otherwise + * Returns: AF_INET or AF_INET6 if @address is an numeric IP address, + * -1 otherwise. */ -bool -virSocketAddrIsNumeric(const char *address) +int +virSocketAddrNumericFamily(const char *address) { struct addrinfo *res; unsigned short family; if (virSocketAddrParseInternal(&res, address, AF_UNSPEC, false) < 0) - return false; + return -1; family = res->ai_addr->sa_family; freeaddrinfo(res); - return family == AF_INET || family == AF_INET6; + return family; } diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 27defa0..35c9c1a 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -125,5 +125,5 @@ bool virSocketAddrIsPrivate(const virSocketAddr *addr); bool virSocketAddrIsWildcard(const virSocketAddr *addr); -bool virSocketAddrIsNumeric(const char *address); +int virSocketAddrNumericFamily(const char *address); #endif /* __VIR_SOCKETADDR_H__ */ diff --git a/tests/sockettest.c b/tests/sockettest.c index 68b0536..abf7350 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -229,7 +229,7 @@ testIsNumericHelper(const void *opaque) { const struct testIsNumericData *data = opaque; - if (virSocketAddrIsNumeric(data->addr)) + if (virSocketAddrNumericFamily(data->addr) != -1) return data->pass ? 0 : -1; return data->pass ? -1 : 0; } -- 1.9.3

On 10/07/2014 06:07 AM, Chen Fan wrote: We don't format the qemu.conf back and we don't need the brackets for anything. We can just store the migration host without them in cfg->migrationHost There's no need to check for family upfront. I've rewritten this function to take a 'const char *' argument. Along with the virStringStripIPv6Brackets function I've sent for review separately, this removes the need for a separate qemuCheckLocalhost function and it can be inlined. The diff I'll squash into this patch before pushing (after virStringStripIPv6Brackets is sorted out): --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -707,8 +707,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox); GET_VALUE_STR("migration_host", cfg->migrateHost); + virStringStripIPv6Brackets(cfg->migrateHost); if (cfg->migrateHost && - qemuCheckLocalhost(cfg->migrateHost)) { + (STRPREFIX(cfg->migrateHost, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrateHost))) { virReportError(VIR_ERR_CONF_SYNTAX, _("migration_host must not be the address of" " the local machine: %s"), @@ -1380,44 +1382,3 @@ qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, return qemuGetHugepagePath(&hugetlbfs[i]); } - -bool -qemuCheckLocalhost(const char *addrStr) -{ - virSocketAddr addr; - char *hostname, *tmp; - bool encloseAddress = false; - int family; - bool ret = true; - - if (VIR_STRDUP(hostname, addrStr) < 0) - return false; - - tmp = hostname; - - if (STRPREFIX(hostname, "[")) { - char *end = strchr(hostname, ']'); - if (end) { - *end = '\0'; - hostname++; - encloseAddress = true; - } - } - - if (STRPREFIX(hostname, "localhost")) - goto cleanup; - - family = virSocketAddrNumericFamily(hostname); - if ((family == AF_INET && !encloseAddress) || - family == AF_INET6) { - if (virSocketAddrParse(&addr, hostname, family) > 0 && - virSocketAddrIsNumericLocalhost(&addr)) { - goto cleanup; - } - } - - ret = false; -cleanup: - VIR_FREE(tmp); - return ret; -} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c9ce53c..cb01fb6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -322,6 +322,4 @@ int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn, char * qemuGetHugepagePath(virHugeTLBFSPtr hugepage); char * qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, size_t nhugetlbfs); - -bool qemuCheckLocalhost(const char *addrStr); #endif /* __QEMUD_CONF_H */ diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 6d36689..a19e3af 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -889,15 +889,24 @@ virSocketAddrNumericFamily(const char *address) * false otherwise */ bool -virSocketAddrIsNumericLocalhost(const virSocketAddr *addr) +virSocketAddrIsNumericLocalhost(const char *addr) { + struct addrinfo *res; struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) }; - switch (addr->data.stor.ss_family) { + struct sockaddr_in *inet4; + struct sockaddr_in6 *inet6; + + if (virSocketAddrParseInternal(&res, addr, AF_UNSPEC, false) < 0) + return false; + + switch (res->ai_addr->sa_family) { case AF_INET: - return memcmp(&addr->data.inet4.sin_addr.s_addr, &tmp.s_addr, - sizeof(addr->data.inet4.sin_addr.s_addr)) == 0; + inet4 = (struct sockaddr_in*) res->ai_addr; + return memcmp(&inet4->sin_addr.s_addr, &tmp.s_addr, + sizeof(inet4->sin_addr.s_addr)) == 0; case AF_INET6: - return IN6_IS_ADDR_LOOPBACK(&addr->data.inet6.sin6_addr); + inet6 = (struct sockaddr_in6*) res->ai_addr; + return IN6_IS_ADDR_LOOPBACK(&(inet6->sin6_addr)); } return false; diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index fa9e98b..053855b 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -127,5 +127,5 @@ bool virSocketAddrIsWildcard(const virSocketAddr *addr); int virSocketAddrNumericFamily(const char *address); -bool virSocketAddrIsNumericLocalhost(const virSocketAddr *addr); +bool virSocketAddrIsNumericLocalhost(const char *addr); #endif /* __VIR_SOCKETADDR_H__ */ Jan

This patch has triggered a Coverity RESOURCE_LEAK (3 actually) On 10/08/2014 09:54 PM, Chen, Fan wrote: 'res' allocates something that must be free'd Leak #1 Leak #2 Leak #3 Other callers will call 'freeaddrinfo(res);' In order to resolve - you probably need to create a local ret = false, then assign ret = to either memcmp/IN6_IS_ADDR_LOOPBACK return, then call freeaddrinfo() before return ret John

When enabling the migration_address option, by default it is set to "127.0.0.1", but it's not a valid address for migration. so we should add verification and set the default migration_address to "0.0.0.0". Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/test_libvirtd_qemu.aug.in | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 92ca715..c6db568 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -467,7 +467,7 @@ # Override the listen address for all incoming migrations. Defaults to # 0.0.0.0, or :: if both host and qemu are capable of IPv6. -#migration_address = "127.0.0.1" +#migration_address = "0.0.0.0" # The default hostname or IP address which will be used by a migration diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6b0ac5c..f34fa06 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -717,6 +717,14 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } GET_VALUE_STR("migration_address", cfg->migrationAddress); + if (cfg->migrationAddress && + qemuCheckLocalhost(cfg->migrationAddress)) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_address must not be the address of" + " the local machine: %s"), + cfg->migrationAddress); + goto cleanup; + } GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index d2bc2c0..30fd27e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -69,7 +69,7 @@ module Test_libvirtd_qemu = { "keepalive_interval" = "5" } { "keepalive_count" = "5" } { "seccomp_sandbox" = "1" } -{ "migration_address" = "127.0.0.1" } +{ "migration_address" = "0.0.0.0" } { "migration_host" = "host.example.com" } { "migration_port_min" = "49152" } { "migration_port_max" = "49215" } -- 1.9.3