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@xxxxxxxxxx>
> ---
> 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.
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.
> 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
> + *@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/*/* /
> +#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 ...
> + 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
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...).
> + }
> +
> + 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:
> + ret = 0;
> +
> + cleanup:
> + freeifaddrs(ifap);
> + return ret;
> +}
> +
> +#else
> +
> +int virNetDevGetIPv6Address(const char *ifname ATTRIBUTE_UNUSED,
> + virSocketAddrPtr addr ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Unable to get IPv6 address on this platform"));
> + return -1;
> +}
> +
> +#endif
> +
>
> /**
> * virNetDevValidateConfig:
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index de8b480..c065381 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -104,6 +104,8 @@ 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;
> +int virNetDevGetIPv6Address(const char *ifname, virSocketAddrPtr addr)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>
>
> int virNetDevSetMAC(const char *ifname,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list