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@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(+)
>>>
> ...
>>
>>> + 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
> +{
> + 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list