3:50 a.m.
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(a)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