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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]