Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address

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