Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
On 03/10/2015 06:50 PM, John Ferlan wrote:
On 03/10/2015 06:03 AM, lhuang wrote:
<...snip...>
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?
Sorry, i don't know what these words' ("how do you use it ?") mean.
My point was - how does the existing code decide which of the 'n'
addresses found/returned to use as "the" address?
Oh, got it, i don't want to use it for decide use which address in those
addresses, i just want to move the check and chose in the function which
call it (virNetDevGetOneIPAddress()). Also if other people need a
function to get all the address for one interface in the future, then
virNetDevGetIfaddrsAddress() will help him (although i can not find a
place need this until now :-( ).
I guess your mean is ask me how to use the code or function you
mentioned, i think maybe
we can avoid to use it :)
However this should be another patch which add a function to get a list
of ip address.
Sure, but would it be just for display purposes only? Once there is more
than one address per dev_name being queried, code that wants to use "an"
address will need to have some decision point in order to "filter" out
addresses known to be bad. Perhaps this is done by type - multicast,
localnet, etc or perhaps by some other lookup mechanism. I'm thinking of
the netinet/in.h macros (search on MULTICAST, LOCAL, LOOPBACK, etc.).
Whatever "filters" are desired, they could be added as an attribute list
of sorts (eg, filter='multicast,local') that way it's up to the consumer
to decide which filters apply knowing what that filter maps to. In the
example you provided ("2001:db8:ca2:2::1/64") - what about that address
made it unusable? That's what needs to be filtered... Doing a google
search on the address ironically points a bz that Laine owns... I'm not
"up" on all the IPv6 addressing rules, so my view is a more high level...
As you said, if we want to get a right address, we need more information
from the user (address type...), and still need some (or a lot ) patches
for this. However, I still think it won't work for every scene even we
add a lot of filters and check, because we cannot know and control
everything in the different env :)
So as Jan's advise, hooks can be a good choice in the different scene.
John
Luyao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
On 03/10/2015 06:03 AM, lhuang wrote:
<...snip...>
>> 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?
>
> Sorry, i don't know what these words' ("how do you use it ?") mean.
>
My point was - how does the existing code decide which of the 'n'
addresses found/returned to use as "the" address?
> I guess your mean is ask me how to use the code or function you
> mentioned, i think maybe
> we can avoid to use it :)
>
> However this should be another patch which add a function to get a list
> of ip address.
>
Sure, but would it be just for display purposes only? Once there is more
than one address per dev_name being queried, code that wants to use "an"
address will need to have some decision point in order to "filter" out
addresses known to be bad. Perhaps this is done by type - multicast,
localnet, etc or perhaps by some other lookup mechanism. I'm thinking of
the netinet/in.h macros (search on MULTICAST, LOCAL, LOOPBACK, etc.).
Whatever "filters" are desired, they could be added as an attribute list
of sorts (eg, filter='multicast,local') that way it's up to the consumer
to decide which filters apply knowing what that filter maps to. In the
example you provided ("2001:db8:ca2:2::1/64") - what about that address
made it unusable? That's what needs to be filtered... Doing a google
search on the address ironically points a bz that Laine owns... I'm not
"up" on all the IPv6 addressing rules, so my view is a more high level...
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
On 03/10/2015 07:50 AM, John Ferlan wrote:
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
---
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 "
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.
Yes, i forgot this and please ignore the code, it has so many mistake
(so it is not a good idea to write a patch when you want sleep:-( )
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?
Sorry, i don't know what these words' ("how do you use it ?") mean.
I guess your mean is ask me how to use the code or function you
mentioned, i think maybe
we can avoid to use it :)
However this should be another patch which add a function to get a list
of ip address.
John
Luyao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
On 03/10/2015 07:44 AM, John Ferlan wrote:
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.
yes, migration part need more patch and more intelligence :)
<...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.
right, i forgot this
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.
Thanks a lot for your help !
John
Luyao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>>> ---
>>> 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 ">
>> 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 virN
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
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
---
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 "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)
+{
+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, a
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
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
---
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
+#endif
+
#include
#include
#include
@@ -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 "
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 w
Re: [libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
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
> ---
> 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...
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
> 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
> 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
> +#endif
> +
> #include
> #include
> #include
> @@ -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)
> +{
> +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 " +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 see
[libvirt] [PATCHv2 1/4] util: introduce 2 new helpers for get interface IPv6 address
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
---
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(+)
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;
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
+#endif
+
#include
#include
#include
@@ -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)
+{
+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;
+}
+if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) {
+if (++nIPaddr > 1)
+break;
+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"),
+ ifname, want_ipv6 ? "IPv6" : "IPv4");
+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)
+{
+virReportSystemError(ENOSYS,
+ _("Unable to get %s address on this platform"),
+ want_ipv6 ? "IPv6" : "IPv4");
+return -2;
+}
+
+#endif
+
+
+int virNetDevGetIPAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
+{
+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);
+
+return -1;
+}
/**
* 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;
+int virNetDevGetIPAddress(const char *ifname,
+ bool want_ipv6,
+ virSocketAddrPtr addr)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
int virNetDevSetMAC(const char *ifn
