[libvirt] [PATCH 00/11] Misc fixes and changes related to virSocket APIs
In working on the DTrace patches I needed to be able to format a struct sockaddr into a string easily. The virSocketFormatAddr API was close to what I needed, but couldn't handle including the port number, nor UNIX domain sockets. This patch series addresses that limitation. Along the way it fixes miscellaneous bugs with the virSocket APis, adds a test suite, removes & bans all use of inet_* functions and replaces the addrToString methods used in SASL code and simplifies some nwfilter code using virSocket.
If getnameinfo() with NI_NUMERICHOST set fails, there are no grounds to expect inet_ntop to succeed, since these calls are functionally equivalent. Remove useless inet_ntop code in the getnameinfo() error path. * daemon/remote.c, src/remote/remote_driver.c: Remove calls to inet_ntop --- daemon/remote.c | 20 +++----------------- src/remote/remote_driver.c | 20 +++----------------- 2 files changed, 6 insertions(+), 34 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 75df9b5..ae7a2d3 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3653,23 +3653,9 @@ static char *addrToString(remote_error *rerr, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - char ip[INET6_ADDRSTRLEN]; - void *rawaddr; - - if (sa->sa_family == AF_INET) - rawaddr = &((struct sockaddr_in *)sa)->sin_addr; - else - rawaddr = &((struct sockaddr_in6 *)sa)->sin6_addr; - - if (inet_ntop(sa->sa_family, rawaddr, ip, sizeof ip)) { - remoteDispatchFormatError(rerr, - _("Cannot resolve address %s: %s"), - ip, gai_strerror(err)); - } else { - remoteDispatchFormatError(rerr, - _("Cannot resolve address: %s"), - gai_strerror(err)); - } + remoteDispatchFormatError(rerr, + _("Cannot convert socket address to string: %s"), + gai_strerror(err)); return NULL; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 37c37ef..38e2d55 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6910,23 +6910,9 @@ static char *addrToString(struct sockaddr_storage *ss, socklen_t salen) host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - char ip[INET6_ADDRSTRLEN]; - void *rawaddr; - - if (sa->sa_family == AF_INET) - rawaddr = &((struct sockaddr_in *)sa)->sin_addr; - else - rawaddr = &((struct sockaddr_in6 *)sa)->sin6_addr; - - if (inet_ntop(sa->sa_family, rawaddr, ip, sizeof ip)) { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address %s: %s"), - ip, gai_strerror(err)); - } else { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address: %s"), - gai_strerror(err)); - } + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot convert socket address to string: %s"), + gai_strerror(err)); return NULL; } -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
If getnameinfo() with NI_NUMERICHOST set fails, there are no grounds to expect inet_ntop to succeed, since these calls are functionally equivalent. Remove useless inet_ntop code in the getnameinfo() error path.
* daemon/remote.c, src/remote/remote_driver.c: Remove calls to inet_ntop --- daemon/remote.c | 20 +++----------------- src/remote/remote_driver.c | 20 +++----------------- 2 files changed, 6 insertions(+), 34 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/21/2010 02:17 PM, Daniel P. Berrange wrote:
If getnameinfo() with NI_NUMERICHOST set fails, there are no grounds to expect inet_ntop to succeed, since these calls are functionally equivalent. Remove useless inet_ntop code in the getnameinfo() error path.
* daemon/remote.c, src/remote/remote_driver.c: Remove calls to inet_ntop --- daemon/remote.c | 20 +++----------------- src/remote/remote_driver.c | 20 +++----------------- 2 files changed, 6 insertions(+), 34 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 75df9b5..ae7a2d3 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3653,23 +3653,9 @@ static char *addrToString(remote_error *rerr, host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - char ip[INET6_ADDRSTRLEN]; - void *rawaddr; - - if (sa->sa_family == AF_INET) - rawaddr =&((struct sockaddr_in *)sa)->sin_addr; - else - rawaddr =&((struct sockaddr_in6 *)sa)->sin6_addr; - - if (inet_ntop(sa->sa_family, rawaddr, ip, sizeof ip)) { - remoteDispatchFormatError(rerr, - _("Cannot resolve address %s: %s"), - ip, gai_strerror(err)); - } else { - remoteDispatchFormatError(rerr, - _("Cannot resolve address: %s"), - gai_strerror(err)); - } + remoteDispatchFormatError(rerr, + _("Cannot convert socket address to string: %s"), + gai_strerror(err)); return NULL; }
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 37c37ef..38e2d55 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6910,23 +6910,9 @@ static char *addrToString(struct sockaddr_storage *ss, socklen_t salen) host, sizeof(host), port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - char ip[INET6_ADDRSTRLEN]; - void *rawaddr; - - if (sa->sa_family == AF_INET) - rawaddr =&((struct sockaddr_in *)sa)->sin_addr; - else - rawaddr =&((struct sockaddr_in6 *)sa)->sin6_addr; - - if (inet_ntop(sa->sa_family, rawaddr, ip, sizeof ip)) { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address %s: %s"), - ip, gai_strerror(err)); - } else { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot resolve address: %s"), - gai_strerror(err)); - } + remoteError(VIR_ERR_UNKNOWN_HOST, + _("Cannot convert socket address to string: %s"), + gai_strerror(err)); return NULL; }
Aren't you removing these functions in PATCH 8/11 anyway?
Some operations on socket addresses need to know the length of the sockaddr struct for the particular address family. This info was being discarded when passing around virSocketAddr instances. Turn it from a union into a struct containing union+socklen_t fields, so length is always kept around. * src/util/network.h: Add socklen_t field to virSocketAddr * src/util/network.c, src/network/bridge_driver.c, src/conf/domain_conf.c: Update to take account of new struct definition. --- src/conf/domain_conf.c | 2 +- src/network/bridge_driver.c | 10 +++--- src/util/network.c | 60 ++++++++++++++++++++++-------------------- src/util/network.h | 12 +++++--- 4 files changed, 45 insertions(+), 39 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6486f9c..bd9f425 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2522,7 +2522,7 @@ virDomainChrDefParseTargetXML(virCapsPtr caps, goto error; } - if (def->target.addr->stor.ss_family != AF_INET) { + if (def->target.addr->data.stor.ss_family != AF_INET) { virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("guestfwd channel only supports " "IPv4 addresses")); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 01d2171..6323fa5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1060,14 +1060,14 @@ static int networkCheckRouteCollision(virNetworkObjPtr network) goto error; } - if (inaddress.stor.ss_family != AF_INET || - innetmask.stor.ss_family != AF_INET) { + if (inaddress.data.stor.ss_family != AF_INET || + innetmask.data.stor.ss_family != AF_INET) { /* Only support collision check for IPv4 */ goto out; } - net_dest = (inaddress.inet4.sin_addr.s_addr & - innetmask.inet4.sin_addr.s_addr); + net_dest = (inaddress.data.inet4.sin_addr.s_addr & + innetmask.data.inet4.sin_addr.s_addr); /* Read whole routing table into memory */ if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) @@ -1120,7 +1120,7 @@ static int networkCheckRouteCollision(virNetworkObjPtr network) addr_val &= mask_val; if ((net_dest == addr_val) && - (innetmask.inet4.sin_addr.s_addr == mask_val)) { + (innetmask.data.inet4.sin_addr.s_addr == mask_val)) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("Network %s/%s is already in use by " "interface %s"), diff --git a/src/util/network.c b/src/util/network.c index 17aa746..b8107f7 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -27,10 +27,10 @@ static int getIPv4Addr(virSocketAddrPtr addr, virIPv4AddrPtr tab) { unsigned long val; int i; - if ((addr == NULL) || (tab == NULL) || (addr->stor.ss_family != AF_INET)) + if ((addr == NULL) || (tab == NULL) || (addr->data.stor.ss_family != AF_INET)) return(-1); - val = ntohl(addr->inet4.sin_addr.s_addr); + val = ntohl(addr->data.inet4.sin_addr.s_addr); for (i = 0;i < 4;i++) { (*tab)[3 - i] = val & 0xFF; @@ -43,12 +43,12 @@ static int getIPv4Addr(virSocketAddrPtr addr, virIPv4AddrPtr tab) { static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { int i; - if ((addr == NULL) || (tab == NULL) || (addr->stor.ss_family != AF_INET6)) + if ((addr == NULL) || (tab == NULL) || (addr->data.stor.ss_family != AF_INET6)) return(-1); for (i = 0;i < 8;i++) { - (*tab)[i] = ((addr->inet6.sin6_addr.s6_addr[2 * i] << 8) | - addr->inet6.sin6_addr.s6_addr[2 * i + 1]); + (*tab)[i] = ((addr->data.inet6.sin6_addr.s6_addr[2 * i] << 8) | + addr->data.inet6.sin6_addr.s6_addr[2 * i + 1]); } return(0); @@ -81,8 +81,10 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { } len = res->ai_addrlen; - if (addr != NULL) - memcpy(&addr->stor, res->ai_addr, len); + if (addr != NULL) { + memcpy(&addr->data.stor, res->ai_addr, len); + addr->len = res->ai_addrlen; + } freeaddrinfo(res); return(len); @@ -133,14 +135,14 @@ virSocketFormatAddr(virSocketAddrPtr addr) { if (addr == NULL) return NULL; - if (addr->stor.ss_family == AF_INET) { + if (addr->data.stor.ss_family == AF_INET) { outlen = INET_ADDRSTRLEN; - inaddr = &addr->inet4.sin_addr; + inaddr = &addr->data.inet4.sin_addr; } - else if (addr->stor.ss_family == AF_INET6) { + else if (addr->data.stor.ss_family == AF_INET6) { outlen = INET6_ADDRSTRLEN; - inaddr = &addr->inet6.sin6_addr; + inaddr = &addr->data.inet6.sin6_addr; } else { @@ -150,7 +152,7 @@ virSocketFormatAddr(virSocketAddrPtr addr) { if (VIR_ALLOC_N(out, outlen) < 0) return NULL; - if (inet_ntop(addr->stor.ss_family, inaddr, out, outlen) == NULL) { + if (inet_ntop(addr->data.stor.ss_family, inaddr, out, outlen) == NULL) { VIR_FREE(out); return NULL; } @@ -174,12 +176,12 @@ virSocketSetPort(virSocketAddrPtr addr, int port) { port = htons(port); - if(addr->stor.ss_family == AF_INET) { - addr->inet4.sin_port = port; + if(addr->data.stor.ss_family == AF_INET) { + addr->data.inet4.sin_port = port; } - else if(addr->stor.ss_family == AF_INET6) { - addr->inet6.sin6_port = port; + else if(addr->data.stor.ss_family == AF_INET6) { + addr->data.inet6.sin6_port = port; } else { @@ -201,12 +203,12 @@ virSocketGetPort(virSocketAddrPtr addr) { if (addr == NULL) return -1; - if(addr->stor.ss_family == AF_INET) { - return ntohs(addr->inet4.sin_port); + if(addr->data.stor.ss_family == AF_INET) { + return ntohs(addr->data.inet4.sin_port); } - else if(addr->stor.ss_family == AF_INET6) { - return ntohs(addr->inet6.sin6_port); + else if(addr->data.stor.ss_family == AF_INET6) { + return ntohs(addr->data.inet6.sin6_port); } return -1; @@ -245,14 +247,14 @@ int virSocketCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, if ((addr1 == NULL) || (addr2 == NULL) || (netmask == NULL)) return(-1); - if ((addr1->stor.ss_family != addr2->stor.ss_family) || - (addr1->stor.ss_family != netmask->stor.ss_family)) + if ((addr1->data.stor.ss_family != addr2->data.stor.ss_family) || + (addr1->data.stor.ss_family != netmask->data.stor.ss_family)) return(-1); if (virSocketAddrIsNetmask(netmask) != 0) return(-1); - if (addr1->stor.ss_family == AF_INET) { + if (addr1->data.stor.ss_family == AF_INET) { virIPv4Addr t1, t2, tm; if ((getIPv4Addr(addr1, &t1) < 0) || @@ -265,7 +267,7 @@ int virSocketCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, return(0); } - } else if (addr1->stor.ss_family == AF_INET) { + } else if (addr1->data.stor.ss_family == AF_INET) { virIPv6Addr t1, t2, tm; if ((getIPv6Addr(addr1, &t1) < 0) || @@ -301,10 +303,10 @@ int virSocketGetRange(virSocketAddrPtr start, virSocketAddrPtr end) { if ((start == NULL) || (end == NULL)) return(-1); - if (start->stor.ss_family != end->stor.ss_family) + if (start->data.stor.ss_family != end->data.stor.ss_family) return(-1); - if (start->stor.ss_family == AF_INET) { + if (start->data.stor.ss_family == AF_INET) { virIPv4Addr t1, t2; if ((getIPv4Addr(start, &t1) < 0) || @@ -319,7 +321,7 @@ int virSocketGetRange(virSocketAddrPtr start, virSocketAddrPtr end) { if (ret < 0) return(-1); ret++; - } else if (start->stor.ss_family == AF_INET6) { + } else if (start->data.stor.ss_family == AF_INET6) { virIPv6Addr t1, t2; if ((getIPv6Addr(start, &t1) < 0) || @@ -355,7 +357,7 @@ int virSocketGetNumNetmaskBits(const virSocketAddrPtr netmask) int i, j; int c = 0; - if (netmask->stor.ss_family == AF_INET) { + if (netmask->data.stor.ss_family == AF_INET) { virIPv4Addr tm; uint8_t bit; @@ -389,7 +391,7 @@ int virSocketGetNumNetmaskBits(const virSocketAddrPtr netmask) } return c; - } else if (netmask->stor.ss_family == AF_INET6) { + } else if (netmask->data.stor.ss_family == AF_INET6) { virIPv6Addr tm; uint16_t bit; diff --git a/src/util/network.h b/src/util/network.h index 5307c8c..ef92c9b 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -17,11 +17,15 @@ # include <sys/socket.h> # include <netdb.h> -typedef union { - struct sockaddr_storage stor; - struct sockaddr_in inet4; - struct sockaddr_in6 inet6; +typedef struct { + union { + struct sockaddr_storage stor; + struct sockaddr_in inet4; + struct sockaddr_in6 inet6; + } data; + socklen_t len; } virSocketAddr; + typedef virSocketAddr *virSocketAddrPtr; int virSocketParseAddr (const char *val, -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
Some operations on socket addresses need to know the length of the sockaddr struct for the particular address family. This info was being discarded when passing around virSocketAddr instances. Turn it from a union into a struct containing union+socklen_t fields, so length is always kept around.
* src/util/network.h: Add socklen_t field to virSocketAddr * src/util/network.c, src/network/bridge_driver.c, src/conf/domain_conf.c: Update to take account of new struct definition. --- src/conf/domain_conf.c | 2 +- src/network/bridge_driver.c | 10 +++--- src/util/network.c | 60 ++++++++++++++++++++++-------------------- src/util/network.h | 12 +++++--- 4 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/src/util/network.h b/src/util/network.h index 5307c8c..ef92c9b 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -17,11 +17,15 @@ # include<sys/socket.h> # include<netdb.h>
-typedef union { - struct sockaddr_storage stor; - struct sockaddr_in inet4; - struct sockaddr_in6 inet6; +typedef struct { + union { + struct sockaddr_storage stor; + struct sockaddr_in inet4; + struct sockaddr_in6 inet6; + } data; + socklen_t len; } virSocketAddr;
ACK; and the rest of the patch is compiler-enforced fallout. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The virSocketParseAddr function was accepting any AF_* constant and using that to set the ai_flags field in struct addrinfo. This is invalid, since address familys must go in the ai_family field of the struct. * src/util/network.c: Fix handling of address family * src/conf/network_conf.c, src/network/bridge_driver.c: Pass AF_UNSPEC instead of relying on it being 0. --- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 4 ++-- src/network/bridge_driver.c | 4 ++-- src/util/network.c | 7 ++++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd9f425..53c8d09 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2515,7 +2515,7 @@ virDomainChrDefParseTargetXML(virCapsPtr caps, goto error; } - if (virSocketParseAddr(addrStr, def->target.addr, 0) < 0) { + if (virSocketParseAddr(addrStr, def->target.addr, AF_UNSPEC) < 0) { virDomainReportError(VIR_ERR_XML_ERROR, _("%s is not a valid address"), addrStr); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ddef790..f209dad 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -243,7 +243,7 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, continue; } - if (virSocketParseAddr(start, &saddr, 0) < 0) { + if (virSocketParseAddr(start, &saddr, AF_UNSPEC) < 0) { virNetworkReportError(VIR_ERR_XML_ERROR, _("cannot parse dhcp start address '%s'"), start); @@ -252,7 +252,7 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, cur = cur->next; continue; } - if (virSocketParseAddr(end, &eaddr, 0) < 0) { + if (virSocketParseAddr(end, &eaddr, AF_UNSPEC) < 0) { virNetworkReportError(VIR_ERR_XML_ERROR, _("cannot parse dhcp end address '%s'"), end); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6323fa5..ac91c57 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1046,14 +1046,14 @@ static int networkCheckRouteCollision(virNetworkObjPtr network) if (!network->def->ipAddress || !network->def->netmask) return 0; - if (virSocketParseAddr(network->def->ipAddress, &inaddress, 0) < 0) { + if (virSocketParseAddr(network->def->ipAddress, &inaddress, AF_UNSPEC) < 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse IP address '%s'"), network->def->ipAddress); goto error; } - if (virSocketParseAddr(network->def->netmask, &innetmask, 0) < 0) { + if (virSocketParseAddr(network->def->netmask, &innetmask, AF_UNSPEC) < 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse netmask '%s'"), network->def->netmask); diff --git a/src/util/network.c b/src/util/network.c index b8107f7..4cb6bfe 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -58,7 +58,7 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { * virSocketParseAddr: * @val: a numeric network address IPv4 or IPv6 * @addr: where to store the return value, optional. - * @hint: optional hint to pass down to getaddrinfo + * @family: address family to pass down to getaddrinfo * * Mostly a wrapper for getaddrinfo() extracting the address storage * from the numeric string like 1.2.3.4 or 2001:db8:85a3:0:0:8a2e:370:7334 @@ -66,7 +66,7 @@ static int getIPv6Addr(virSocketAddrPtr addr, virIPv6AddrPtr tab) { * Returns the length of the network address or -1 in case of error. */ int -virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { +virSocketParseAddr(const char *val, virSocketAddrPtr addr, int family) { int len; struct addrinfo hints; struct addrinfo *res = NULL; @@ -75,7 +75,8 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { return(-1); memset(&hints, 0, sizeof(hints)); - hints.ai_flags = AI_NUMERICHOST | hint; + hints.ai_family = family; + hints.ai_flags = AI_NUMERICHOST; if ((getaddrinfo(val, NULL, &hints, &res) != 0) || (res == NULL)) { return(-1); } -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The virSocketParseAddr function was accepting any AF_* constant and using that to set the ai_flags field in struct addrinfo. This is invalid, since address familys must go in the ai_family
s/familys/families/
field of the struct.
* src/util/network.c: Fix handling of address family * src/conf/network_conf.c, src/network/bridge_driver.c: Pass AF_UNSPEC instead of relying on it being 0.
+virSocketParseAddr(const char *val, virSocketAddrPtr addr, int family) { int len; struct addrinfo hints; struct addrinfo *res = NULL; @@ -75,7 +75,8 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int hint) { return(-1);
memset(&hints, 0, sizeof(hints)); - hints.ai_flags = AI_NUMERICHOST | hint; + hints.ai_family = family; + hints.ai_flags = AI_NUMERICHOST;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
There was a typo in the IPv6 path of virSocketCheckNetmask which caused it to never execute. * src/util/network.c: s/AF_INET/AF_INET6/ in virSocketCheckNetmask --- src/util/network.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/network.c b/src/util/network.c index 4cb6bfe..de22ded 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -268,7 +268,7 @@ int virSocketCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, return(0); } - } else if (addr1->data.stor.ss_family == AF_INET) { + } else if (addr1->data.stor.ss_family == AF_INET6) { virIPv6Addr t1, t2, tm; if ((getIPv6Addr(addr1, &t1) < 0) || -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
There was a typo in the IPv6 path of virSocketCheckNetmask which caused it to never execute.
* src/util/network.c: s/AF_INET/AF_INET6/ in virSocketCheckNetmask --- src/util/network.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c index 4cb6bfe..de22ded 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -268,7 +268,7 @@ int virSocketCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, return(0); }
- } else if (addr1->data.stor.ss_family == AF_INET) { + } else if (addr1->data.stor.ss_family == AF_INET6) { virIPv6Addr t1, t2, tm;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 21, 2010 at 07:17:18PM +0100, Daniel P. Berrange wrote:
There was a typo in the IPv6 path of virSocketCheckNetmask which caused it to never execute.
* src/util/network.c: s/AF_INET/AF_INET6/ in virSocketCheckNetmask --- src/util/network.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c index 4cb6bfe..de22ded 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -268,7 +268,7 @@ int virSocketCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, return(0); }
- } else if (addr1->data.stor.ss_family == AF_INET) { + } else if (addr1->data.stor.ss_family == AF_INET6) { virIPv6Addr t1, t2, tm;
if ((getIPv6Addr(addr1, &t1) < 0) ||
ACK :-) 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/
The nwIPAddress was simply a wrapper about virSocketAddr. Just use the latter directly, removing all the extra field de-references from code & helper APIs for parsing/formatting. Also remove all the redundant casts from strong types to void * and then immediately back to strong types. * src/conf/nwfilter_conf.h: Remove nwIPAddress * src/conf/nwfilter_conf.c, src/nwfilter/nwfilter_ebiptables_driver.c: Update to use virSocketAddr and remove void * casts. --- src/conf/nwfilter_conf.c | 103 +++++++++-------------------- src/conf/nwfilter_conf.h | 9 +-- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- 3 files changed, 34 insertions(+), 82 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 40fbf5e..6fd07d4 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1325,26 +1325,6 @@ virNWMACAddressParser(const char *input, } -static bool -virNWIPv4AddressParser(const char *input, - nwIPAddressPtr output) -{ - if (virSocketParseIpv4Addr(input, &output->addr) == -1) - return 0; - return 1; -} - - -static bool -virNWIPv6AddressParser(const char *input, - nwIPAddressPtr output) -{ - if (virSocketParseIpv6Addr(input, &output->addr) == -1) - return 0; - return 1; -} - - static int virNWFilterRuleDetailsParse(xmlNodePtr node, virNWFilterRuleDefPtr nwf, @@ -1359,11 +1339,10 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, nwItemDesc *item; int int_val; unsigned int uint_val; - void *storage_ptr; union data data; valueValidator validator; char *match = virXMLPropString(node, "match"); - nwIPAddress ipaddr; + virSocketAddr ipaddr; int base; if (match && STREQ(match, "no")) @@ -1385,8 +1364,6 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, if (STRPREFIX(prop, "$")) { flags_set |= NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR; - storage_ptr = NULL; - if (virNWFilterRuleDefAddVar(nwf, item, &prop[1])) @@ -1411,10 +1388,9 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, case DATATYPE_UINT8_HEX: base = 16; case DATATYPE_UINT8: - storage_ptr = &item->u.u8; if (virStrToLong_ui(prop, NULL, base, &uint_val) >= 0) { if (uint_val <= 0xff) { - *(uint8_t *)storage_ptr = uint_val; + item->u.u8 = uint_val; found = 1; data.ui = uint_val; } else @@ -1426,10 +1402,9 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, case DATATYPE_UINT16_HEX: base = 16; case DATATYPE_UINT16: - storage_ptr = &item->u.u16; if (virStrToLong_ui(prop, NULL, base, &uint_val) >= 0) { if (uint_val <= 0xffff) { - *(uint16_t *)storage_ptr = uint_val; + item->u.u16 = uint_val; found = 1; data.ui = uint_val; } else @@ -1439,43 +1414,38 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, break; case DATATYPE_IPADDR: - storage_ptr = &item->u.ipaddr; - if (!virNWIPv4AddressParser(prop, - (nwIPAddressPtr)storage_ptr)) { + if (virSocketParseIpv4Addr(prop, + &item->u.ipaddr) < 0) rc = -1; - } found = 1; break; case DATATYPE_IPMASK: - storage_ptr = &item->u.u8; if (virStrToLong_ui(prop, NULL, 10, &uint_val) == 0) { if (uint_val <= 32) { if (!validator) - *(uint8_t *)storage_ptr = - (uint8_t)uint_val; + item->u.u8 = (uint8_t)uint_val; found = 1; data.ui = uint_val; } else rc = -1; } else { - if (virNWIPv4AddressParser(prop, &ipaddr)) { - int_val = virSocketGetNumNetmaskBits( - &ipaddr.addr); + if (virSocketParseIpv4Addr(prop, &ipaddr) < 0) { + rc = -1; + } else { + int_val = virSocketGetNumNetmaskBits(&ipaddr); if (int_val >= 0) - *(uint8_t *)storage_ptr = int_val; + item->u.u8 = int_val; else rc = -1; found = 1; - } else - rc = -1; + } } break; case DATATYPE_MACADDR: - storage_ptr = &item->u.macaddr; if (!virNWMACAddressParser(prop, - (nwMACAddressPtr)storage_ptr)) { + &item->u.macaddr)) { rc = -1; } found = 1; @@ -1483,46 +1453,41 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, case DATATYPE_MACMASK: validator = checkMACMask; - storage_ptr = &item->u.macaddr; if (!virNWMACAddressParser(prop, - (nwMACAddressPtr)storage_ptr)) { + &item->u.macaddr)) { rc = -1; } - data.v = storage_ptr; + data.v = &item->u.macaddr; found = 1; break; case DATATYPE_IPV6ADDR: - storage_ptr = &item->u.ipaddr; - if (!virNWIPv6AddressParser(prop, - (nwIPAddressPtr)storage_ptr)) { + if (virSocketParseIpv6Addr(prop, + &item->u.ipaddr) < 0) rc = -1; - } found = 1; break; case DATATYPE_IPV6MASK: - storage_ptr = &item->u.u8; if (virStrToLong_ui(prop, NULL, 10, &uint_val) == 0) { if (uint_val <= 128) { if (!validator) - *(uint8_t *)storage_ptr = - (uint8_t)uint_val; + item->u.u8 = (uint8_t)uint_val; found = 1; data.ui = uint_val; } else rc = -1; } else { - if (virNWIPv6AddressParser(prop, &ipaddr)) { - int_val = virSocketGetNumNetmaskBits( - &ipaddr.addr); + if (virSocketParseIpv6Addr(prop, &ipaddr) < 0) { + rc = -1; + } else { + int_val = virSocketGetNumNetmaskBits(&ipaddr); if (int_val >= 0) - *(uint8_t *)storage_ptr = int_val; + item->u.u8 = int_val; else rc = -1; found = 1; - } else - rc = -1; + } } break; @@ -2642,10 +2607,9 @@ virNWFilterPoolObjDeleteDef(virNWFilterPoolObjPtr pool) static void -virNWIPAddressFormat(virBufferPtr buf, nwIPAddressPtr ipaddr) +virNWIPAddressFormat(virBufferPtr buf, virSocketAddrPtr ipaddr) { - virSocketAddrPtr addr = &ipaddr->addr; - char *output = virSocketFormatAddr(addr); + char *output = virSocketFormatAddr(ipaddr); if (output) { virBufferVSprintf(buf, "%s", output); @@ -2674,7 +2638,6 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, while (att[i].name) { item = (nwItemDesc *)((char *)def + att[i].dataIdx); enum virNWFilterEntryItemFlags flags = item->flags; - void *storage_ptr; if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) { if (!typeShown) { virBufferVSprintf(buf, " <%s", type); @@ -2725,33 +2688,29 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, case DATATYPE_IPV6MASK: // display all masks in CIDR format case DATATYPE_UINT8: - storage_ptr = &item->u.u8; virBufferVSprintf(buf, asHex ? "0x%x" : "%d", - *(uint8_t *)storage_ptr); + item->u.u8); break; case DATATYPE_UINT16_HEX: asHex = true; case DATATYPE_UINT16: - storage_ptr = &item->u.u16; virBufferVSprintf(buf, asHex ? "0x%x" : "%d", - *(uint16_t *)storage_ptr); + item->u.u16); break; case DATATYPE_IPADDR: case DATATYPE_IPV6ADDR: - storage_ptr = &item->u.ipaddr; virNWIPAddressFormat(buf, - (nwIPAddressPtr)storage_ptr); + &item->u.ipaddr); break; case DATATYPE_MACMASK: case DATATYPE_MACADDR: - storage_ptr = &item->u.macaddr; for (j = 0; j < 6; j++) virBufferVSprintf(buf, "%02x%s", - ((nwMACAddressPtr)storage_ptr)->addr[j], - (j < 5) ? ":" : ""); + item->u.macaddr.addr[j], + (j < 5) ? ":" : ""); break; case DATATYPE_STRINGCOPY: diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4274b1a..4d76c4c 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -110,13 +110,6 @@ struct _nwMACAddress { }; -typedef struct _nwIPAddress nwIPAddress; -typedef nwIPAddress *nwIPAddressPtr; -struct _nwIPAddress { - virSocketAddr addr; -}; - - typedef struct _nwItemDesc nwItemDesc; typedef nwItemDesc *nwItemDescPtr; struct _nwItemDesc { @@ -125,7 +118,7 @@ struct _nwItemDesc { enum attrDatatype datatype; union { nwMACAddress macaddr; - nwIPAddress ipaddr; + virSocketAddr ipaddr; uint8_t u8; uint16_t u16; char protocolID[10]; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 3eb1368..caa37cb 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -189,7 +189,7 @@ _printDataType(virNWFilterHashTablePtr vars, switch (item->datatype) { case DATATYPE_IPADDR: - data = virSocketFormatAddr(&item->u.ipaddr.addr); + data = virSocketFormatAddr(&item->u.ipaddr); if (!data) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal IPv4 address representation " @@ -206,7 +206,7 @@ _printDataType(virNWFilterHashTablePtr vars, break; case DATATYPE_IPV6ADDR: - data = virSocketFormatAddr(&item->u.ipaddr.addr); + data = virSocketFormatAddr(&item->u.ipaddr); if (!data) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal IPv6 address representation " -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The nwIPAddress was simply a wrapper about virSocketAddr. Just use the latter directly, removing all the extra field de-references from code& helper APIs for parsing/formatting.
Also remove all the redundant casts from strong types to void * and then immediately back to strong types.
* src/conf/nwfilter_conf.h: Remove nwIPAddress * src/conf/nwfilter_conf.c, src/nwfilter/nwfilter_ebiptables_driver.c: Update to use virSocketAddr and remove void * casts. --- src/conf/nwfilter_conf.c | 103 +++++++++-------------------- src/conf/nwfilter_conf.h | 9 +-- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- 3 files changed, 34 insertions(+), 82 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 40fbf5e..6fd07d4 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1325,26 +1325,6 @@ virNWMACAddressParser(const char *input, }
-static bool -virNWIPv4AddressParser(const char *input, - nwIPAddressPtr output) -{ - if (virSocketParseIpv4Addr(input,&output->addr) == -1) - return 0; - return 1;
Good change. I'd rather see functions return true/false than 0/1 when labeled as bool, so I'm glad to see this go.
- *(uint8_t *)storage_ptr = - (uint8_t)uint_val; + item->u.u8 = (uint8_t)uint_val;
Technically, the (uint8_t) cast isn't needed, either, since in C, assignment auto-narrows. But I don't know if it might trigger a picky compiler warning and thus a -Werror failure, so it's probably okay to leave it in.
-typedef struct _nwIPAddress nwIPAddress; -typedef nwIPAddress *nwIPAddressPtr; -struct _nwIPAddress { - virSocketAddr addr; -};
I suppose nwfilter originally wrapped this, in case it needs to add another member. But if that is the case, then we can probably add that member directly in virSocketAddr, as it would probably be useful elsewhere. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 21, 2010 at 02:02:44PM -0600, Eric Blake wrote:
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The nwIPAddress was simply a wrapper about virSocketAddr. Just use the latter directly, removing all the extra field de-references from code& helper APIs for parsing/formatting.
Also remove all the redundant casts from strong types to void * and then immediately back to strong types.
* src/conf/nwfilter_conf.h: Remove nwIPAddress * src/conf/nwfilter_conf.c, src/nwfilter/nwfilter_ebiptables_driver.c: Update to use virSocketAddr and remove void * casts. --- src/conf/nwfilter_conf.c | 103 +++++++++-------------------- src/conf/nwfilter_conf.h | 9 +-- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- 3 files changed, 34 insertions(+), 82 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 40fbf5e..6fd07d4 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1325,26 +1325,6 @@ virNWMACAddressParser(const char *input, }
-static bool -virNWIPv4AddressParser(const char *input, - nwIPAddressPtr output) -{ - if (virSocketParseIpv4Addr(input,&output->addr) == -1) - return 0; - return 1;
Good change. I'd rather see functions return true/false than 0/1 when labeled as bool, so I'm glad to see this go.
- *(uint8_t *)storage_ptr = - (uint8_t)uint_val; + item->u.u8 = (uint8_t)uint_val;
Technically, the (uint8_t) cast isn't needed, either, since in C, assignment auto-narrows. But I don't know if it might trigger a picky compiler warning and thus a -Werror failure, so it's probably okay to leave it in.
Yes, IIRC this one was a case where I needed a cast to avoid a compiler warning.
-typedef struct _nwIPAddress nwIPAddress; -typedef nwIPAddress *nwIPAddressPtr; -struct _nwIPAddress { - virSocketAddr addr; -};
I suppose nwfilter originally wrapped this, in case it needs to add another member. But if that is the case, then we can probably add that member directly in virSocketAddr, as it would probably be useful elsewhere.
I was thinking that nwIPAddress just originally pre-dated the virSocket struct existing, either way, I think its now redundant. 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 :|
The getnameinfo() function is more flexible than inet_ntop() avoiding the need to if/else the code based on socket family. Also make it support UNIX socket addrs and allow inclusion of a port (service) address. Finally do proper error reporting via normal APIs. * src/conf/domain_conf.c, src/nwfilter/nwfilter_ebiptables_driver.c, src/qemu/qemu_conf.c: Fix error handling with virSocketFormat * src/util/network.c: Rewrite virSocketFormat to use getnameinfo and cope with UNIX socket addrs. --- po/POTFILES.in | 1 + src/conf/domain_conf.c | 5 +-- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 12 +---- src/qemu/qemu_conf.c | 2 + src/util/network.c | 82 ++++++++++++++++++++++------- src/util/network.h | 5 ++ 7 files changed, 75 insertions(+), 33 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index e30fea0..60ba68b 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -83,6 +83,7 @@ src/util/hostusb.c src/util/interface.c src/util/json.c src/util/macvtap.c +src/util/network.c src/util/pci.c src/util/processinfo.c src/util/stats_linux.c diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 53c8d09..945c1f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5960,11 +5960,8 @@ virDomainChrDefFormat(virBufferPtr buf, } const char *addr = virSocketFormatAddr(def->target.addr); - if (addr == NULL) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to format guestfwd address")); + if (addr == NULL) return -1; - } virBufferVSprintf(buf, " address='%s' port='%d'", addr, port); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b1c482..5c7f929 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -485,6 +485,7 @@ virFree; virSocketAddrIsNetmask; virSocketCheckNetmask; virSocketFormatAddr; +virSocketFormatAddrFull; virSocketGetPort; virSocketGetRange; virSocketParseAddr; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index caa37cb..21b1b51 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -190,12 +190,8 @@ _printDataType(virNWFilterHashTablePtr vars, switch (item->datatype) { case DATATYPE_IPADDR: data = virSocketFormatAddr(&item->u.ipaddr); - if (!data) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("internal IPv4 address representation " - "is bad")); + if (!data) return 1; - } if (snprintf(buf, bufsize, "%s", data) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer too small for IP address")); @@ -207,12 +203,8 @@ _printDataType(virNWFilterHashTablePtr vars, case DATATYPE_IPV6ADDR: data = virSocketFormatAddr(&item->u.ipaddr); - if (!data) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("internal IPv6 address representation " - "is bad")); + if (!data) return 1; - } if (snprintf(buf, bufsize, "%s", data) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6719578..e2c67a3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4727,6 +4727,8 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG(devstr); char *addr = virSocketFormatAddr(channel->target.addr); + if (!addr) + goto error; int port = virSocketGetPort(channel->target.addr); ADD_ARG_LIT("-netdev"); diff --git a/src/util/network.c b/src/util/network.c index de22ded..7c6ced9 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -13,6 +13,13 @@ #include "memory.h" #include "network.h" +#include "util.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +#define virSocketError(code, ...) \ + virReportErrorHelper(NULL, VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) /* * Helpers to extract the IP arrays from the virSocketAddrPtr @@ -129,38 +136,75 @@ virSocketParseIpv6Addr(const char *val, virSocketAddrPtr addr) { */ char * virSocketFormatAddr(virSocketAddrPtr addr) { - char *out; - size_t outlen; - void *inaddr; + return virSocketFormatAddrFull(addr, false, NULL); +} - if (addr == NULL) - return NULL; - if (addr->data.stor.ss_family == AF_INET) { - outlen = INET_ADDRSTRLEN; - inaddr = &addr->data.inet4.sin_addr; - } +/* + * virSocketFormatAddr: + * @addr: an initialized virSocketAddrPtr + * @withService: if true, then service info is appended + * @separator: separator between hostname & service. + * + * Returns a string representation of the given address + * Returns NULL on any error + * Caller must free the returned string + */ +char * +virSocketFormatAddrFull(virSocketAddrPtr addr, + bool withService, + const char *separator) +{ + char host[NI_MAXHOST], port[NI_MAXSERV]; + char *addrstr; + int err; - else if (addr->data.stor.ss_family == AF_INET6) { - outlen = INET6_ADDRSTRLEN; - inaddr = &addr->data.inet6.sin6_addr; + if (addr == NULL) { + virSocketError(VIR_ERR_INVALID_ARG, _("Missing address")); + return NULL; } - else { - return NULL; + /* Short-circuit since getnameinfo doesn't work + * nicely for UNIX sockets */ + if (addr->data.sa.sa_family == AF_UNIX) { + if (withService) { + if (virAsprintf(&addrstr, "127.0.0.1%s0", + separator ? separator : ":") < 0) + goto no_memory; + } else { + if (!(addrstr = strdup("127.0.0.1"))) + goto no_memory; + } + return addrstr; } - if (VIR_ALLOC_N(out, outlen) < 0) + if ((err = getnameinfo(&addr->data.sa, + addr->len, + host, sizeof(host), + port, sizeof(port), + NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { + virSocketError(VIR_ERR_SYSTEM_ERROR, + _("Cannot convert socket address to string: %s"), + gai_strerror(err)); return NULL; + } - if (inet_ntop(addr->data.stor.ss_family, inaddr, out, outlen) == NULL) { - VIR_FREE(out); - return NULL; + if (withService) { + if (virAsprintf(&addrstr, "%s%s%s", host, separator, port) == -1) + goto no_memory; + } else { + if (!(addrstr = strdup(host))) + goto no_memory; } - return out; + return addrstr; + +no_memory: + virReportOOMError(); + return NULL; } + /* * virSocketSetPort: * @addr: an initialized virSocketAddrPtr diff --git a/src/util/network.h b/src/util/network.h index ef92c9b..5147ea5 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -16,9 +16,11 @@ # include <sys/types.h> # include <sys/socket.h> # include <netdb.h> +# include <stdbool.h> typedef struct { union { + struct sockaddr sa; struct sockaddr_storage stor; struct sockaddr_in inet4; struct sockaddr_in6 inet6; @@ -39,6 +41,9 @@ int virSocketParseIpv6Addr(const char *val, virSocketAddrPtr addr); char * virSocketFormatAddr(virSocketAddrPtr addr); +char * virSocketFormatAddrFull(virSocketAddrPtr addr, + bool withService, + const char *separator); int virSocketSetPort(virSocketAddrPtr addr, int port); -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The getnameinfo() function is more flexible than inet_ntop() avoiding the need to if/else the code based on socket family. Also make it support UNIX socket addrs and allow inclusion of a port (service) address. Finally do proper error reporting via normal APIs.
* src/conf/domain_conf.c, src/nwfilter/nwfilter_ebiptables_driver.c, src/qemu/qemu_conf.c: Fix error handling with virSocketFormat * src/util/network.c: Rewrite virSocketFormat to use getnameinfo and cope with UNIX socket addrs. --- po/POTFILES.in | 1 + src/conf/domain_conf.c | 5 +-- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 12 +---- src/qemu/qemu_conf.c | 2 + src/util/network.c | 82 ++++++++++++++++++++++------- src/util/network.h | 5 ++ 7 files changed, 75 insertions(+), 33 deletions(-)
+/* + * virSocketFormatAddr:
s/Addr:/AddrFull:/
+ * @addr: an initialized virSocketAddrPtr + * @withService: if true, then service info is appended + * @separator: separator between hostname& service.
Must be non-NULL if withService is true, and ignored when withService is false? In which case, is it merely sufficient to provide a non-NULL string to request withService, in which point the second parameter is redundant?
+ * + * Returns a string representation of the given address + * Returns NULL on any error + * Caller must free the returned string + */ +char * +virSocketFormatAddrFull(virSocketAddrPtr addr, + bool withService, + const char *separator) +{ + char host[NI_MAXHOST], port[NI_MAXSERV]; + char *addrstr; + int err;
- else if (addr->data.stor.ss_family == AF_INET6) { - outlen = INET6_ADDRSTRLEN; - inaddr =&addr->data.inet6.sin6_addr; + if (addr == NULL) { + virSocketError(VIR_ERR_INVALID_ARG, _("Missing address")); + return NULL; }
- else { - return NULL; + /* Short-circuit since getnameinfo doesn't work + * nicely for UNIX sockets */ + if (addr->data.sa.sa_family == AF_UNIX) { + if (withService) { + if (virAsprintf(&addrstr, "127.0.0.1%s0", + separator ? separator : ":")< 0)
Oh, I see - separator defaults to ":" if withService is true but separator is NULL. Still, is the three-argument version buying us anything over a two-argument version where the caller must supply ":" rather than relying on a default? ACK if you decide 3 arguments is worth it, and with the doc typo fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The virSocketParse method was not doing any error reporting which meant the true cause of the problem was lost. Remove all error reporting from callers, and push it into virSocketParse * src/util/network.c: Add error reporting to virSocketParse * src/conf/domain_conf.c, src/conf/network_conf.c, src/network/bridge_driver.c: Remove error reporting in callers of virSocketParse --- src/conf/domain_conf.c | 6 +----- src/conf/network_conf.c | 15 +++------------ src/network/bridge_driver.c | 12 ++---------- src/util/network.c | 21 +++++++++++++++++---- 4 files changed, 23 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 945c1f4..fe93711 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2515,12 +2515,8 @@ virDomainChrDefParseTargetXML(virCapsPtr caps, goto error; } - if (virSocketParseAddr(addrStr, def->target.addr, AF_UNSPEC) < 0) { - virDomainReportError(VIR_ERR_XML_ERROR, - _("%s is not a valid address"), - addrStr); + if (virSocketParseAddr(addrStr, def->target.addr, AF_UNSPEC) < 0) goto error; - } if (def->target.addr->data.stor.ss_family != AF_INET) { virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f209dad..fe52f95 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -244,22 +244,14 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, } if (virSocketParseAddr(start, &saddr, AF_UNSPEC) < 0) { - virNetworkReportError(VIR_ERR_XML_ERROR, - _("cannot parse dhcp start address '%s'"), - start); xmlFree(start); xmlFree(end); - cur = cur->next; - continue; + return -1; } if (virSocketParseAddr(end, &eaddr, AF_UNSPEC) < 0) { - virNetworkReportError(VIR_ERR_XML_ERROR, - _("cannot parse dhcp end address '%s'"), - end); xmlFree(start); xmlFree(end); - cur = cur->next; - continue; + return -1; } range = virSocketGetRange(&saddr, &eaddr); @@ -269,8 +261,7 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, start, end); xmlFree(start); xmlFree(end); - cur = cur->next; - continue; + return -1; } if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) { diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ac91c57..37ed32e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1046,19 +1046,11 @@ static int networkCheckRouteCollision(virNetworkObjPtr network) if (!network->def->ipAddress || !network->def->netmask) return 0; - if (virSocketParseAddr(network->def->ipAddress, &inaddress, AF_UNSPEC) < 0) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse IP address '%s'"), - network->def->ipAddress); + if (virSocketParseAddr(network->def->ipAddress, &inaddress, AF_UNSPEC) < 0) goto error; - } - if (virSocketParseAddr(network->def->netmask, &innetmask, AF_UNSPEC) < 0) { - networkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse netmask '%s'"), - network->def->netmask); + if (virSocketParseAddr(network->def->netmask, &innetmask, AF_UNSPEC) < 0) goto error; - } if (inaddress.data.stor.ss_family != AF_INET || innetmask.data.stor.ss_family != AF_INET) { diff --git a/src/util/network.c b/src/util/network.c index 7c6ced9..4ee4532 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -77,15 +77,28 @@ virSocketParseAddr(const char *val, virSocketAddrPtr addr, int family) { int len; struct addrinfo hints; struct addrinfo *res = NULL; + int err; - if (val == NULL) - return(-1); + if (val == NULL) { + virSocketError(VIR_ERR_INVALID_ARG, _("Missing address")); + return -1; + } memset(&hints, 0, sizeof(hints)); hints.ai_family = family; hints.ai_flags = AI_NUMERICHOST; - if ((getaddrinfo(val, NULL, &hints, &res) != 0) || (res == NULL)) { - return(-1); + if ((err = getaddrinfo(val, NULL, &hints, &res)) != 0) { + virSocketError(VIR_ERR_SYSTEM_ERROR, + _("Cannot parse socket address '%s': %s"), + val, gai_strerror(err)); + return -1; + } + + if (res == NULL) { + virSocketError(VIR_ERR_SYSTEM_ERROR, + _("No socket addresses found for '%s'"), + val); + return -1; } len = res->ai_addrlen; -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The virSocketParse method was not doing any error reporting which meant the true cause of the problem was lost. Remove all error reporting from callers, and push it into virSocketParse
* src/util/network.c: Add error reporting to virSocketParse * src/conf/domain_conf.c, src/conf/network_conf.c, src/network/bridge_driver.c: Remove error reporting in callers of virSocketParse --- src/conf/domain_conf.c | 6 +----- src/conf/network_conf.c | 15 +++------------ src/network/bridge_driver.c | 12 ++---------- src/util/network.c | 21 +++++++++++++++++---- 4 files changed, 23 insertions(+), 31 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The addrToString functionality is now available via the virSocketFormatAddrFull method. * daemon/remote.c, src/remote/remote_driver.c: Remove addrToString methods --- daemon/remote.c | 57 ++++++++----------------------------------- src/remote/remote_driver.c | 52 +++++---------------------------------- 2 files changed, 18 insertions(+), 91 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index ae7a2d3..37dffe3 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -58,6 +58,7 @@ #include "util.h" #include "stream.h" #include "uuid.h" +#include "network.h" #include "virtaudit.h" #include "libvirt/libvirt-qemu.h" @@ -3632,43 +3633,6 @@ remoteDispatchAuthList (struct qemud_server *server, #if HAVE_SASL /* - * NB, keep in sync with similar method in src/remote/remote_driver.c - */ -static char *addrToString(remote_error *rerr, - struct sockaddr_storage *ss, socklen_t salen) { - char host[NI_MAXHOST], port[NI_MAXSERV]; - char *addr; - int err; - struct sockaddr *sa = (struct sockaddr *)ss; - - if (sa->sa_family == AF_UNIX) { - if (!(addr = strdup("127.0.0.1;0"))) { - virReportOOMError(); - return NULL; - } - return addr; - } - - if ((err = getnameinfo(sa, salen, - host, sizeof(host), - port, sizeof(port), - NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteDispatchFormatError(rerr, - _("Cannot convert socket address to string: %s"), - gai_strerror(err)); - return NULL; - } - - if (virAsprintf(&addr, "%s;%s", host, port) == -1) { - virReportOOMError(); - return NULL; - } - - return addr; -} - - -/* * Initializes the SASL session in prepare for authentication * and gives the client a list of allowed mechanisms to choose * @@ -3677,7 +3641,7 @@ static char *addrToString(remote_error *rerr, static int remoteDispatchAuthSaslInit (struct qemud_server *server, struct qemud_client *client, - virConnectPtr conn ATTRIBUTE_UNUSED, + virConnectPtr conn, remote_message_header *hdr ATTRIBUTE_UNUSED, remote_error *rerr, void *args ATTRIBUTE_UNUSED, @@ -3686,8 +3650,7 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, const char *mechlist = NULL; sasl_security_properties_t secprops; int err; - struct sockaddr_storage sa; - socklen_t salen; + virSocketAddr sa; char *localAddr, *remoteAddr; virMutexLock(&server->lock); @@ -3702,29 +3665,31 @@ remoteDispatchAuthSaslInit (struct qemud_server *server, } /* Get local address in form IPADDR:PORT */ - salen = sizeof(sa); - if (getsockname(client->fd, (struct sockaddr*)&sa, &salen) < 0) { + sa.len = sizeof(sa.data.stor); + if (getsockname(client->fd, &sa.data.sa, &sa.len) < 0) { char ebuf[1024]; remoteDispatchFormatError(rerr, _("failed to get sock address: %s"), virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if ((localAddr = addrToString(rerr, &sa, salen)) == NULL) { + if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) { + remoteDispatchConnError(rerr, conn); goto error; } /* Get remote address in form IPADDR:PORT */ - salen = sizeof(sa); - if (getpeername(client->fd, (struct sockaddr*)&sa, &salen) < 0) { + sa.len = sizeof(sa.data.stor); + if (getpeername(client->fd, &sa.data.sa, &sa.len) < 0) { char ebuf[1024]; remoteDispatchFormatError(rerr, _("failed to get peer address: %s"), virStrerror(errno, ebuf, sizeof ebuf)); VIR_FREE(localAddr); goto error; } - if ((remoteAddr = addrToString(rerr, &sa, salen)) == NULL) { + if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) { VIR_FREE(localAddr); + remoteDispatchConnError(rerr, conn); goto error; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 38e2d55..c8d9a4d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6888,43 +6888,6 @@ remoteAuthenticate (virConnectPtr conn, struct private_data *priv, #if HAVE_SASL -/* - * NB, keep in sync with similar method in daemon/remote.c - */ -static char *addrToString(struct sockaddr_storage *ss, socklen_t salen) -{ - char host[NI_MAXHOST], port[NI_MAXSERV]; - char *addr; - int err; - struct sockaddr *sa = (struct sockaddr *)ss; - - if (sa->sa_family == AF_UNIX) { - if (!(addr = strdup("127.0.0.1;0"))) { - virReportOOMError(); - return NULL; - } - return addr; - } - - if ((err = getnameinfo(sa, salen, - host, sizeof(host), - port, sizeof(port), - NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - remoteError(VIR_ERR_UNKNOWN_HOST, - _("Cannot convert socket address to string: %s"), - gai_strerror(err)); - return NULL; - } - - if (virAsprintf(&addr, "%s;%s", host, port) == -1) { - virReportOOMError(); - return NULL; - } - - return addr; -} - - static int remoteAuthCredVir2SASL(int vircred) { switch (vircred) { @@ -7105,8 +7068,7 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, unsigned int clientoutlen, serverinlen; const char *mech; int err, complete; - struct sockaddr_storage sa; - socklen_t salen; + virSocketAddr sa; char *localAddr = NULL, *remoteAddr = NULL; const void *val; sasl_ssf_t ssf; @@ -7128,23 +7090,23 @@ remoteAuthSASL (virConnectPtr conn, struct private_data *priv, int in_open, } /* Get local address in form IPADDR:PORT */ - salen = sizeof(sa); - if (getsockname(priv->sock, (struct sockaddr*)&sa, &salen) < 0) { + sa.len = sizeof(sa.data.stor); + if (getsockname(priv->sock, &sa.data.sa, &sa.len) < 0) { virReportSystemError(errno, "%s", _("failed to get sock address")); goto cleanup; } - if ((localAddr = addrToString(&sa, salen)) == NULL) + if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) goto cleanup; /* Get remote address in form IPADDR:PORT */ - salen = sizeof(sa); - if (getpeername(priv->sock, (struct sockaddr*)&sa, &salen) < 0) { + sa.len = sizeof(sa.data.stor); + if (getpeername(priv->sock, &sa.data.sa, &sa.len) < 0) { virReportSystemError(errno, "%s", _("failed to get peer address")); goto cleanup; } - if ((remoteAddr = addrToString(&sa, salen)) == NULL) + if ((remoteAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) goto cleanup; if (auth) { -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The addrToString functionality is now available via the virSocketFormatAddrFull method.
* daemon/remote.c, src/remote/remote_driver.c: Remove addrToString methods --- daemon/remote.c | 57 ++++++++----------------------------------- src/remote/remote_driver.c | 52 +++++---------------------------------- 2 files changed, 18 insertions(+), 91 deletions(-)
ACK; nice cleanup! -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The inet_pton and inet_ntop functions are obsolete, replaced by getaddrinfo+getnameinfo with the AI_NUMERICHOST flag set. These can be accessed via the virSocket APIs. The bridge.c code had methods for fetching the IP address of a bridge which used inet_ntop. Aside from the use of inet_ntop these methods are broken, because a NIC can have multiple addresses and this only returns one address. Since the methods are never used, just remove them. * src/conf/network_conf.c, src/nwfilter/nwfilter_learnipaddr.c: Replace inet_pton and inet_ntop with virSocket APIs * src/util/bridge.c, src/util/bridge.h: Remove unused methods which called inet_ntop. --- src/conf/network_conf.c | 36 +++++++------- src/nwfilter/nwfilter_learnipaddr.c | 36 +++++++------ src/util/bridge.c | 95 ++--------------------------------- src/util/bridge.h | 8 --- 4 files changed, 43 insertions(+), 132 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index fe52f95..0663d52 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -278,7 +278,7 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, xmlStrEqual(cur->name, BAD_CAST "host")) { xmlChar *mac, *name, *ip; unsigned char addr[6]; - struct in_addr inaddress; + virSocketAddr inaddr; mac = xmlGetProp(cur, BAD_CAST "mac"); if ((mac != NULL) && @@ -305,10 +305,7 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, continue; } ip = xmlGetProp(cur, BAD_CAST "ip"); - if (inet_pton(AF_INET, (const char *) ip, &inaddress) <= 0) { - virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse IP address '%s'"), - ip); + if (virSocketParseAddr((const char *)ip, &inaddr, AF_UNSPEC) < 0) { VIR_FREE(ip); VIR_FREE(mac); VIR_FREE(name); @@ -428,31 +425,34 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->netmask = virXPathString("string(./ip[1]/@netmask)", ctxt); if (def->ipAddress && def->netmask) { - /* XXX someday we want IPv6 too, so inet_aton won't work there */ - struct in_addr inaddress, innetmask; + virSocketAddr inaddress, innetmask; char *netaddr; xmlNodePtr ip; - if (inet_pton(AF_INET, def->ipAddress, &inaddress) <= 0) { - virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse IP address '%s'"), - def->ipAddress); + if (virSocketParseAddr(def->ipAddress, &inaddress, AF_UNSPEC) < 0) goto error; - } - if (inet_pton(AF_INET, def->netmask, &innetmask) <= 0) { - virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse netmask '%s'"), - def->netmask); + if (virSocketParseAddr(def->netmask, &innetmask, AF_UNSPEC) < 0) + goto error; + + /* XXX someday we want IPv6, so will need to relax this */ + if (inaddress.data.sa.sa_family != AF_INET || + innetmask.data.sa.sa_family != AF_INET) { + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Only IPv4 addresses are supported")); goto error; } - inaddress.s_addr &= innetmask.s_addr; - netaddr = inet_ntoa(inaddress); + inaddress.data.inet4.sin_addr.s_addr &= + innetmask.data.inet4.sin_addr.s_addr; + if (!(netaddr = virSocketFormatAddr(&inaddress))) + goto error; if (virAsprintf(&def->network, "%s/%s", netaddr, def->netmask) < 0) { + VIR_FREE(netaddr); virReportOOMError(); goto error; } + VIR_FREE(netaddr); if ((ip = virXPathNode("./ip[1]", ctxt)) && virNetworkIPParseXML(def, ip) < 0) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 7c94fc2..813a205 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -627,22 +627,26 @@ learnIPAddressThread(void *arg) if (req->status == 0) { int ret; - char inetaddr[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &vmaddr, inetaddr, sizeof(inetaddr)); - - virNWFilterAddIpAddrForIfname(req->ifname, strdup(inetaddr)); - - ret = virNWFilterInstantiateFilterLate(NULL, - req->ifname, - req->ifindex, - req->linkdev, - req->nettype, - req->macaddr, - req->filtername, - req->filterparams, - req->driver); - VIR_DEBUG("Result from applying firewall rules on " - "%s with IP addr %s : %d\n", req->ifname, inetaddr, ret); + virSocketAddr sa; + sa.len = sizeof(sa.data.inet4); + sa.data.inet4.sin_addr.s_addr = vmaddr; + char *inetaddr; + + if ((inetaddr = virSocketFormatAddr(&sa))!= NULL) { + virNWFilterAddIpAddrForIfname(req->ifname, inetaddr); + + ret = virNWFilterInstantiateFilterLate(NULL, + req->ifname, + req->ifindex, + req->linkdev, + req->nettype, + req->macaddr, + req->filtername, + req->filterparams, + req->driver); + VIR_DEBUG("Result from applying firewall rules on " + "%s with IP addr %s : %d\n", req->ifname, inetaddr, ret); + } } else { if (showError) virReportSystemError(req->status, diff --git a/src/util/bridge.c b/src/util/bridge.c index da62c5e..b4a7e26 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -48,6 +48,7 @@ # include "memory.h" # include "util.h" # include "logging.h" +# include "network.h" # define JIFFIES_TO_MS(j) (((j)*1000)/HZ) # define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000) @@ -660,13 +661,8 @@ brSetInetAddr(brControl *ctl, int cmd, const char *addr) { - union { - struct sockaddr sa; - struct sockaddr_in sa_in; - } s; + virSocketAddr sa; struct ifreq ifr; - struct in_addr inaddr; - int ret; if (!ctl || !ctl->fd || !ifname || !addr) return EINVAL; @@ -676,51 +672,17 @@ brSetInetAddr(brControl *ctl, if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) return EINVAL; - if ((ret = inet_pton(AF_INET, addr, &inaddr)) < 0) - return errno; - else if (ret == 0) + if (virSocketParseAddr(addr, &sa, AF_UNSPEC) < 0) return EINVAL; - s.sa_in.sin_family = AF_INET; - s.sa_in.sin_addr = inaddr; - - ifr.ifr_addr = s.sa; - - if (ioctl(ctl->fd, cmd, &ifr) < 0) - return errno; - - return 0; -} - -static int -brGetInetAddr(brControl *ctl, - const char *ifname, - int cmd, - char *addr, - int maxlen) -{ - struct ifreq ifr; - struct in_addr *inaddr; - - if (!ctl || !ctl->fd || !ifname || !addr) + if (sa.data.sa.sa_family != AF_INET) return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + ifr.ifr_addr = sa.data.sa; if (ioctl(ctl->fd, cmd, &ifr) < 0) return errno; - if (maxlen < BR_INET_ADDR_MAXLEN || ifr.ifr_addr.sa_family != AF_INET) - return EFAULT; - - inaddr = &((struct sockaddr_in *)&ifr.ifr_data)->sin_addr; - - if (!inet_ntop(AF_INET, inaddr, addr, maxlen)) - return errno; - return 0; } @@ -746,29 +708,6 @@ brSetInetAddress(brControl *ctl, } /** - * brGetInetAddress: - * @ctl: bridge control pointer - * @ifname: the interface name - * @addr: the array for the string representation of the IP address - * @maxlen: size of @addr in bytes - * - * Function to get the IP address of an interface, it should handle - * IPV4 and IPv6. The returned string for addr would be of the form - * "ddd.ddd.ddd.ddd" assuming the common IPv4 format. - * - * Returns 0 in case of success or an errno code in case of failure. - */ - -int -brGetInetAddress(brControl *ctl, - const char *ifname, - char *addr, - int maxlen) -{ - return brGetInetAddr(ctl, ifname, SIOCGIFADDR, addr, maxlen); -} - -/** * brSetInetNetmask: * @ctl: bridge control pointer * @ifname: the interface name @@ -790,30 +729,6 @@ brSetInetNetmask(brControl *ctl, } /** - * brGetInetNetmask: - * @ctl: bridge control pointer - * @ifname: the interface name - * @addr: the array for the string representation of the netmask - * @maxlen: size of @addr in bytes - * - * Function to get the netmask of an interface, it should handle - * IPV4 and IPv6. The returned string for addr would be of the form - * "ddd.ddd.ddd.ddd" assuming the common IPv4 format. - * - * Returns 0 in case of success or an errno code in case of failure. - */ - -int -brGetInetNetmask(brControl *ctl, - const char *ifname, - char *addr, - int maxlen) -{ - return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen); -} - - -/** * brSetForwardDelay: * @ctl: bridge control pointer * @bridge: the bridge name diff --git a/src/util/bridge.h b/src/util/bridge.h index 96696ac..abcd1b5 100644 --- a/src/util/bridge.h +++ b/src/util/bridge.h @@ -85,17 +85,9 @@ int brGetInterfaceUp (brControl *ctl, int brSetInetAddress (brControl *ctl, const char *ifname, const char *addr); -int brGetInetAddress (brControl *ctl, - const char *ifname, - char *addr, - int maxlen); int brSetInetNetmask (brControl *ctl, const char *ifname, const char *netmask); -int brGetInetNetmask (brControl *ctl, - const char *ifname, - char *netmask, - int maxlen); int brSetForwardDelay (brControl *ctl, const char *bridge, -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
The inet_pton and inet_ntop functions are obsolete, replaced by getaddrinfo+getnameinfo with the AI_NUMERICHOST flag set. These can be accessed via the virSocket APIs.
The bridge.c code had methods for fetching the IP address of a bridge which used inet_ntop. Aside from the use of inet_ntop these methods are broken, because a NIC can have multiple addresses and this only returns one address. Since the methods are never used, just remove them.
* src/conf/network_conf.c, src/nwfilter/nwfilter_learnipaddr.c: Replace inet_pton and inet_ntop with virSocket APIs * src/util/bridge.c, src/util/bridge.h: Remove unused methods which called inet_ntop. --- src/conf/network_conf.c | 36 +++++++------- src/nwfilter/nwfilter_learnipaddr.c | 36 +++++++------ src/util/bridge.c | 95 ++--------------------------------- src/util/bridge.h | 8 --- 4 files changed, 43 insertions(+), 132 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
All the inet_* functions can be replaced with calls to the virSocket APIs. Since many of the inet_* funtions are unsafe, and the remainder are obsolete, forbid all future use of them in libvirt. * Makefile.nonreentrant: Ban use of inet_* --- Makefile.nonreentrant | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/Makefile.nonreentrant b/Makefile.nonreentrant index b567f31..f656dbb 100644 --- a/Makefile.nonreentrant +++ b/Makefile.nonreentrant @@ -12,6 +12,10 @@ # | uniq \ # | sed -e 's/_r//' # +# Also manually add in all inet_* functions some of which +# are not threadsafe and do not have _r variants. They are +# all deprecated in favour of getnameinfo/getaddrinfo +# NON_REENTRANT = NON_REENTRANT += asctime @@ -83,3 +87,14 @@ NON_REENTRANT += strerror NON_REENTRANT += strtok NON_REENTRANT += tmpnam NON_REENTRANT += ttyname +NON_REENTRANT += inet_addr +NON_REENTRANT += inet_aton +NON_REENTRANT += inet_lnaof +NON_REENTRANT += inet_makeaddr +NON_REENTRANT += inet_netof +NON_REENTRANT += inet_network +NON_REENTRANT += inet_nsap_addr +NON_REENTRANT += inet_nsap_ntoa +NON_REENTRANT += inet_ntoa +NON_REENTRANT += inet_ntop +NON_REENTRANT += inet_pton -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
All the inet_* functions can be replaced with calls to the virSocket APIs. Since many of the inet_* funtions are unsafe, and the remainder are obsolete, forbid all future use of them in libvirt.
* Makefile.nonreentrant: Ban use of inet_* --- Makefile.nonreentrant | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Add a test suite for check parsing, formatting, range calculation and netmask checking APIs in virSocketAddr. * tests/sockettest.c, tests/Makefile.am: Add new test case --- tests/Makefile.am | 8 ++- tests/sockettest.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 1 deletions(-) create mode 100644 tests/sockettest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 44fe579..9720b6e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -72,7 +72,7 @@ EXTRA_DIST = \ domainsnapshotxml2xmlin \ qemuhelpdata -check_PROGRAMS = virshtest conftest \ +check_PROGRAMS = virshtest conftest sockettest \ nodeinfotest statstest qparamtest virbuftest if WITH_XEN @@ -152,6 +152,7 @@ TESTS = virshtest \ statstest \ qparamtest \ virbuftest \ + sockettest \ $(test_scripts) if WITH_XEN @@ -214,6 +215,11 @@ TESTS_ENVIRONMENT = \ valgrind: $(MAKE) check VG="valgrind --quiet --leak-check=full --suppressions=$(srcdir)/.valgrind.supp" +sockettest_SOURCES = \ + sockettest.c \ + testutils.c testutils.h +sockettest_LDADD = ../src/libvirt_util.la $(LDADDS) + if WITH_XEN xml2sexprtest_SOURCES = \ xml2sexprtest.c testutilsxen.c testutilsxen.h \ diff --git a/tests/sockettest.c b/tests/sockettest.c new file mode 100644 index 0000000..eebac4a --- /dev/null +++ b/tests/sockettest.c @@ -0,0 +1,238 @@ +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include "network.h" +#include "testutils.h" +#include "logging.h" +#include "memory.h" + +static void testQuietError(void *userData ATTRIBUTE_UNUSED, + virErrorPtr error ATTRIBUTE_UNUSED) +{ + /* nada */ +} + +static int testParse(virSocketAddr *addr, const char *addrstr, int family, bool pass) +{ + int rc; + + rc = virSocketParseAddr(addrstr, addr, family); + + if (rc < 0) + return pass ? -1 : 0; + else + return pass ? 0 : -1; +} + +static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) +{ + char *newaddrstr; + + newaddrstr = virSocketFormatAddr(addr); + if (!newaddrstr) + return pass ? -1 : 0; + + if (STRNEQ(newaddrstr, addrstr)) { + virtTestDifference(stderr, newaddrstr, addrstr); + VIR_FREE(newaddrstr); + return pass ? -1 : 0; + } else { + VIR_FREE(newaddrstr); + return pass ? 0 : -1; + } +} + +struct testParseData { + virSocketAddr *addr; + const char *addrstr; + int family; + bool pass; +}; +static int testParseHelper(const void *opaque) +{ + const struct testParseData *data = opaque; + return testParse(data->addr, data->addrstr, data->family, data->pass); +} + +struct testFormatData { + virSocketAddr *addr; + const char *addrstr; + bool pass; +}; +static int testFormatHelper(const void *opaque) +{ + const struct testFormatData *data = opaque; + return testFormat(data->addr, data->addrstr, data->pass); +} + + +static int testRange(const char *saddrstr, const char *eaddrstr, int size, bool pass) +{ + virSocketAddr saddr; + virSocketAddr eaddr; + + if (virSocketParseAddr(saddrstr, &saddr, AF_UNSPEC) < 0) + return -1; + if (virSocketParseAddr(eaddrstr, &eaddr, AF_UNSPEC) < 0) + return -1; + + int gotsize = virSocketGetRange(&saddr, &eaddr); + VIR_DEBUG("Size want %d vs got %d", size, gotsize); + if (gotsize < 0 || gotsize != size) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1; + } +} + +struct testRangeData { + const char *saddr; + const char *eaddr; + int size; + bool pass; +}; +static int testRangeHelper(const void *opaque) +{ + const struct testRangeData *data = opaque; + return testRange(data->saddr, data->eaddr, data->size, data->pass); +} + + +static int testNetmask(const char *addr1str, const char *addr2str, + const char *netmaskstr, bool pass) +{ + virSocketAddr addr1; + virSocketAddr addr2; + virSocketAddr netmask; + + if (virSocketParseAddr(addr1str, &addr1, AF_UNSPEC) < 0) + return -1; + if (virSocketParseAddr(addr2str, &addr2, AF_UNSPEC) < 0) + return -1; + if (virSocketParseAddr(netmaskstr, &netmask, AF_UNSPEC) < 0) + return -1; + + int ret = virSocketCheckNetmask(&addr1, &addr2, &netmask); + + if (ret <= 0) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1; + } +} + +struct testNetmaskData { + const char *addr1; + const char *addr2; + const char *netmask; + bool pass; +}; +static int testNetmaskHelper(const void *opaque) +{ + const struct testNetmaskData *data = opaque; + return testNetmask(data->addr1, data->addr2, data->netmask, data->pass); +} + + +static int +mymain(int argc ATTRIBUTE_UNUSED, + char **argv ATTRIBUTE_UNUSED) +{ + int ret = 0; + /* Some of our tests delibrately test failure cases, so + * register a handler to stop error messages cluttering + * up display + */ + if (!virTestGetDebug()) + virSetErrorFunc(NULL, testQuietError); + +#define DO_TEST_PARSE(addrstr, family, pass) \ + do { \ + virSocketAddr addr; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + memset(&addr, 0, sizeof(addr)); \ + if (virtTestRun("Test parse " addrstr, \ + 1, testParseHelper, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_PARSE_AND_FORMAT(addrstr, family, pass) \ + do { \ + virSocketAddr addr; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + memset(&addr, 0, sizeof(addr)); \ + if (virtTestRun("Test parse " addrstr " family " #family, \ + 1, testParseHelper, &data) < 0) \ + ret = -1; \ + struct testFormatData data2 = { &addr, addrstr, pass }; \ + if (virtTestRun("Test format " addrstr " family " #family, \ + 1, testFormatHelper, &data2) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_RANGE(saddr, eaddr, size, pass) \ + do { \ + struct testRangeData data = { saddr, eaddr, size, pass }; \ + if (virtTestRun("Test range " saddr " -> " eaddr " size " #size, \ + 1, testRangeHelper, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_NETMASK(addr1, addr2, netmask, pass) \ + do { \ + struct testNetmaskData data = { addr1, addr2, netmask, pass }; \ + if (virtTestRun("Test netmask " addr1 " + " addr2 " in " netmask, \ + 1, testNetmaskHelper, &data) < 0) \ + ret = -1; \ + } while (0) + + + DO_TEST_PARSE_AND_FORMAT("127.0.0.1", AF_UNSPEC, true); + DO_TEST_PARSE_AND_FORMAT("127.0.0.1", AF_INET, true); + DO_TEST_PARSE_AND_FORMAT("127.0.0.1", AF_INET6, false); + DO_TEST_PARSE_AND_FORMAT("127.0.0.1", AF_UNIX, false); + DO_TEST_PARSE_AND_FORMAT("127.0.0.256", AF_UNSPEC, false); + + DO_TEST_PARSE_AND_FORMAT("::1", AF_UNSPEC, true); + DO_TEST_PARSE_AND_FORMAT("::1", AF_INET, false); + DO_TEST_PARSE_AND_FORMAT("::1", AF_INET6, true); + DO_TEST_PARSE_AND_FORMAT("::1", AF_UNIX, false); + DO_TEST_PARSE_AND_FORMAT("::ffff", AF_UNSPEC, true); + + DO_TEST_RANGE("192.168.122.1", "192.168.122.1", 1, true); + DO_TEST_RANGE("192.168.122.1", "192.168.122.20", 20, true); + DO_TEST_RANGE("192.168.122.0", "192.168.122.255", 256, true); + DO_TEST_RANGE("192.168.122.20", "192.168.122.1", -1, false); + DO_TEST_RANGE("10.0.0.1", "192.168.122.20", -1, false); + DO_TEST_RANGE("192.168.122.20", "10.0.0.1", -1, false); + + DO_TEST_RANGE("2000::1", "2000::1", 1, true); + DO_TEST_RANGE("2000::1", "2000::2", 2, true); + DO_TEST_RANGE("2000::2", "2000::1", -1, false); + DO_TEST_RANGE("2000::1", "9001::1", -1, false); + + DO_TEST_NETMASK("192.168.122.1", "192.168.122.2", + "255.255.255.0", true); + DO_TEST_NETMASK("192.168.122.1", "192.168.122.4", + "255.255.255.248", true); + DO_TEST_NETMASK("192.168.122.1", "192.168.123.2", + "255.255.255.0", false); + DO_TEST_NETMASK("192.168.122.1", "192.168.123.2", + "255.255.0.0", true); + + DO_TEST_NETMASK("2000::1:1", "2000::1:1", + "ffff:ffff:ffff:ffff:ffff:ffff:ffff:0", true); + DO_TEST_NETMASK("2000::1:1", "2000::2:1", + "ffff:ffff:ffff:ffff:ffff:ffff:ffff:0", false); + DO_TEST_NETMASK("2000::1:1", "2000::2:1", + "ffff:ffff:ffff:ffff:ffff:ffff:fff8:0", true); + DO_TEST_NETMASK("2000::1:1", "9000::1:1", + "ffff:ffff:ffff:ffff:ffff:ffff:ffff:0", false); + + return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain) -- 1.7.2.3
On 10/21/2010 12:17 PM, Daniel P. Berrange wrote:
Add a test suite for check parsing, formatting, range calculation and netmask checking APIs in virSocketAddr.
* tests/sockettest.c, tests/Makefile.am: Add new test case --- tests/Makefile.am | 8 ++- tests/sockettest.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 1 deletions(-) create mode 100644 tests/sockettest.c
Nice addition.
+++ b/tests/sockettest.c @@ -0,0 +1,238 @@ +#include<config.h>
Missing a copyright disclaimer.
+ +static int +mymain(int argc ATTRIBUTE_UNUSED, + char **argv ATTRIBUTE_UNUSED) +{ + int ret = 0; + /* Some of our tests delibrately test failure cases, so
s/delibrately/deliberately/ ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 21, 2010 at 07:17:14PM +0100, Daniel P. Berrange wrote:
In working on the DTrace patches I needed to be able to format a struct sockaddr into a string easily. The virSocketFormatAddr API was close to what I needed, but couldn't handle including the port number, nor UNIX domain sockets.
This patch series addresses that limitation. Along the way it fixes miscellaneous bugs with the virSocket APis, adds a test suite, removes & bans all use of inet_* functions and replaces the addrToString methods used in SASL code and simplifies some nwfilter code using virSocket.
Pushed all these with the suggested changes, and one fix to the nwfilter code identified by the TCK tests 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 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump