10:37 a.m.
On 03/09/2015 07:50 AM, John Ferlan wrote:
On 02/28/2015 04:08 AM, Luyao Huang wrote:
> Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6
> and IPv4 address.
>
> Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress
> and virNetDevGetIPv4Address to get IP address.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> v2: add a new function virNetDevGetIPAddress to call virNetDevGetIfaddrsAddress
> , and make virNetDevGetIfaddrsAddress can get ipv4 address for a interface.
> Also add a error output in virNetDevGetIfaddrsAddress when get multiple ip
> address for a host interface.
>
> src/libvirt_private.syms | 1 +
> src/util/virnetdev.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 4 ++
> 3 files changed, 103 insertions(+)
>
...
> + if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) {
> + if (++nIPaddr > 1)
> + break;
[1]... hmm not sure about this...
> + if (want_ipv6) {
> + 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);
> + }
> + addr->data.stor.ss_family = ifa->ifa_addr->sa_family;
> + }
> + }
> +
> + if (nIPaddr == 1)
> + ret = 0;
> + else if (nIPaddr > 1)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Interface '%s' has multiple %s
address"),
s/address/addresses
> + ifname, want_ipv6 ? "IPv6" : "IPv4");
[1]
But is this really desired... It seems from the previous review, if
someone wants a specific address they use "<listen
type='address'...".
Otherwise, they use this function. Since you'll be returning either an
IPv4 or IPv6 address here you'd be causing an error here if the network
had more than one IPv4 address defined; whereas, the previous code just
returned the "first" from the ioctl(SIOCGIFADDR...).
I think once you have an address you just return the first one found and
document it that way avoiding this path completely. You could also note
that if you want a specific address use the type='address'
I had an idea but my eyes almost close ;) so i write here without test
them.
I think it will be better if we make this function to get mutli address
and return the number of address we get, like this:
+static int
+virNetDevGetIfaddrsAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr *addrs)
+{
+ struct ifaddrs *ifap, *ifa;
+ int nIPaddr = 0;
+
+ if (getifaddrs(&ifap) < 0) {
+ virReportSystemError(errno,
+ _("Could not get interface list for
'%s'"),
+ ifname);
+ return -1;
+ }
+
+ for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+ virSocketAddrPtr tmpaddr;
+
+ if (!ifa->ifa_addr ||
+ STRNEQ_NULLABLE(ifa->ifa_name, ifname)) {
+ continue;
+ }
+
+ if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) {
+ memset(tmpaddr, 0, sizeof(*tmpaddr));
+
+ if (!ifa->ifa_addr ||
+ STRNEQ_NULLABLE(ifa->ifa_name, ifname)) {
+ continue;
+ }
+
+ if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) {
+ memset(tmpaddr, 0, sizeof(*tmpaddr));
+
+ if (want_ipv6) {
+ tmpaddr->len = sizeof(tmpaddr->data.inet6);
+ memcpy(&tmpaddr->data.inet6, ifa->ifa_addr, tmpaddr->len);
+ } else {
+ tmpaddr->len = sizeof(tmpaddr->data.inet4);
+ memcpy(&tmpaddr->data.inet4, ifa->ifa_addr, tmpaddr->len);
+ }
+ tmpaddr->data.stor.ss_family = ifa->ifa_addr->sa_family;
+ addrs[nIPaddr++] = tmpaddr;
+ }
+ }
+ if (nIPaddr == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Interface '%s' not found"),
+ ifname);
+ return -1;
+ }
+ freeifaddrs(ifap);
+ return nIPaddr;
+}
> +int virNetDevGetIPAddress(const char *ifname,
> + bool want_ipv6,
> + virSocketAddrPtr addr)
and then rename this function
to virNetDevGetOneIPAddress()
and rework the code like this:
+int virNetDevGetOneIPAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
+{
+ int ret;
+ virSocketAddrPtr *addrs = NULL;
+
+ ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addrs);
+ if (ret > 0) {
+ addr = addrs[0];
+ } else if (ret == -2 && !want_ipv6) {
+ return virNetDevGetIPv4Address(ifname, addr);
+ }
+
+ return ret;
+}
...
These changes require modifying src/network/bridge_driver.c (temporarily
until we add the IPv6 aware code later):
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 2a61991..7d6e173 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4608,7 +4608,7 @@ networkGetNetworkAddress(const char *netname, char
**netaddr)
}
if (dev_name) {
- if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
+ if (virNetDevGetIPAddress(dev_name, false, &addr) < 0)
goto cleanup;
addrptr = &addr;
}
At last, add the multiple address check to here and this place will like
this:
+ int num = virNetDevGetOneIPAddress(dev_name, want_ipv6, &addr);
+ if (num > 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("interface '%s' has multiple %s
addresses"),
+ dev_name, want_ipv6 ? "IPv6" : "IPv4");
+ goto cleanup;
+ } else if (num < 0) {
goto cleanup;
+ }
because i am not test them so there will be some mistake, I will test
them tomorrow, hope it will work :)
John
Luyao