[libvirt] [PATCH 1/4] util: introduce a new helper for get interface IPv6 address
lhuang
lhuang at redhat.com
Wed Feb 25 09:50:10 UTC 2015
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 at 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
More information about the libvir-list
mailing list