Re: [libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address

On 02/13/2015 02:17 AM, Luyao Huang wrote: s/Introduce a new function to help to/Introduce virNetDevGetIPv6Address/ 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. 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. s/IP/IPv6 Each of the lines above needs s/*/* / s/list$/list for '%s'/ and of course an ifname parameter ... "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 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...). s/do/does/ 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: