Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address

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(a)redhat.com&gt; >> --- >> 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)

+{ + 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