On 03/09/2015 03:35 AM, lhuang wrote:
<...snip...>
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
Right and as Laine points out: "... Because it's impossible to do it right, I say we shouldn't even try; it would be worse for it to work halfway." Doing it "right" thus requires proper configuration rather than "premonition" by libvirt. I think a follow up patch could describe the migration conundrum where if the plan is to migrate the vm, then the target node will have to have a "valid" IPv{4|6} address as the first address; otherwise, the connection will be broken. Migration is a tricky beast. I recall doing something for a former project regarding this kind of issue, but I cannot remember the details. I do remember though our logic filtered out a few address types before returning what "should" be a usable address. I also recall some sort of nasty ARP test, but the details were specific to the HP-UX world I used to live in. <...snip...>
+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)
Hmm... if we return -2, then the caller's caller spits out "network-based listen not possible, network driver not present" rather than our message... Which is less than helpful. I still have to process your follow-up email with a patch in it, but my guess is I'm going to repost the patches I have so that we're on the same page. John