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

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(+) > Closer... But still missing a couple of things, which I can add for you. I'll make my comments and do the changes locally but not commit until Mon afternoon (Eastern US) so if Laine wants to comment he can and of course that you agree with my comments...

Going to split this commit into two - The first commit will just make the virNetDevIPAddress shim, moving the virNetDevIPv4Address to a static function. The second commit will add the IPv6 option to virNetDevIPAddress

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index ba05cc6..f88626a 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1670,6 +1670,7 @@ virNetDevClearIPAddress; > virNetDevDelMulti; > virNetDevExists; > virNetDevGetIndex; > +virNetDevGetIPAddress; > virNetDevGetIPv4Address; We can remove the GetIPv4Address and make it static to virtnetdev.c

> virNetDevGetLinkInfo; > virNetDevGetMAC; > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index 2a0eb43..318c3b6 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -33,6 +33,10 @@ > #include "virstring.h" > #include "virutil.h" > > +#if defined(HAVE_GETIFADDRS) > +# include <ifaddrs.h> > +#endif > + > #include <sys/ioctl.h> > #include <net/if.h> > #include <fcntl.h> > @@ -1432,6 +1436,100 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED, > > #endif /* ! SIOCGIFADDR */ > > +/** > + * virNetDevGetIfaddrsAddress: > + * @ifname: name of the interface whose IP address we want > + * @want_ipv6: get IPv4 or IPv6 address > + * @addr: filled with the IP address > + * > + * This function gets the IP address for the interface @ifname > + * and stores it in @addr > + * > + * Returns 0 on success, -1 on failure, -2 on unsupported. > + */ > +#if defined(HAVE_GETIFADDRS) > +static int virNetDevGetIfaddrsAddress(const char *ifname, > + bool want_ipv6, > + virSocketAddrPtr addr) Although not a rule - we try to make newer API's as: static int fname(param1, param2)

> +{ > + struct ifaddrs *ifap, *ifa; > + int ret = -1; > + 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) { > + if (!ifa->ifa_addr || > + STRNEQ(ifa->ifa_name, ifname)) { > + continue; > + } STRNEQ_NULLABLE does the trick... > + 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'

> + else > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Interface '%s' not found"), > + ifname); > + > + freeifaddrs(ifap); > + return ret; > +} > + > +#else > + > +static int virNetDevGetIfaddrsAddress(const char *ifname ATTRIBUTE_UNUSED, > + bool want_ipv6, > + virSocketAddrPtr addr ATTRIBUTE_UNUSED) ditto on function definition > +{ > + virReportSystemError(ENOSYS, > + _("Unable to get %s address on this platform"), > + want_ipv6 ? "IPv6" : "IPv4"); While this seems appropriate, because you potentially call virNetDevGetIPv4Address below that means it's possible we return with this error message set, but a good status or we overwrite some other message... So I'll move it...

> + return -2; > +} > + > +#endif > + > + > +int virNetDevGetIPAddress(const char *ifname, > + bool want_ipv6, > + virSocketAddrPtr addr) same > +{ > + int ret; > + > + memset(addr, 0, sizeof(*addr)); > + addr->data.stor.ss_family = AF_UNSPEC; > + > + ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addr); > + if (ret != -2) > + return ret; > + > + if (!want_ipv6) > + return virNetDevGetIPv4Address(ifname, addr); Here if we have virNetDevGetIPv4Address() return -2, then we can take our message above and place it right here while also adjusting the "! SIOCGIFADDR" to just return -2. > + > + return -1; > +} NOTE: This does not return -2 in any

> > /** > * virNetDevValidateConfig: > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index de8b480..faf3b2f 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -104,6 +104,10 @@ int virNetDevClearIPAddress(const char *ifname, > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > int virNetDevGetIPv4Address(const char *ifname, virSocketAddrPtr addr) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; Removing the IPv4Address API > +int virNetDevGetIPAddress(const char *ifname, > + bool want_ipv6, > + virSocketAddrPtr addr) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; > > > int virNetDevSetMAC(const char *ifname, > 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; } John