On 03/09/2015 11:37 AM, Luyao Huang wrote:
On 03/09/2015 07:50 AM, John Ferlan wrote:
On 02/28/2015 04:08 AM, Luyao Huang wrote:
Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6 and IPv4 address.
Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress and virNetDevGetIPv4Address to get IP address.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: add a new function virNetDevGetIPAddress to call virNetDevGetIfaddrsAddress , and make virNetDevGetIfaddrsAddress can get ipv4 address for a interface. Also add a error output in virNetDevGetIfaddrsAddress when get multiple ip address for a host interface.
src/libvirt_private.syms | 1 + src/util/virnetdev.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 4 ++ 3 files changed, 103 insertions(+)
...
+ if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + if (++nIPaddr > 1) + break; [1]... hmm not sure about this...
+ if (want_ipv6) { + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); + } else { + addr->len = sizeof(addr->data.inet4); + memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len); + } + addr->data.stor.ss_family = ifa->ifa_addr->sa_family; + } + } + + if (nIPaddr == 1) + ret = 0; + else if (nIPaddr > 1) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' has multiple %s address"), s/address/addresses
+ ifname, want_ipv6 ? "IPv6" : "IPv4"); [1] But is this really desired... It seems from the previous review, if someone wants a specific address they use "<listen type='address'...".
Otherwise, they use this function. Since you'll be returning either an IPv4 or IPv6 address here you'd be causing an error here if the network had more than one IPv4 address defined; whereas, the previous code just returned the "first" from the ioctl(SIOCGIFADDR...).
I think once you have an address you just return the first one found and document it that way avoiding this path completely. You could also note that if you want a specific address use the type='address'
I had an idea but my eyes almost close ;) so i write here without test them.
I think it will be better if we make this function to get mutli address and return the number of address we get, like this:
+static int +virNetDevGetIfaddrsAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr *addrs)
We'd need to have an naddrs to know how many we can stuff in... or you'd need to do the *REALLOC on the fly in this module for each address found. Interesting idea, but you'd be just throwing things away. I could perhaps having some code "periodically" cache addresses... or cache addresses, subscribe to some event to let you know when a new address is generated so you can add it to your list (if there is one)... But, how do you use it? John
+{ + struct ifaddrs *ifap, *ifa; + int nIPaddr = 0; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, + _("Could not get interface list for '%s'"), + ifname); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + virSocketAddrPtr tmpaddr; + + if (!ifa->ifa_addr || + STRNEQ_NULLABLE(ifa->ifa_name, ifname)) { + continue; + } + + if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + memset(tmpaddr, 0, sizeof(*tmpaddr)); + + if (!ifa->ifa_addr || + STRNEQ_NULLABLE(ifa->ifa_name, ifname)) { + continue; + } + + if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + memset(tmpaddr, 0, sizeof(*tmpaddr)); + + if (want_ipv6) { + tmpaddr->len = sizeof(tmpaddr->data.inet6); + memcpy(&tmpaddr->data.inet6, ifa->ifa_addr, tmpaddr->len); + } else { + tmpaddr->len = sizeof(tmpaddr->data.inet4); + memcpy(&tmpaddr->data.inet4, ifa->ifa_addr, tmpaddr->len); + } + tmpaddr->data.stor.ss_family = ifa->ifa_addr->sa_family; + addrs[nIPaddr++] = tmpaddr; + } + } + if (nIPaddr == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + return -1; + } + freeifaddrs(ifap); + return nIPaddr; +}
+int virNetDevGetIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) and then rename this function to virNetDevGetOneIPAddress()
and rework the code like this:
+int virNetDevGetOneIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) +{ + int ret; + virSocketAddrPtr *addrs = NULL; + + ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addrs); + if (ret > 0) { + addr = addrs[0]; + } else if (ret == -2 && !want_ipv6) { + return virNetDevGetIPv4Address(ifname, addr); + } + + return ret; +}
...
These changes require modifying src/network/bridge_driver.c (temporarily until we add the IPv6 aware code later):
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2a61991..7d6e173 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4608,7 +4608,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) }
if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + if (virNetDevGetIPAddress(dev_name, false, &addr) < 0) goto cleanup; addrptr = &addr; }
At last, add the multiple address check to here and this place will like this:
+ int num = virNetDevGetOneIPAddress(dev_name, want_ipv6, &addr); + if (num > 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("interface '%s' has multiple %s addresses"), + dev_name, want_ipv6 ? "IPv6" : "IPv4"); + goto cleanup; + } else if (num < 0) { goto cleanup; + }
because i am not test them so there will be some mistake, I will test them tomorrow, hope it will work :)
John
Luyao