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

On 03/09/2015 11:37 AM, Luyao Huang wrote: > 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&gt; >>> --- >>> 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) We'd need to have an naddrs to know how many we can stuff in... or you'd need to do the *REALLOC on the fly in this module for each address found.

Interesting idea, but you'd be just throwing things away. I could perhaps having some code "periodically" cache addresses... or cache addresses, subscribe to some event to let you know when a new address is generated so you can add it to your list (if there is one)... But, how do you use it?

John