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@xxxxxxxxxx>
---
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(+)
Closer... But still missing a couple of things, which I can add for
you. I'll make my comments and do the changes locally but not commit
until Mon afternoon (Eastern US) so if Laine wants to comment he can and
of course that you agree with my comments...
Thanks in advance for your help
Going to split this commit into two -
The first commit will just make the virNetDevIPAddress shim, moving the
virNetDevIPv4Address to a static function.
The second commit will add the IPv6 option to virNetDevIPAddress
No problem
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ba05cc6..f88626a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1670,6 +1670,7 @@ virNetDevClearIPAddress;
virNetDevDelMulti;
virNetDevExists;
virNetDevGetIndex;
+virNetDevGetIPAddress;
virNetDevGetIPv4Address;
We can remove the GetIPv4Address and make it static to virtnetdev.c
Yes
virNetDevGetLinkInfo;
virNetDevGetMAC;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2a0eb43..318c3b6 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,100 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFADDR */
+/**
+ * virNetDevGetIfaddrsAddress:
+ * @ifname: name of the interface whose IP address we want
+ * @want_ipv6: get IPv4 or IPv6 address
+ * @addr: filled with the IP address
+ *
+ * This function gets the IP address for the interface @ifname
+ * and stores it in @addr
+ *
+ * Returns 0 on success, -1 on failure, -2 on unsupported.
+ */
+#if defined(HAVE_GETIFADDRS)
+static int virNetDevGetIfaddrsAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
Although not a rule - we try to make newer API's as:
static int
fname(param1,
param2)
oh, i should noticed this
+{
+ struct ifaddrs *ifap, *ifa;
+ int ret = -1;
+ 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) {
+ if (!ifa->ifa_addr ||
+ STRNEQ(ifa->ifa_name, ifname)) {
+ continue;
+ }
STRNEQ_NULLABLE does the trick...
+ 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'
Hmm, yes, I agree it is not a good idea to report error when we get
multiple addresses in this function (In some case, caller just want one
ip address and not care about which one we chose), so remove the check
and error output here is reasonable (maybe we can use a DEBUG or WARNING
here complain about this and set ret to 0).
However this check and error output is a result from last review, i
think maybe we can move them to a right place (should in another patch).
Because we use listen type='network' for migration in the most case, and
if a host interface has multiple address than we always chose the first
one, this will make things worse in some case. An example:
In host A, we have a happy vm which use listen type='network' and use a
spice client to connect to it (listen address is
"fe80::7ae3:b5ff:fec5:189e%enp1s0" and address is get from an interface
via virNetDevGetIfaddrsAddress() ), than we migrate this vm to a another
host B, we found a network have the same name and use a host interface
in host B, we use virNetDevGetIfaddrsAddress() get the first ipv6
address and we get "2001:db8:ca2:2::1/64", unfortunately this address is
not a "good" address (the good one is the second one :-P ) and spice
connection will be broken, and the user will say : "why libvirt chose a
wrong address and broke my connection :-(".
NB: the comment from Laine in this mail:
https://www.redhat.com/archives/libvir-list/2015-February/msg01080.html
+ else
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Interface '%s' not found"),
+ ifname);
+
+ freeifaddrs(ifap);
+ return ret;
+}
+
+#else
+
+static int virNetDevGetIfaddrsAddress(const char *ifname ATTRIBUTE_UNUSED,
+ bool want_ipv6,
+ virSocketAddrPtr addr ATTRIBUTE_UNUSED)
ditto on function definition
+{
+ virReportSystemError(ENOSYS,
+ _("Unable to get %s address on this platform"),
+ want_ipv6 ? "IPv6" : "IPv4");
While this seems appropriate, because you potentially call
virNetDevGetIPv4Address below that means it's possible we return with
this error message set, but a good status or we overwrite some other
message... So I'll move it...
Yes, i forgot remove error message when wrote v2, thanks for pointing out.
+ return -2;
+}
+
+#endif
+
+
+int virNetDevGetIPAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
same
+{
+ 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);
Here if we have virNetDevGetIPv4Address() return -2, then we can take
our message above and place it right here while also adjusting the "!
SIOCGIFADDR" to just return -2.
+
+ return -1;
+}
NOTE: This does not return -2 in any
should be return -2 (and this only happen when want_ipv6 is true and the
ret is -2)
/**
* virNetDevValidateConfig:
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index de8b480..faf3b2f 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -104,6 +104,10 @@ 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;
Removing the IPv4Address API
+int virNetDevGetIPAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
int virNetDevSetMAC(const char *ifname,
These changes require modifying src/network/bridge_driver.c (temporarily
until we add the IPv6 aware code later):
Yes, i should move this modifying from another patch to this patch.
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;
}
John
Thanks for your review.
Luyao
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list