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

lhuang lhuang at redhat.com
Mon Mar 9 07:35:03 UTC 2015


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 at redhat.com>
>> ---
>> 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(+)
>>
> Closer...  But still missing a couple of things, which I can add for
> you. I'll make my comments and do the changes locally but not commit
> until Mon afternoon (Eastern US) so if Laine wants to comment he can and
> of course that you agree with my comments...

Thanks in advance for your help
> Going to split this commit into two -
>
> The first commit will just make the virNetDevIPAddress shim, moving the
> virNetDevIPv4Address to a static function.
>
> The second commit will add the IPv6 option to virNetDevIPAddress
>

No problem
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ba05cc6..f88626a 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1670,6 +1670,7 @@ virNetDevClearIPAddress;
>>   virNetDevDelMulti;
>>   virNetDevExists;
>>   virNetDevGetIndex;
>> +virNetDevGetIPAddress;
>>   virNetDevGetIPv4Address;
> We can remove the GetIPv4Address and make it static to virtnetdev.c

Yes
>>   virNetDevGetLinkInfo;
>>   virNetDevGetMAC;
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 2a0eb43..318c3b6 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -33,6 +33,10 @@
>>   #include "virstring.h"
>>   #include "virutil.h"
>>   
>> +#if defined(HAVE_GETIFADDRS)
>> +# include <ifaddrs.h>
>> +#endif
>> +
>>   #include <sys/ioctl.h>
>>   #include <net/if.h>
>>   #include <fcntl.h>
>> @@ -1432,6 +1436,100 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,
>>   
>>   #endif /* ! SIOCGIFADDR */
>>   
>> +/**
>> + * virNetDevGetIfaddrsAddress:
>> + * @ifname: name of the interface whose IP address we want
>> + * @want_ipv6: get IPv4 or IPv6 address
>> + * @addr: filled with the IP address
>> + *
>> + * This function gets the IP address for the interface @ifname
>> + * and stores it in @addr
>> + *
>> + * Returns 0 on success, -1 on failure, -2 on unsupported.
>> + */
>> +#if defined(HAVE_GETIFADDRS)
>> +static int virNetDevGetIfaddrsAddress(const char *ifname,
>> +                                      bool want_ipv6,
>> +                                      virSocketAddrPtr addr)
> Although not a rule - we try to make newer API's as:
>
> static int
> fname(param1,
>        param2)
>

oh, i should noticed this
>> +{
>> +    struct ifaddrs *ifap, *ifa;
>> +    int ret = -1;
>> +    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) {
>> +        if (!ifa->ifa_addr ||
>> +            STRNEQ(ifa->ifa_name, ifname)) {
>> +            continue;
>> +        }
> STRNEQ_NULLABLE does the trick...
>
>> +        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'
>

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

>> +    else
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Interface '%s' not found"),
>> +                       ifname);
>> +
>> +    freeifaddrs(ifap);
>> +    return ret;
>> +}
>> +
>> +#else
>> +
>> +static int virNetDevGetIfaddrsAddress(const char *ifname ATTRIBUTE_UNUSED,
>> +                                      bool want_ipv6,
>> +                                      virSocketAddrPtr addr ATTRIBUTE_UNUSED)
> ditto on function definition
>
>> +{
>> +    virReportSystemError(ENOSYS,
>> +                         _("Unable to get %s address on this platform"),
>> +                         want_ipv6 ? "IPv6" : "IPv4");
> While this seems appropriate, because you potentially call
> virNetDevGetIPv4Address below that means it's possible we return with
> this error message set, but a good status or we overwrite some other
> message...  So I'll move it...

Yes, i forgot remove error message when wrote v2, thanks for pointing out.
>
>> +    return -2;
>> +}
>> +
>> +#endif
>> +
>> +
>> +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)
>>   
>>   /**
>>    * virNetDevValidateConfig:
>> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
>> index de8b480..faf3b2f 100644
>> --- a/src/util/virnetdev.h
>> +++ b/src/util/virnetdev.h
>> @@ -104,6 +104,10 @@ int virNetDevClearIPAddress(const char *ifname,
>>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>>   int virNetDevGetIPv4Address(const char *ifname, virSocketAddrPtr addr)
>>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> Removing the IPv4Address API
>
>> +int virNetDevGetIPAddress(const char *ifname,
>> +                          bool want_ipv6,
>> +                          virSocketAddrPtr addr)
>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
>>   
>>   
>>   int virNetDevSetMAC(const char *ifname,
>>
>
> These changes require modifying src/network/bridge_driver.c (temporarily
> until we add the IPv6 aware code later):

Yes, i should move this modifying from another patch to this patch.
> 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;
>       }
>
>
>
>
> John
Thanks for your review.

Luyao




More information about the libvir-list mailing list