[libvirt] [PATCH 1/2] util: add wrapper function to check if address string is an ip
--- src/util/network.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/network.h | 2 ++ 2 files changed, 39 insertions(+), 0 deletions(-) diff --git a/src/util/network.c b/src/util/network.c index 6e24792..2e0cf9c 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -54,6 +54,43 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { } /** + * virIsNumericIPAddr: + * @address: a text string containing a host name or IPv4/IPv6 address + * + * Given a text string, determines whether it contains a valid IPv4/IPv6 + * address. + * + * Returns 0 if the given string is an IPv4 or IPv6 address, or non-zero + * for any other condition. + */ +int +virIsNumericIPAddr(const char *address) { + int returnCode; + struct addrinfo hints; + struct addrinfo *res = NULL; + + /* Sanity check we've been given a text string */ + if (address == NULL) + return(-1); + + /* Do a name lookup on the given string, and see what we're given back */ + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_UNSPEC; /* Doesn't matter whether IPv4 or IPv6 */ + hints.ai_socktype = 0; /* Socket type doesn't matter */ + hints.ai_flags = AI_NUMERICHOST; /* Force a numeric IP address check */ + hints.ai_protocol = 0; /* Doesn't matter which protocol */ + returnCode = getaddrinfo (address, NULL, &hints, &res); + if (0 == returnCode) { + /* A result was returned. This means the text string we were + * given is a valid IP address. We now free the result(s) we + * were given, and pass back the return code */ + freeaddrinfo(res); + } + + return returnCode; +} + +/** * virSocketParseAddr: * @val: a numeric network address IPv4 or IPv6 * @addr: where to store the return value. diff --git a/src/util/network.h b/src/util/network.h index 5307c8c..c2e88d8 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -24,6 +24,8 @@ typedef union { } virSocketAddr; typedef virSocketAddr *virSocketAddrPtr; +int virIsNumericIPAddr (const char *address); + int virSocketParseAddr (const char *val, virSocketAddrPtr addr, int hint); -- 1.7.2.1
This fixes a problem where migration fails when a destination host uses a wrongly configured hostname, that doesn't resolve correctly. If we're given the direct IP address of the destination host, the migration will now use this instead of the resolved host name, allowing things to work. Addresses BZ # 615941: https://bugzilla.redhat.com/show_bug.cgi?id=615941 --- Please note. The DEBUG() and DEBUG0() statements in the code below don't seem to output anywhere. Left them in for now, in the assumption the problem is somewhere in my system configuration. Happy to remove them entirely if that's a better idea. src/libvirt.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 3ec5724..05e8d76 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -39,6 +39,7 @@ #include "uuid.h" #include "util.h" #include "memory.h" +#include "network.h" #ifndef WITH_DRIVER_MODULES # ifdef WITH_TEST @@ -3280,6 +3281,7 @@ virDomainMigrateVersion2 (virDomainPtr domain, unsigned long bandwidth) { virDomainPtr ddomain = NULL; + char *new_uri = NULL; char *uri_out = NULL; char *cookie = NULL; char *dom_xml = NULL; @@ -3332,8 +3334,68 @@ virDomainMigrateVersion2 (virDomainPtr domain, virDispatchError(domain->conn); goto done; } - if (uri_out) - uri = uri_out; /* Did domainMigratePrepare2 change URI? */ + + if (uri_out) { + /* We need to determine the best server address to use for the + * destination host. We potentially have a choice of (up to) two: + * + * a) The server address given to the migration function on the + * source, whose info must be ok otherwise we wouldn't have + * made it this far through the code, or + * + * b) The server address returned to us by the destination + * host itself (may be broken in some situations) + * + * We decide by checking if the address given for a) above is a + * numeric IP address. If it is, we know that's good, so we + * prefer that over anything given to us by the destination host. + */ + + DEBUG("URI returned by the destination host: '%s'", uri_out); + + /* Check if the server adress we were originally given is an IP */ + if (virIsNumericIPAddr(dconn->uri->server) != 0) { + /* The server address we were passed from the source isn't + * a numeric IP address. It's likely a host name in text form. + * Therefore, we instead use the URI provided to us by the + * destination server. + */ + DEBUG0("Server address in URI is not numeric-only"); + uri = uri_out; + } else { + /* The server address we were passed from the source *is* a + * numeric IP address. We recreate the URI given to us by the + * destination server, but using this "known good" IP address + * instead. + */ + + DEBUG0("Server address in URI is numeric-only"); + + char *transport; + char *destGivenHostname; + char *destGivenPort; + transport = strtok(uri_out, ":"); + destGivenHostname = strtok(NULL, ":"); + destGivenPort = strtok(NULL, ":"); + DEBUG("URI extracted strings; transport: '%s', " + "destination address: '%s', destination port: '%s'", + transport, destGivenHostname, destGivenPort); + ret = virAsprintf(&new_uri, "%s:%s:%s", + transport, + dconn->uri->server, + destGivenPort); + if (ret < 0) { + /* An error occurred when creating the new URI string, so fall + * back to using the URI provided by the destination server. + */ + uri = uri_out; + } else { + /* String creation worked, so use it */ + uri = new_uri; + } + DEBUG("URI we'll be using: '%s'", uri); + } + } assert (uri != NULL); /* Perform the migration. The driver isn't supposed to return -- 1.7.2.1
On 08/10/2010 07:42 PM, Justin Clift wrote:
+ /* Check if the server adress we were originally given is an IP */
Ugh. "addres" -> "address". I'll fix that before committing (if this passes review and gets ACK'd). + Justin
On Tue, Aug 10, 2010 at 07:42:50PM +1000, Justin Clift wrote:
This fixes a problem where migration fails when a destination host uses a wrongly configured hostname, that doesn't resolve correctly.
If we're given the direct IP address of the destination host, the migration will now use this instead of the resolved host name, allowing things to work.
Addresses BZ # 615941:
https://bugzilla.redhat.com/show_bug.cgi?id=615941 ---
Please note. The DEBUG() and DEBUG0() statements in the code below don't seem to output anywhere. Left them in for now, in the assumption the problem is somewhere in my system configuration.
Happy to remove them entirely if that's a better idea.
src/libvirt.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 3ec5724..05e8d76 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -39,6 +39,7 @@ #include "uuid.h" #include "util.h" #include "memory.h" +#include "network.h"
#ifndef WITH_DRIVER_MODULES # ifdef WITH_TEST @@ -3280,6 +3281,7 @@ virDomainMigrateVersion2 (virDomainPtr domain, unsigned long bandwidth) { virDomainPtr ddomain = NULL; + char *new_uri = NULL; char *uri_out = NULL; char *cookie = NULL; char *dom_xml = NULL; @@ -3332,8 +3334,68 @@ virDomainMigrateVersion2 (virDomainPtr domain, virDispatchError(domain->conn); goto done; } - if (uri_out) - uri = uri_out; /* Did domainMigratePrepare2 change URI? */ + + if (uri_out) { + /* We need to determine the best server address to use for the + * destination host. We potentially have a choice of (up to) two: + * + * a) The server address given to the migration function on the + * source, whose info must be ok otherwise we wouldn't have + * made it this far through the code, or + * + * b) The server address returned to us by the destination + * host itself (may be broken in some situations) + * + * We decide by checking if the address given for a) above is a + * numeric IP address. If it is, we know that's good, so we + * prefer that over anything given to us by the destination host. + */ + + DEBUG("URI returned by the destination host: '%s'", uri_out); + + /* Check if the server adress we were originally given is an IP */ + if (virIsNumericIPAddr(dconn->uri->server) != 0) {
I we use my version of the previous patch, this need to change to if (virSocketParseAddr(dconn->uri->server, NULL, 0) != 0) {
+ /* The server address we were passed from the source isn't + * a numeric IP address. It's likely a host name in text form. + * Therefore, we instead use the URI provided to us by the + * destination server. + */ + DEBUG0("Server address in URI is not numeric-only"); + uri = uri_out; + } else { + /* The server address we were passed from the source *is* a + * numeric IP address. We recreate the URI given to us by the + * destination server, but using this "known good" IP address + * instead. + */ + + DEBUG0("Server address in URI is numeric-only"); + + char *transport; + char *destGivenHostname; + char *destGivenPort; + transport = strtok(uri_out, ":"); + destGivenHostname = strtok(NULL, ":"); + destGivenPort = strtok(NULL, ":");
aye aye aye ... strtok is not thread safe, so we would have to use strtok_r() instead to avoid this major problem. But the simplest (assuming uri_out is a valid URI) is to parse the URI using libxml2 xmlURIPtr uriptr = xmlParseURI(uri_out) then you can extract in an exact fashion the server, port, etc ... as fields i the uriptr structure, once done xmlFreeURI(uriptr);
+ DEBUG("URI extracted strings; transport: '%s', " + "destination address: '%s', destination port: '%s'", + transport, destGivenHostname, destGivenPort); + ret = virAsprintf(&new_uri, "%s:%s:%s", + transport, + dconn->uri->server, + destGivenPort); + if (ret < 0) { + /* An error occurred when creating the new URI string, so fall + * back to using the URI provided by the destination server. + */ + uri = uri_out; + } else { + /* String creation worked, so use it */ + uri = new_uri;
Hum .... shouldn't we free uri_out here since we don't use it ?
+ } + DEBUG("URI we'll be using: '%s'", uri); + } + } assert (uri != NULL);
/* Perform the migration. The driver isn't supposed to return -- 1.7.2.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Aug 10, 2010 at 07:42:50PM +1000, Justin Clift wrote:
This fixes a problem where migration fails when a destination host uses a wrongly configured hostname, that doesn't resolve correctly.
If we're given the direct IP address of the destination host, the migration will now use this instead of the resolved host name, allowing things to work.
Addresses BZ # 615941:
https://bugzilla.redhat.com/show_bug.cgi?id=615941 ---
Please note. The DEBUG() and DEBUG0() statements in the code below don't seem to output anywhere. Left them in for now, in the assumption the problem is somewhere in my system configuration.
Happy to remove them entirely if that's a better idea.
src/libvirt.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 3ec5724..05e8d76 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -39,6 +39,7 @@ #include "uuid.h" #include "util.h" #include "memory.h" +#include "network.h"
#ifndef WITH_DRIVER_MODULES # ifdef WITH_TEST @@ -3280,6 +3281,7 @@ virDomainMigrateVersion2 (virDomainPtr domain, unsigned long bandwidth) { virDomainPtr ddomain = NULL; + char *new_uri = NULL; char *uri_out = NULL; char *cookie = NULL; char *dom_xml = NULL; @@ -3332,8 +3334,68 @@ virDomainMigrateVersion2 (virDomainPtr domain, virDispatchError(domain->conn); goto done; } - if (uri_out) - uri = uri_out; /* Did domainMigratePrepare2 change URI? */ + + if (uri_out) { + /* We need to determine the best server address to use for the + * destination host. We potentially have a choice of (up to) two: + * + * a) The server address given to the migration function on the + * source, whose info must be ok otherwise we wouldn't have + * made it this far through the code, or + * + * b) The server address returned to us by the destination + * host itself (may be broken in some situations) + * + * We decide by checking if the address given for a) above is a + * numeric IP address. If it is, we know that's good, so we + * prefer that over anything given to us by the destination host. + */ + + DEBUG("URI returned by the destination host: '%s'", uri_out); + + /* Check if the server adress we were originally given is an IP */ + if (virIsNumericIPAddr(dconn->uri->server) != 0) { + /* The server address we were passed from the source isn't + * a numeric IP address. It's likely a host name in text form. + * Therefore, we instead use the URI provided to us by the + * destination server. + */ + DEBUG0("Server address in URI is not numeric-only"); + uri = uri_out; + } else { + /* The server address we were passed from the source *is* a + * numeric IP address. We recreate the URI given to us by the + * destination server, but using this "known good" IP address + * instead. + */ + + DEBUG0("Server address in URI is numeric-only"); + + char *transport; + char *destGivenHostname; + char *destGivenPort; + transport = strtok(uri_out, ":"); + destGivenHostname = strtok(NULL, ":"); + destGivenPort = strtok(NULL, ":"); + DEBUG("URI extracted strings; transport: '%s', " + "destination address: '%s', destination port: '%s'", + transport, destGivenHostname, destGivenPort); + ret = virAsprintf(&new_uri, "%s:%s:%s", + transport, + dconn->uri->server, + destGivenPort); + if (ret < 0) { + /* An error occurred when creating the new URI string, so fall + * back to using the URI provided by the destination server. + */ + uri = uri_out; + } else { + /* String creation worked, so use it */ + uri = new_uri; + }
NACK, this doesn't work correctly. First, there is no correlation between the use of hostname & IP address, and whether connectivity will succeed for migration. It just happens to be so in your case, but the opposite could equally be true. Further, the 'dconn->uri' is a URI that the *client* can use to connect to the desination. This does not imply that the source host can connect to the destination host with this URI. eg, consider connecting over an ssh tunnel ssh -L 9000:localhost:22 virsh -c qemu+ssh://root@localhost:9000/system This proposed patch will substitute in 'localhost' instead of the valid IP address Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Aug 10, 2010 at 07:42:49PM +1000, Justin Clift wrote:
--- src/util/network.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/network.h | 2 ++ 2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c index 6e24792..2e0cf9c 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -54,6 +54,43 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { }
/** + * virIsNumericIPAddr: + * @address: a text string containing a host name or IPv4/IPv6 address + * + * Given a text string, determines whether it contains a valid IPv4/IPv6 + * address. + * + * Returns 0 if the given string is an IPv4 or IPv6 address, or non-zero + * for any other condition. + */ +int +virIsNumericIPAddr(const char *address) { + int returnCode; + struct addrinfo hints; + struct addrinfo *res = NULL; + + /* Sanity check we've been given a text string */ + if (address == NULL) + return(-1); + + /* Do a name lookup on the given string, and see what we're given back */ + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_UNSPEC; /* Doesn't matter whether IPv4 or IPv6 */ + hints.ai_socktype = 0; /* Socket type doesn't matter */ + hints.ai_flags = AI_NUMERICHOST; /* Force a numeric IP address check */ + hints.ai_protocol = 0; /* Doesn't matter which protocol */ + returnCode = getaddrinfo (address, NULL, &hints, &res); + if (0 == returnCode) { + /* A result was returned. This means the text string we were + * given is a valid IP address. We now free the result(s) we + * were given, and pass back the return code */ + freeaddrinfo(res); + } + + return returnCode; +}
Hum ... I think that it's better to allow virSocketParseAddr() to take a NULL Addr pointer it's very simple, I'm attaching the patch and then virIsNumericIPAddr(foo) can be replaced by virSocketParseAddr(foo, NULL, 0) it also has the extensibility of being able to cope with IPv4 or IPv6 only if one need, just by adjusting the hint. Am I right ? I see you do more initializations on hints.ai_family but the memset means the two functions are just the same for ai_socktype and ai_protocol, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Aug 10, 2010 at 02:03:27PM +0200, Daniel Veillard wrote:
On Tue, Aug 10, 2010 at 07:42:49PM +1000, Justin Clift wrote:
Hum ... I think that it's better to allow virSocketParseAddr() to take a NULL Addr pointer it's very simple, I'm attaching the patch and then virIsNumericIPAddr(foo) can be replaced by
virSocketParseAddr(foo, NULL, 0)
it also has the extensibility of being able to cope with IPv4 or IPv6 only if one need, just by adjusting the hint. Am I right ? I see you do more initializations on hints.ai_family but the memset means the two functions are just the same for ai_socktype and ai_protocol,
Yep, AI_NUMERICHOST is the only one that really matters here.
diff --git a/src/util/network.c b/src/util/network.c index 6e24792..b17d419 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -56,7 +56,7 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { /** * virSocketParseAddr: * @val: a numeric network address IPv4 or IPv6 - * @addr: where to store the return value. + * @addr: where to store the return value, optional. * @hint: optional hint to pass down to getaddrinfo * * Mostly a wrapper for getaddrinfo() extracting the address storage @@ -70,7 +70,7 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { struct addrinfo hints; struct addrinfo *res = NULL;
- if ((val == NULL) || (addr == NULL)) + if (val == NULL) return(-1);
memset(&hints, 0, sizeof(hints)); @@ -80,7 +80,8 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { }
len = res->ai_addrlen; - memcpy(&addr->stor, res->ai_addr, len); + if (addr != NULL) + memcpy(&addr->stor, res->ai_addr, len);
freeaddrinfo(res); return(len);
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Aug 10, 2010 at 02:02:09PM +0100, Daniel P. Berrange wrote:
On Tue, Aug 10, 2010 at 02:03:27PM +0200, Daniel Veillard wrote:
On Tue, Aug 10, 2010 at 07:42:49PM +1000, Justin Clift wrote:
Hum ... I think that it's better to allow virSocketParseAddr() to take a NULL Addr pointer it's very simple, I'm attaching the patch and then virIsNumericIPAddr(foo) can be replaced by
virSocketParseAddr(foo, NULL, 0)
it also has the extensibility of being able to cope with IPv4 or IPv6 only if one need, just by adjusting the hint. Am I right ? I see you do more initializations on hints.ai_family but the memset means the two functions are just the same for ai_socktype and ai_protocol,
Yep, AI_NUMERICHOST is the only one that really matters here. [...]
ACK
Okay, pushed, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Justin Clift