3:08 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1192318
We already support add a IPv6 address to graphics listen address
and xml like this:
<graphics type='spice' port='5900' autoport='yes'
listen='2001b8:ca2:2::1' keymap='en-us'>
<listen type='address' address='2001b8:ca2:2::1'/>
</graphics>
However we do not support get a IPv6 address for the network.
add some helpers and rework networkGetNetworkAddress to make it
can get a IPv6 address when we need.
Luyao Huang (4):
util: introduce 2 new helpers for get interface IPv6 address
conf: introduce new family attribute in graphics listen for chose IP
family
network: rework networkGetNetworkAddress to make it can get IPv6
address
qemu: fix a error coverd issue in 2 place
docs/formatdomain.html.in | 10 ++-
docs/schemas/domaincommon.rng | 8 ++
src/conf/domain_conf.c | 20 +++++
src/conf/domain_conf.h | 9 ++
src/libvirt_private.syms | 1 +
src/network/bridge_driver.c | 15 ++--
src/network/bridge_driver.h | 6 +-
src/qemu/qemu_command.c | 22 +++--
src/util/virnetdev.c | 98 ++++++++++++++++++++++
src/util/virnetdev.h | 4 +
.../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++
.../qemuxml2argv-graphics-listen-network.xml | 2 +-
.../qemuxml2xmlout-graphics-listen-network2.xml | 2 +-
tests/qemuxml2xmltest.c | 1 +
14 files changed, 209 insertions(+), 24 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
--
1.8.3.1
3:08 a.m.
New subject: [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 <lhuang(a)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(+)
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 <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)
+{
+ 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 *ifname,
--
1.8.3.1
6:50 p.m.
New subject: [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 <lhuang(a)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...
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 <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)
+{
+ 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'
+ 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...
+ 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
/**
* 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):
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
2:35 a.m.
New subject: [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 <lhuang(a)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
6:44 p.m.
New subject: [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
10:44 p.m.
New subject: [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
10:37 a.m.
New subject: [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 <lhuang(a)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(+)
>
...
> + 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)
+{
+ 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
6:50 p.m.
New subject: [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>
>> ---
>> 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
5:03 a.m.
New subject: [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 <lhuang(a)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(+)
>>>
> ...
>>> + 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.
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
5:50 a.m.
New subject: [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
10:11 a.m.
New subject: [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
3:08 a.m.
New subject: [libvirt] [PATCHv2 2/4] conf: introduce new family attribute in graphics listen for chose IP family
If a interface or network have both ipv6 and ipv4 address which can be used,
we do not know use which be a listen address. So introduce a new attribute
to help us chose this.
graphics XML will like this after this commit:
<graphics type='spice' port='5900' autoport='yes'>
<listen type='network' address='192.168.0.1'
network='vepa-net' family='ipv4'/>
</graphics>
and this ip family can be set 2 type, and the default one is ipv4:
ipv4: check if the interface or network have a can be used ipv4 address
ipv6: check if the interface or network have a can be used ipv6 address
fix some test which will be break by this commit and add a new test.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
v2: remove family='default' and add a test.
docs/formatdomain.html.in | 10 ++++++-
docs/schemas/domaincommon.rng | 8 +++++
src/conf/domain_conf.c | 20 +++++++++++++
src/conf/domain_conf.h | 9 ++++++
.../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++++++++++++++++
.../qemuxml2argv-graphics-listen-network.xml | 2 +-
.../qemuxml2xmlout-graphics-listen-network2.xml | 2 +-
tests/qemuxml2xmltest.c | 1 +
8 files changed, 84 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fb0a0d1..e8ea6fa 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null
<graphics type='rdp' autoport='yes' multiUser='yes'
/>
<graphics type='desktop' fullscreen='yes'/>
<graphics type='spice'>
- <listen type='network' network='rednet'/>
+ <listen type='network' network='rednet'
family='ipv4'/>
</graphics>
</devices>
...</pre>
@@ -4785,6 +4785,14 @@ qemu-kvm -net nic,model=? /dev/null
the first forward dev will be used.
</dd>
</dl>
+ <dl>
+ <dt><code>family</code></dt>
+ <dd>if <code>type='network'</code>, the
<code>family</code>
+ attribute will contain an IP family. This tells which IP address
+ will be got for the network. IP family can be set to ipv4
+ or ipv6.
+ </dd>
+ </dl>
<h4><a name="elementsVideo">Video devices</a></h4>
<p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4e4fe9f..e84b213 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2909,6 +2909,14 @@
<ref name="addrIPorName"/>
</attribute>
</optional>
+ <optional>
+ <attribute name="family">
+ <choice>
+ <value>ipv4</value>
+ <value>ipv6</value>
+ </choice>
+ </attribute>
+ </optional>
</group>
</choice>
</element>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9b7ae3f..97f1250 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen,
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST,
"address",
"network")
+VIR_ENUM_IMPL(virDomainGraphicsListenFamily,
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST,
+ "ipv4",
+ "ipv6")
+
VIR_ENUM_IMPL(virDomainGraphicsAuthConnected,
VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST,
"default",
@@ -9464,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
char *address = virXMLPropString(node, "address");
char *network = virXMLPropString(node, "network");
char *fromConfig = virXMLPropString(node, "fromConfig");
+ char *family = virXMLPropString(node, "family");
int tmp;
if (!type) {
@@ -9501,6 +9507,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
network = NULL;
}
+ if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
+ if (!family) {
+ def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4;
+ } else if ((def->family = virDomainGraphicsListenFamilyTypeFromString(family))
< 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown graphics listen IP family '%s'"),
family);
+ goto error;
+ }
+ }
+
if (fromConfig &&
flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) {
@@ -9520,6 +9536,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
VIR_FREE(address);
VIR_FREE(network);
VIR_FREE(fromConfig);
+ VIR_FREE(family);
return ret;
}
@@ -19084,6 +19101,9 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf,
if (def->network &&
(def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
virBufferEscapeString(buf, " network='%s'", def->network);
+
+ virBufferAsprintf(buf, " family='%s'",
+ virDomainGraphicsListenFamilyTypeToString(def->family));
}
if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 02ddd93..afffe9c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1441,6 +1441,13 @@ typedef enum {
} virDomainGraphicsListenType;
typedef enum {
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4,
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6,
+
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST
+} virDomainGraphicsListenFamily;
+
+typedef enum {
VIR_DOMAIN_HUB_TYPE_USB,
VIR_DOMAIN_HUB_TYPE_LAST
@@ -1453,6 +1460,7 @@ struct _virDomainGraphicsListenDef {
char *address;
char *network;
bool fromConfig; /* true if the @address is config file originated */
+ int family; /*enum virDomainGraphicsListenFamily*/
};
struct _virDomainGraphicsDef {
@@ -2866,6 +2874,7 @@ VIR_ENUM_DECL(virDomainInput)
VIR_ENUM_DECL(virDomainInputBus)
VIR_ENUM_DECL(virDomainGraphics)
VIR_ENUM_DECL(virDomainGraphicsListen)
+VIR_ENUM_DECL(virDomainGraphicsListenFamily)
VIR_ENUM_DECL(virDomainGraphicsAuthConnected)
VIR_ENUM_DECL(virDomainGraphicsSpiceChannelName)
VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
new file mode 100644
index 0000000..6cce7a8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <graphics type='vnc' port='5903' autoport='no'>
+ <listen type='network' network='Bobsnetwork'
family='ipv6'/>
+ </graphics>
+ <video>
+ <model type='cirrus' vram='16384' heads='1'/>
+ </video>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
index bf78ca8..3b5c2de 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
@@ -25,7 +25,7 @@
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<graphics type='vnc' port='5903' autoport='no'>
- <listen type='network' network='Bobsnetwork'/>
+ <listen type='network' network='Bobsnetwork'
family='ipv4'/>
</graphics>
<video>
<model type='cirrus' vram='16384' heads='1'/>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
index abee7b6..2b2d78a 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
@@ -26,7 +26,7 @@
<input type='keyboard' bus='ps2'/>
<graphics type='vnc' port='-1' autoport='yes'
listen='1.2.3.4'>
<listen type='address' address='1.2.3.4'/>
- <listen type='network' network='Bobsnetwork'/>
+ <listen type='network' network='Bobsnetwork'
family='ipv4'/>
</graphics>
<video>
<model type='cirrus' vram='16384' heads='1'/>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 99d4629..c966eb5 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -248,6 +248,7 @@ mymain(void)
DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE);
DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE);
DO_TEST("graphics-listen-network");
+ DO_TEST("graphics-listen-network-ipv6");
DO_TEST("graphics-vnc");
DO_TEST("graphics-vnc-websocket");
DO_TEST("graphics-vnc-sasl");
--
1.8.3.1
6:50 p.m.
New subject: [libvirt] [PATCHv2 2/4] conf: introduce new family attribute in graphics listen for chose IP family
On 02/28/2015 04:08 AM, Luyao Huang wrote:
If a interface or network have both ipv6 and ipv4 address which can
be used,
we do not know use which be a listen address. So introduce a new attribute
to help us chose this.
graphics XML will like this after this commit:
<graphics type='spice' port='5900' autoport='yes'>
<listen type='network' address='192.168.0.1'
network='vepa-net' family='ipv4'/>
</graphics>
and this ip family can be set 2 type, and the default one is ipv4:
ipv4: check if the interface or network have a can be used ipv4 address
ipv6: check if the interface or network have a can be used ipv6 address
fix some test which will be break by this commit and add a new test.
Adjusting the commit text slightly to match the review below
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
v2: remove family='default' and add a test.
docs/formatdomain.html.in | 10 ++++++-
docs/schemas/domaincommon.rng | 8 +++++
src/conf/domain_conf.c | 20 +++++++++++++
src/conf/domain_conf.h | 9 ++++++
.../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++++++++++++++++
.../qemuxml2argv-graphics-listen-network.xml | 2 +-
.../qemuxml2xmlout-graphics-listen-network2.xml | 2 +-
tests/qemuxml2xmltest.c | 1 +
8 files changed, 84 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fb0a0d1..e8ea6fa 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null
<graphics type='rdp' autoport='yes' multiUser='yes'
/>
<graphics type='desktop' fullscreen='yes'/>
<graphics type='spice'>
- <listen type='network' network='rednet'/>
+ <listen type='network' network='rednet'
family='ipv4'/>
</graphics>
</devices>
...</pre>
@@ -4785,6 +4785,14 @@ qemu-kvm -net nic,model=? /dev/null
the first forward dev will be used.
</dd>
</dl>
+ <dl>
+ <dt><code>family</code></dt>
+ <dd>if <code>type='network'</code>, the
<code>family</code>
+ attribute will contain an IP family. This tells which IP address
+ will be got for the network. IP family can be set to ipv4
+ or ipv6.
Adjusted to:
<dd>if <code>type='network'</code>, the
<code>family</code>
attribute may contain the IP family. The <code>family</code>
can be set to either <code>ipv4</code> or
<code>ipv6</code>.
This advises the graphics device which IP address family
to use as listen address for the network. The listen address
used will be the first found address of the <code>family</code>
type defined for the host.
<span class="since">Since 1.2.14</span>
+ </dd>
+ </dl>
<h4><a name="elementsVideo">Video
devices</a></h4>
<p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4e4fe9f..e84b213 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2909,6 +2909,14 @@
<ref name="addrIPorName"/>
</attribute>
</optional>
+ <optional>
+ <attribute name="family">
+ <choice>
+ <value>ipv4</value>
+ <value>ipv6</value>
+ </choice>
+ </attribute>
+ </optional>
</group>
</choice>
</element>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9b7ae3f..97f1250 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen,
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST,
"address",
"network")
+VIR_ENUM_IMPL(virDomainGraphicsListenFamily,
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST,
+ "ipv4",
+ "ipv6")
+
Need to add a "none"... As with the previous review - adding an
attribute that's optional, but then generating it on output isn't good.
So we need a way to signify it didn't exist on input. If provided that's
fine, we can output it. If not provided, then sure we're going to
eventually default to IPv4, but that's why I said to have the boolean be
"if ipv6 desired"...
VIR_ENUM_IMPL(virDomainGraphicsAuthConnected,
VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST,
"default",
@@ -9464,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
char *address = virXMLPropString(node, "address");
char *network = virXMLPropString(node, "network");
char *fromConfig = virXMLPropString(node, "fromConfig");
+ char *family = virXMLPropString(node, "family");
int tmp;
if (!type) {
@@ -9501,6 +9507,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
network = NULL;
}
+ if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
+ if (!family) {
+ def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4;
strike this check
+ } else if ((def->family =
virDomainGraphicsListenFamilyTypeFromString(family)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown graphics listen IP family
'%s'"), family);
+ goto error;
+ }
+ }
+
if (fromConfig &&
flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) {
@@ -9520,6 +9536,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr
def,
VIR_FREE(address);
VIR_FREE(network);
VIR_FREE(fromConfig);
+ VIR_FREE(family);
return ret;
}
@@ -19084,6 +19101,9 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf,
if (def->network &&
(def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
virBufferEscapeString(buf, " network='%s'", def->network);
+
Add a condition
if (def->family != VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE)
+ virBufferAsprintf(buf, " family='%s'",
+ virDomainGraphicsListenFamilyTypeToString(def->family));
}
if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 02ddd93..afffe9c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1441,6 +1441,13 @@ typedef enum {
} virDomainGraphicsListenType;
typedef enum {
Add:
VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE = 0,
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4,
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6,
+
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST
+} virDomainGraphicsListenFamily;
+
+typedef enum {
VIR_DOMAIN_HUB_TYPE_USB,
VIR_DOMAIN_HUB_TYPE_LAST
@@ -1453,6 +1460,7 @@ struct _virDomainGraphicsListenDef {
char *address;
char *network;
bool fromConfig; /* true if the @address is config file originated */
+ int family; /*enum virDomainGraphicsListenFamily*/
};
struct _virDomainGraphicsDef {
@@ -2866,6 +2874,7 @@ VIR_ENUM_DECL(virDomainInput)
VIR_ENUM_DECL(virDomainInputBus)
VIR_ENUM_DECL(virDomainGraphics)
VIR_ENUM_DECL(virDomainGraphicsListen)
+VIR_ENUM_DECL(virDomainGraphicsListenFamily)
VIR_ENUM_DECL(virDomainGraphicsAuthConnected)
VIR_ENUM_DECL(virDomainGraphicsSpiceChannelName)
VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
new file mode 100644
index 0000000..6cce7a8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <graphics type='vnc' port='5903' autoport='no'>
+ <listen type='network' network='Bobsnetwork'
family='ipv6'/>
+ </graphics>
+ <video>
+ <model type='cirrus' vram='16384' heads='1'/>
+ </video>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
Also create an -ipv4 specific file...
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
index bf78ca8..3b5c2de 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
@@ -25,7 +25,7 @@
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<graphics type='vnc' port='5903' autoport='no'>
- <listen type='network' network='Bobsnetwork'/>
+ <listen type='network' network='Bobsnetwork'
family='ipv4'/>
</graphics>
<video>
<model type='cirrus' vram='16384' heads='1'/>
Revert this to having no change
diff --git
a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
index abee7b6..2b2d78a 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
@@ -26,7 +26,7 @@
<input type='keyboard' bus='ps2'/>
<graphics type='vnc' port='-1' autoport='yes'
listen='1.2.3.4'>
<listen type='address' address='1.2.3.4'/>
- <listen type='network' network='Bobsnetwork'/>
+ <listen type='network' network='Bobsnetwork'
family='ipv4'/>
</graphics>
<video>
<model type='cirrus' vram='16384' heads='1'/>
Revert to having no change
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 99d4629..c966eb5 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -248,6 +248,7 @@ mymain(void)
DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE);
DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE);
DO_TEST("graphics-listen-network");
Add
DO_TEST("graphics-listen-network-ipv4");
and of course the corresponding -ipv4.xml file.
+ DO_TEST("graphics-listen-network-ipv6");
DO_TEST("graphics-vnc");
DO_TEST("graphics-vnc-websocket");
DO_TEST("graphics-vnc-sasl");
John
10:43 p.m.
New subject: [libvirt] [PATCHv2 2/4] conf: introduce new family attribute in graphics listen for chose IP family
On 03/09/2015 07:50 AM, John Ferlan wrote:
On 02/28/2015 04:08 AM, Luyao Huang wrote:
> If a interface or network have both ipv6 and ipv4 address which can be used,
> we do not know use which be a listen address. So introduce a new attribute
> to help us chose this.
>
> graphics XML will like this after this commit:
>
> <graphics type='spice' port='5900'
autoport='yes'>
> <listen type='network' address='192.168.0.1'
network='vepa-net' family='ipv4'/>
> </graphics>
>
> and this ip family can be set 2 type, and the default one is ipv4:
>
> ipv4: check if the interface or network have a can be used ipv4 address
> ipv6: check if the interface or network have a can be used ipv6 address
>
> fix some test which will be break by this commit and add a new test.
>
Adjusting the commit text slightly to match the review below
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> v2: remove family='default' and add a test.
>
> docs/formatdomain.html.in | 10 ++++++-
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 20 +++++++++++++
> src/conf/domain_conf.h | 9 ++++++
> .../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++++++++++++++++
> .../qemuxml2argv-graphics-listen-network.xml | 2 +-
> .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +-
> tests/qemuxml2xmltest.c | 1 +
> 8 files changed, 84 insertions(+), 3 deletions(-)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fb0a0d1..e8ea6fa 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null
> <graphics type='rdp' autoport='yes'
multiUser='yes' />
> <graphics type='desktop' fullscreen='yes'/>
> <graphics type='spice'>
> - <listen type='network' network='rednet'/>
> + <listen type='network' network='rednet'
family='ipv4'/>
> </graphics>
> </devices>
> ...</pre>
> @@ -4785,6 +4785,14 @@ qemu-kvm -net nic,model=? /dev/null
> the first forward dev will be used.
> </dd>
> </dl>
> + <dl>
> + <dt><code>family</code></dt>
> + <dd>if <code>type='network'</code>, the
<code>family</code>
> + attribute will contain an IP family. This tells which IP address
> + will be got for the network. IP family can be set to ipv4
> + or ipv6.
Adjusted to:
<dd>if <code>type='network'</code>, the
<code>family</code>
attribute may contain the IP family. The <code>family</code>
can be set to either <code>ipv4</code> or
<code>ipv6</code>.
This advises the graphics device which IP address family
to use as listen address for the network. The listen address
used will be the first found address of the <code>family</code>
type defined for the host.
<span class="since">Since 1.2.14</span>
Looks good to me.
> + </dd>
> + </dl>
>
> <h4><a name="elementsVideo">Video
devices</a></h4>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4e4fe9f..e84b213 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2909,6 +2909,14 @@
> <ref name="addrIPorName"/>
> </attribute>
> </optional>
> + <optional>
> + <attribute name="family">
> + <choice>
> + <value>ipv4</value>
> + <value>ipv6</value>
> + </choice>
> + </attribute>
> + </optional>
> </group>
> </choice>
> </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9b7ae3f..97f1250 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen,
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST,
> "address",
> "network")
>
> +VIR_ENUM_IMPL(virDomainGraphicsListenFamily,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST,
> + "ipv4",
> + "ipv6")
> +
Need to add a "none"... As with the previous review - adding an
attribute that's optional, but then generating it on output isn't good.
So we need a way to signify it didn't exist on input. If provided that's
fine, we can output it. If not provided, then sure we're going to
eventually default to IPv4, but that's why I said to have the boolean be
"if ipv6 desired"...
yes, thanks for pointing out, add a "none" is better than auto generate
it to ipv4.
Hmm, use family='ipv4|ipv6' in this place , another reason is : maybe
there will be a new ip family in the future (ipv7? :-P ), so use ip
family in this place will be a good choice in that day ( seems so far
away ;) )
> VIR_ENUM_IMPL(virDomainGraphicsAuthConnected,
> VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST,
> "default",
> @@ -9464,6 +9469,7 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> char *address = virXMLPropString(node, "address");
> char *network = virXMLPropString(node, "network");
> char *fromConfig = virXMLPropString(node, "fromConfig");
> + char *family = virXMLPropString(node, "family");
> int tmp;
>
> if (!type) {
> @@ -9501,6 +9507,16 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> network = NULL;
> }
>
> + if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
> + if (!family) {
> + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4;
strike this check
> + } else if ((def->family =
virDomainGraphicsListenFamilyTypeFromString(family)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown graphics listen IP family
'%s'"), family);
> + goto error;
> + }
> + }
> +
> if (fromConfig &&
> flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
> if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) {
> @@ -9520,6 +9536,7 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> VIR_FREE(address);
> VIR_FREE(network);
> VIR_FREE(fromConfig);
> + VIR_FREE(family);
> return ret;
> }
>
> @@ -19084,6 +19101,9 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf,
> if (def->network &&
> (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
> virBufferEscapeString(buf, " network='%s'",
def->network);
> +
Add a condition
if (def->family != VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE)
> + virBufferAsprintf(buf, " family='%s'",
> +
virDomainGraphicsListenFamilyTypeToString(def->family));
> }
>
> if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 02ddd93..afffe9c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1441,6 +1441,13 @@ typedef enum {
> } virDomainGraphicsListenType;
>
> typedef enum {
Add:
VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE = 0,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6,
> +
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST
> +} virDomainGraphicsListenFamily;
> +
> +typedef enum {
> VIR_DOMAIN_HUB_TYPE_USB,
>
> VIR_DOMAIN_HUB_TYPE_LAST
> @@ -1453,6 +1460,7 @@ struct _virDomainGraphicsListenDef {
> char *address;
> char *network;
> bool fromConfig; /* true if the @address is config file originated */
> + int family; /*enum virDomainGraphicsListenFamily*/
> };
>
> struct _virDomainGraphicsDef {
> @@ -2866,6 +2874,7 @@ VIR_ENUM_DECL(virDomainInput)
> VIR_ENUM_DECL(virDomainInputBus)
> VIR_ENUM_DECL(virDomainGraphics)
> VIR_ENUM_DECL(virDomainGraphicsListen)
> +VIR_ENUM_DECL(virDomainGraphicsListenFamily)
> VIR_ENUM_DECL(virDomainGraphicsAuthConnected)
> VIR_ENUM_DECL(virDomainGraphicsSpiceChannelName)
> VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
> new file mode 100644
> index 0000000..6cce7a8
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
> @@ -0,0 +1,35 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu</emulator>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'/>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0'
model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <graphics type='vnc' port='5903' autoport='no'>
> + <listen type='network' network='Bobsnetwork'
family='ipv6'/>
> + </graphics>
> + <video>
> + <model type='cirrus' vram='16384' heads='1'/>
> + </video>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>
Also create an -ipv4 specific file...
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> index bf78ca8..3b5c2de 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> @@ -25,7 +25,7 @@
> <input type='mouse' bus='ps2'/>
> <input type='keyboard' bus='ps2'/>
> <graphics type='vnc' port='5903' autoport='no'>
> - <listen type='network' network='Bobsnetwork'/>
> + <listen type='network' network='Bobsnetwork'
family='ipv4'/>
> </graphics>
> <video>
> <model type='cirrus' vram='16384' heads='1'/>
Revert this to having no change
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> index abee7b6..2b2d78a 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> @@ -26,7 +26,7 @@
> <input type='keyboard' bus='ps2'/>
> <graphics type='vnc' port='-1' autoport='yes'
listen='1.2.3.4'>
> <listen type='address' address='1.2.3.4'/>
> - <listen type='network' network='Bobsnetwork'/>
> + <listen type='network' network='Bobsnetwork'
family='ipv4'/>
> </graphics>
> <video>
> <model type='cirrus' vram='16384' heads='1'/>
Revert to having no change
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 99d4629..c966eb5 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -248,6 +248,7 @@ mymain(void)
> DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE);
> DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE);
> DO_TEST("graphics-listen-network");
Add
DO_TEST("graphics-listen-network-ipv4");
and of course the corresponding -ipv4.xml file.
Yes, need add a test for ipv4 and no need fix the existing tests if we
won't auto generate family='ipv4'.
Thanks for your review.
> + DO_TEST("graphics-listen-network-ipv6");
> DO_TEST("graphics-vnc");
> DO_TEST("graphics-vnc-websocket");
> DO_TEST("graphics-vnc-sasl");
>
John
Luyao
3:08 a.m.
New subject: [libvirt] [PATCHv2 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address
Export the required helpers and rework networkGetNetworkAddress to
make it can get IPv6 address.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
v2: rework the code and make networkGetNetworkAddress
call virNetDevGetIPAddress instead of virNetDevGetIPv4Address.
src/network/bridge_driver.c | 15 ++++++++-------
src/network/bridge_driver.h | 6 ++++--
src/qemu/qemu_command.c | 8 ++++++--
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 1209609..5aeb168 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4524,6 +4524,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
* networkGetNetworkAddress:
* @netname: the name of a network
* @netaddr: string representation of IP address for that network.
+ * @want_ipv6: if true will get IPv6 address, false for IPv4.
*
* Attempt to return an IP (v4) address associated with the named
* network. If a libvirt virtual network, that will be provided in the
@@ -4540,12 +4541,12 @@ networkReleaseActualDevice(virDomainDefPtr dom,
* completely unsupported.
*/
int
-networkGetNetworkAddress(const char *netname, char **netaddr)
+networkGetNetworkAddress(const char *netname, char **netaddr, bool want_ipv6)
{
int ret = -1;
virNetworkObjPtr network;
virNetworkDefPtr netdef;
- virNetworkIpDefPtr ipdef;
+ virNetworkIpDefPtr ipdef = NULL;
virSocketAddr addr;
virSocketAddrPtr addrptr = NULL;
char *dev_name = NULL;
@@ -4566,12 +4567,12 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
case VIR_NETWORK_FORWARD_NONE:
case VIR_NETWORK_FORWARD_NAT:
case VIR_NETWORK_FORWARD_ROUTE:
- /* if there's an ipv4def, get it's address */
- ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
+ /* if there's an ipdef, get it's IPv4 or IPv6 address */
+ ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : AF_INET, 0);
if (!ipdef) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("network '%s' doesn't have an IPv4
address"),
- netdef->name);
+ _("network '%s' doesn't have an '%s'
address"),
+ netdef->name, want_ipv6 ? "IPv6" :
"IPv4");
break;
}
addrptr = &ipdef->address;
@@ -4599,7 +4600,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
}
if (dev_name) {
- if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
+ if (virNetDevGetIPAddress(dev_name, want_ipv6, &addr) < 0)
goto error;
addrptr = &addr;
}
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 2f801ee..465ab18 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -44,7 +44,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom,
virDomainNetDefPtr iface)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-int networkGetNetworkAddress(const char *netname, char **netaddr)
+int networkGetNetworkAddress(const char *netname,
+ char **netaddr,
+ bool want_ipv6)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int networkDnsmasqConfContents(virNetworkObjPtr network,
@@ -57,7 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
# define networkAllocateActualDevice(dom, iface) 0
# define networkNotifyActualDevice(dom, iface) (dom=dom, iface=iface, 0)
# define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0)
-# define networkGetNetworkAddress(netname, netaddr) (-2)
+# define networkGetNetworkAddress(netname, netaddr, want_ipv6) (-2)
# define networkDnsmasqConfContents(network, pidfile, configstr, \
dctx, caps) 0
# endif
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b2ad9..82a4ce3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7286,7 +7286,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
if (!listenNetwork)
break;
- ret = networkGetNetworkAddress(listenNetwork, &netAddr);
+ ret = networkGetNetworkAddress(listenNetwork, &netAddr,
+ graphics->listens->family ==
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
if (ret <= -2) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("network-based listen not possible,
"
@@ -7450,7 +7452,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
if (!listenNetwork)
break;
- ret = networkGetNetworkAddress(listenNetwork, &netAddr);
+ ret = networkGetNetworkAddress(listenNetwork, &netAddr,
+ graphics->listens->family ==
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
if (ret <= -2) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("network-based listen not possible,
"
--
1.8.3.1
6:50 p.m.
New subject: [libvirt] [PATCHv2 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address
$SUBJ:
network: Allow networkGetNetworkAddress to return IPv6 address
On 02/28/2015 04:08 AM, Luyao Huang wrote:
Export the required helpers and rework networkGetNetworkAddress to
make it can get IPv6 address.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
v2: rework the code and make networkGetNetworkAddress
call virNetDevGetIPAddress instead of virNetDevGetIPv4Address.
src/network/bridge_driver.c | 15 ++++++++-------
src/network/bridge_driver.h | 6 ++++--
src/qemu/qemu_command.c | 8 ++++++--
3 files changed, 18 insertions(+), 11 deletions(-)
This one I believe had some merge conflicts with recent upstream changes..
diff --git a/src/network/bridge_driver.c
b/src/network/bridge_driver.c
index 1209609..5aeb168 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4524,6 +4524,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
* networkGetNetworkAddress:
* @netname: the name of a network
* @netaddr: string representation of IP address for that network.
+ * @want_ipv6: if true will get IPv6 address, false for IPv4.
*
* Attempt to return an IP (v4) address associated with the named
s/(v4)/(v4 or v6)
* network. If a libvirt virtual network, that will be provided in
the
@@ -4540,12 +4541,12 @@ networkReleaseActualDevice(virDomainDefPtr dom,
* completely unsupported.
*/
int
-networkGetNetworkAddress(const char *netname, char **netaddr)
+networkGetNetworkAddress(const char *netname, char **netaddr, bool want_ipv6)
{
int ret = -1;
virNetworkObjPtr network;
virNetworkDefPtr netdef;
- virNetworkIpDefPtr ipdef;
+ virNetworkIpDefPtr ipdef = NULL;
This is unecessary since virNetworkDefGetIpByIndex returns NULL if not
found.
Other than that the rest seems OK.
John
virSocketAddr addr;
virSocketAddrPtr addrptr = NULL;
char *dev_name = NULL;
@@ -4566,12 +4567,12 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
case VIR_NETWORK_FORWARD_NONE:
case VIR_NETWORK_FORWARD_NAT:
case VIR_NETWORK_FORWARD_ROUTE:
- /* if there's an ipv4def, get it's address */
- ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
+ /* if there's an ipdef, get it's IPv4 or IPv6 address */
+ ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : AF_INET, 0);
if (!ipdef) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("network '%s' doesn't have an IPv4
address"),
- netdef->name);
+ _("network '%s' doesn't have an '%s'
address"),
+ netdef->name, want_ipv6 ? "IPv6" :
"IPv4");
break;
}
addrptr = &ipdef->address;
@@ -4599,7 +4600,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
}
if (dev_name) {
- if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
+ if (virNetDevGetIPAddress(dev_name, want_ipv6, &addr) < 0)
goto error;
addrptr = &addr;
}
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 2f801ee..465ab18 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -44,7 +44,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom,
virDomainNetDefPtr iface)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-int networkGetNetworkAddress(const char *netname, char **netaddr)
+int networkGetNetworkAddress(const char *netname,
+ char **netaddr,
+ bool want_ipv6)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int networkDnsmasqConfContents(virNetworkObjPtr network,
@@ -57,7 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
# define networkAllocateActualDevice(dom, iface) 0
# define networkNotifyActualDevice(dom, iface) (dom=dom, iface=iface, 0)
# define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0)
-# define networkGetNetworkAddress(netname, netaddr) (-2)
+# define networkGetNetworkAddress(netname, netaddr, want_ipv6) (-2)
# define networkDnsmasqConfContents(network, pidfile, configstr, \
dctx, caps) 0
# endif
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b2ad9..82a4ce3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7286,7 +7286,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
if (!listenNetwork)
break;
- ret = networkGetNetworkAddress(listenNetwork, &netAddr);
+ ret = networkGetNetworkAddress(listenNetwork, &netAddr,
+ graphics->listens->family ==
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
if (ret <= -2) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("network-based listen not
possible, "
@@ -7450,7 +7452,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
if (!listenNetwork)
break;
- ret = networkGetNetworkAddress(listenNetwork, &netAddr);
+ ret = networkGetNetworkAddress(listenNetwork, &netAddr,
+ graphics->listens->family ==
+ VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
if (ret <= -2) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("network-based listen not possible,
"
9:13 p.m.
New subject: [libvirt] [PATCHv2 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address
On 03/09/2015 07:50 AM, John Ferlan wrote:
$SUBJ:
network: Allow networkGetNetworkAddress to return IPv6 address
On 02/28/2015 04:08 AM, Luyao Huang wrote:
> Export the required helpers and rework networkGetNetworkAddress to
> make it can get IPv6 address.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> v2: rework the code and make networkGetNetworkAddress
> call virNetDevGetIPAddress instead of virNetDevGetIPv4Address.
>
> src/network/bridge_driver.c | 15 ++++++++-------
> src/network/bridge_driver.h | 6 ++++--
> src/qemu/qemu_command.c | 8 ++++++--
> 3 files changed, 18 insertions(+), 11 deletions(-)
>
This one I believe had some merge conflicts with recent upstream changes..
Yes..
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 1209609..5aeb168 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4524,6 +4524,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> * networkGetNetworkAddress:
> * @netname: the name of a network
> * @netaddr: string representation of IP address for that network.
> + * @want_ipv6: if true will get IPv6 address, false for IPv4.
> *
> * Attempt to return an IP (v4) address associated with the named
s/(v4)/(v4 or v6)
> * network. If a libvirt virtual network, that will be provided in the
> @@ -4540,12 +4541,12 @@ networkReleaseActualDevice(virDomainDefPtr dom,
> * completely unsupported.
> */
> int
> -networkGetNetworkAddress(const char *netname, char **netaddr)
> +networkGetNetworkAddress(const char *netname, char **netaddr, bool want_ipv6)
> {
> int ret = -1;
> virNetworkObjPtr network;
> virNetworkDefPtr netdef;
> - virNetworkIpDefPtr ipdef;
> + virNetworkIpDefPtr ipdef = NULL;
This is unecessary since virNetworkDefGetIpByIndex returns NULL if not
found.
Other than that the rest seems OK.
Thanks for your review.
John
> virSocketAddr addr;
> virSocketAddrPtr addrptr = NULL;
> char *dev_name = NULL;
> @@ -4566,12 +4567,12 @@ networkGetNetworkAddress(const char *netname, char
**netaddr)
> case VIR_NETWORK_FORWARD_NONE:
> case VIR_NETWORK_FORWARD_NAT:
> case VIR_NETWORK_FORWARD_ROUTE:
> - /* if there's an ipv4def, get it's address */
> - ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
> + /* if there's an ipdef, get it's IPv4 or IPv6 address */
> + ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : AF_INET,
0);
> if (!ipdef) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("network '%s' doesn't have an IPv4
address"),
> - netdef->name);
> + _("network '%s' doesn't have an
'%s' address"),
> + netdef->name, want_ipv6 ? "IPv6" :
"IPv4");
> break;
> }
> addrptr = &ipdef->address;
> @@ -4599,7 +4600,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
> }
>
> if (dev_name) {
> - if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
> + if (virNetDevGetIPAddress(dev_name, want_ipv6, &addr) < 0)
> goto error;
> addrptr = &addr;
> }
> diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
> index 2f801ee..465ab18 100644
> --- a/src/network/bridge_driver.h
> +++ b/src/network/bridge_driver.h
> @@ -44,7 +44,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom,
> virDomainNetDefPtr iface)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> -int networkGetNetworkAddress(const char *netname, char **netaddr)
> +int networkGetNetworkAddress(const char *netname,
> + char **netaddr,
> + bool want_ipv6)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int networkDnsmasqConfContents(virNetworkObjPtr network,
> @@ -57,7 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
> # define networkAllocateActualDevice(dom, iface) 0
> # define networkNotifyActualDevice(dom, iface) (dom=dom, iface=iface, 0)
> # define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0)
> -# define networkGetNetworkAddress(netname, netaddr) (-2)
> +# define networkGetNetworkAddress(netname, netaddr, want_ipv6) (-2)
> # define networkDnsmasqConfContents(network, pidfile, configstr, \
> dctx, caps) 0
> # endif
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 24b2ad9..82a4ce3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7286,7 +7286,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
> listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
> if (!listenNetwork)
> break;
> - ret = networkGetNetworkAddress(listenNetwork, &netAddr);
> + ret = networkGetNetworkAddress(listenNetwork, &netAddr,
> + graphics->listens->family ==
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
> if (ret <= -2) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("network-based listen not
possible, "
> @@ -7450,7 +7452,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
> if (!listenNetwork)
> break;
> - ret = networkGetNetworkAddress(listenNetwork, &netAddr);
> + ret = networkGetNetworkAddress(listenNetwork, &netAddr,
> + graphics->listens->family ==
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
> if (ret <= -2) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> "%s", _("network-based listen not
possible, "
>
Luyao
3:08 a.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: fix a error coverd issue in 2 place
we already set a more clearly error in networkGetNetworkAddress,
so this error will cover our error in networkGetNetworkAddress.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/qemu/qemu_command.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82a4ce3..cd0758d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7295,12 +7295,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
"network driver not present"));
goto error;
}
- if (ret < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("listen network '%s' had no usable
address"),
- listenNetwork);
+ if (ret < 0)
goto error;
- }
+
listenAddr = netAddr;
/* store the address we found in the <graphics> element so it will
* show up in status. */
@@ -7461,12 +7458,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
"network driver not present"));
goto error;
}
- if (ret < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("listen network '%s' had no usable
address"),
- listenNetwork);
+ if (ret < 0)
goto error;
- }
+
listenAddr = netAddr;
/* store the address we found in the <graphics> element so it will
* show up in status. */
--
1.8.3.1
6:51 p.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: fix a error coverd issue in 2 place
On 02/28/2015 04:08 AM, Luyao Huang wrote:
we already set a more clearly error in networkGetNetworkAddress,
so this error will cover our error in networkGetNetworkAddress.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/qemu/qemu_command.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
Change comment to:
Remove unnecessary virReportError on networkGetNetworkAddress return
More specific errors are set in the called functions when -1 is
returned, so don't overwrite those messages.
Code seems fine and I traced functions as well.
John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 82a4ce3..cd0758d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7295,12 +7295,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
"network driver not present"));
goto error;
}
- if (ret < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("listen network '%s' had no usable
address"),
- listenNetwork);
+ if (ret < 0)
goto error;
- }
+
listenAddr = netAddr;
/* store the address we found in the <graphics> element so it will
* show up in status. */
@@ -7461,12 +7458,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
"network driver not present"));
goto error;
}
- if (ret < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("listen network '%s' had no usable
address"),
- listenNetwork);
+ if (ret < 0)
goto error;
- }
+
listenAddr = netAddr;
/* store the address we found in the <graphics> element so it will
* show up in status. */
8:53 p.m.
New subject: [libvirt] [PATCHv2 4/4] qemu: fix a error coverd issue in 2 place
On 03/09/2015 07:51 AM, John Ferlan wrote:
On 02/28/2015 04:08 AM, Luyao Huang wrote:
> we already set a more clearly error in networkGetNetworkAddress,
> so this error will cover our error in networkGetNetworkAddress.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
Change comment to:
Remove unnecessary virReportError on networkGetNetworkAddress return
More specific errors are set in the called functions when -1 is
returned, so don't overwrite those messages.
Code seems fine and I traced functions as well.
Thanks for your review and help.
New comment looks good for me :)
John
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 82a4ce3..cd0758d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7295,12 +7295,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
> "network driver not present"));
> goto error;
> }
> - if (ret < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("listen network '%s' had no usable
address"),
> - listenNetwork);
> + if (ret < 0)
> goto error;
> - }
> +
> listenAddr = netAddr;
> /* store the address we found in the <graphics> element so it
will
> * show up in status. */
> @@ -7461,12 +7458,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> "network driver not present"));
> goto error;
> }
> - if (ret < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("listen network '%s' had no usable
address"),
> - listenNetwork);
> + if (ret < 0)
> goto error;
> - }
> +
> listenAddr = netAddr;
> /* store the address we found in the <graphics> element so it will
> * show up in status. */
>
Luyao
3544
days inactive
3555
days old
19 comments
3 participants
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang