On 02/25/2015 12:12 AM, John Ferlan wrote:
On 02/13/2015 02:17 AM, Luyao Huang wrote:
Introduce a new function to help to get interface IPv6 address.
s/Introduce a new function to help to/Introduce virNetDevGetIPv6Address/
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 73 insertions(+)
Hmm... maybe rather than introducing a new IPv6 specific routine and forcing the caller to handle the logic of knowing how/whether to return an IPv4 or IPv6 address...
Why not change the existing GetIPv4Address into a "shim" virNetDevGetIPAddress which then makes the decisions regarding returning only one family or allowing a failed fetch of IPv4 to use the IPv6 routine...
This way you hide the details. Your first patch would just change the IPv4 into an GetIPAddress API and that would just call the now local/static IPv4 API. You won't have a #if #else #endif for the new API - it would return 0 or -1.
Check out how "safezero" has multiple ways in order to zero out a file based on what's present. I suspect your new API could follow that methodology.
In the long run since getifaddrs() can return an IPv4 or IPv6 address it could be theoretically called first. If it returns nothing, fall back to the IPv4 API
Your new API could be something like:
virNetDevGetIPAddress(const char *ifname, bool want_ipv6, virSocketAddrPtr addr)
{ 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);
return -1; }
The virNetDevGetIfaddrsAddress would follow safezero_posix_fallocate returning -2 in the #else of #if defined(HAVE_GETIFADDRS). The logic in the function would return the first found address of IPv6 if that's desired or IPv4 otherwise.
Good idea! I just thought add a new functions for ipv6 address but forgot getifaddrs() can also get the IPv4 address :) Thanks for pointing out and i will rework this patch in next version.
The virNetDevGetIPv4Address() wouldn't need the two stolen lines to clear addr, but would otherwise function as it does today.
Hopefully this makes sense - you'll be adding more patches, but I think in the long run based on the following patches it will make it "easier" on the caller to just get "an" address and force it to be IPv6 only if that's what's desired.
Yes, i had thought about this before, maybe we can add a new function to get(or chose) the IPv6 address we desired, because one interface can have many IPv6 address and sometimes we just need one of them.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 645aef1..f60911c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1659,6 +1659,7 @@ virNetDevDelMulti; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetIPv6Address; virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43..c1a588e 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,72 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFADDR */
+/** + *virNetDevGetIPv6Address: + *@ifname: name of the interface whose IP address we want s/IP/IPv6
thanks, but seems this function will be removed (or renamed) in next version :)
+ *@addr: filled with the IPv6 address + * + *This function gets the IPv6 address for the interface @ifname + *and stores it in @addr + * + *Returns 0 on success, -1 on failure. + */ Each of the lines above needs s/*/* /
Sorry for my careless.
+#if defined(HAVE_GETIFADDRS) +int virNetDevGetIPv6Address(const char *ifname, + virSocketAddrPtr addr) +{ + struct ifaddrs *ifap, *ifa; + int ret = -1; + bool found_one = false; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list")); s/list$/list for '%s'/ and of course an ifname parameter ...
Hmm, seems it is not the interface issue when getifaddrs cannot get interface list, however it will give a more nice error. Thanks your advise and i will change it in next version.
+ return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (STRNEQ(ifa->ifa_name, ifname)) + continue; + found_one = true; + if (ifa->ifa_addr->sa_family == AF_INET6) + break; From the getifaddrs(3) man page:
"The ifa_addr field points to a structure containing the interface address. (The sa_family subfield should be consulted to determine the format of the address structure.) This field may contain a null pointer."
So you need to check that here before de-referencing a NULL pointer
Oh, thanks for pointing out this.
Also I'm assuming these API's don't care how usable the address is, just that it's the first one found. See 'checkProtocols()' for what I mean about usable - there's an additional getaddrinfo() call there that handles a special IPv6 case (I forget all the details, but the ::1 has some special characteristics...).
Thanks for your remind, i checked checkProtocols() and i guess your mean we can use getaddrinfo() to make sure the address we get can be used for a socket bind ? And i also thought about another issue after your reminding: An interface can have more than one IPv6 address. But i still couldn't find a good way until now to chose which IPv6 address if we find more than one IPv6 address in one interface (maybe users should use "listen type=address" instead of "listen type=network" in this case ;)). There are some special addresses anddefault address selection for IPv6 address, i don't know if it is a good idea to guess which address is the caller really desired if we get some IPv6 address in one interface.
+ } + + if (!ifa) { + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' do not have a IPv6 address"), s/do/does/
+ ifname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + goto cleanup; + } + + addr->data.stor.ss_family = AF_INET6; + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len);
I'd also move this chunk inside that loop above replacing the "break;" with this code plus of course the following ret = 0; and a goto cleanup.
Allowing then the fall of the end of the loop code to just check if (found_one) or not (eg, no need for "if (!ifa)"...
Leaving the following - including the capability to get either IPv6 or IPv4 address depending upon what's desired/required:
for (ifa = ifap; ifa; ifa = ifa->ifa_next) { if (!ifa->ifa_addr || STRNEQ(ifa->ifa_name, ifname)) continue; found_one = true; if (want_ipv6 && ifa->ifa_addr->sa_family != AF_INET6) continue;
/* Found an address to return */ addr->data.stor.ss_family = ifa->ifa_addr->sa_family; if (ifa->ifa_addr->sa_family == AF_INET6) 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); } ret = 0; goto cleanup; }
if (found_one) virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' does not have an %s address"), ifname, want_ipv6 ? "IPv6" : "IPv4"); else virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' not found"), ifname);
cleanup:
Thanks for your code and i have changed some place, and now : + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (!ifa->ifa_addr || + STRNEQ(ifa->ifa_name, ifname)) { + continue; + } + found_one = true; + if (want_ipv6 && ifa->ifa_addr->sa_family == AF_INET6) { + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); + } else if (!want_ipv6 && ifa->ifa_addr->sa_family == AF_INET) { + addr->len = sizeof(addr->data.inet4); + memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len); + } else { + continue; + } + addr->data.stor.ss_family = ifa->ifa_addr->sa_family; + ret = 0; + goto cleanup; + } + + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' does not have a %s address"), + ifname, want_ipv6 ? "IPv6" : "IPv4"); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + + cleanup: Luyao