[libvirt] [PATCH 0/4] add support for graphics to get a IPv6 address
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 a new helper 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 | 9 +++ src/conf/domain_conf.c | 21 +++++++ src/conf/domain_conf.h | 10 ++++ src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 2 + src/network/bridge_driver.c | 50 +++++++++++----- src/network/bridge_driver.h | 6 +- src/qemu/qemu_command.c | 18 ++---- src/util/virnetdev.c | 70 ++++++++++++++++++++++ src/util/virnetdev.h | 2 + .../qemuxml2argv-graphics-listen-network.xml | 2 +- .../qemuxml2argv-graphics-listen-network2.xml | 2 +- .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +- 15 files changed, 174 insertions(+), 33 deletions(-) -- 1.8.3.1
Introduce a new function to help to get interface IPv6 address. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 73 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 645aef1..f60911c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1659,6 +1659,7 @@ virNetDevDelMulti; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetIPv6Address; virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43..c1a588e 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,72 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED, #endif /* ! SIOCGIFADDR */ +/** + *virNetDevGetIPv6Address: + *@ifname: name of the interface whose IP address we want + *@addr: filled with the IPv6 address + * + *This function gets the IPv6 address for the interface @ifname + *and stores it in @addr + * + *Returns 0 on success, -1 on failure. + */ +#if defined(HAVE_GETIFADDRS) +int virNetDevGetIPv6Address(const char *ifname, + virSocketAddrPtr addr) +{ + struct ifaddrs *ifap, *ifa; + int ret = -1; + bool found_one = false; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list")); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (STRNEQ(ifa->ifa_name, ifname)) + continue; + found_one = true; + if (ifa->ifa_addr->sa_family == AF_INET6) + break; + } + + if (!ifa) { + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' do not have a IPv6 address"), + ifname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + goto cleanup; + } + + addr->data.stor.ss_family = AF_INET6; + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); + ret = 0; + + cleanup: + freeifaddrs(ifap); + return ret; +} + +#else + +int virNetDevGetIPv6Address(const char *ifname ATTRIBUTE_UNUSED, + virSocketAddrPtr addr ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get IPv6 address on this platform")); + return -1; +} + +#endif + /** * virNetDevValidateConfig: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index de8b480..c065381 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -104,6 +104,8 @@ 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 virNetDevGetIPv6Address(const char *ifname, virSocketAddrPtr addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevSetMAC(const char *ifname, -- 1.8.3.1
On 02/13/2015 02:17 AM, Luyao Huang wrote:
Introduce a new function to help to get interface IPv6 address.
s/Introduce a new function to help to/Introduce virNetDevGetIPv6Address/
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 73 insertions(+)
Hmm... maybe rather than introducing a new IPv6 specific routine and forcing the caller to handle the logic of knowing how/whether to return an IPv4 or IPv6 address... Why not change the existing GetIPv4Address into a "shim" virNetDevGetIPAddress which then makes the decisions regarding returning only one family or allowing a failed fetch of IPv4 to use the IPv6 routine... This way you hide the details. Your first patch would just change the IPv4 into an GetIPAddress API and that would just call the now local/static IPv4 API. You won't have a #if #else #endif for the new API - it would return 0 or -1. Check out how "safezero" has multiple ways in order to zero out a file based on what's present. I suspect your new API could follow that methodology. In the long run since getifaddrs() can return an IPv4 or IPv6 address it could be theoretically called first. If it returns nothing, fall back to the IPv4 API Your new API could be something like: 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; } The virNetDevGetIfaddrsAddress would follow safezero_posix_fallocate returning -2 in the #else of #if defined(HAVE_GETIFADDRS). The logic in the function would return the first found address of IPv6 if that's desired or IPv4 otherwise. The virNetDevGetIPv4Address() wouldn't need the two stolen lines to clear addr, but would otherwise function as it does today. Hopefully this makes sense - you'll be adding more patches, but I think in the long run based on the following patches it will make it "easier" on the caller to just get "an" address and force it to be IPv6 only if that's what's desired.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 645aef1..f60911c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1659,6 +1659,7 @@ virNetDevDelMulti; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetIPv6Address; virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43..c1a588e 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,72 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFADDR */
+/** + *virNetDevGetIPv6Address: + *@ifname: name of the interface whose IP address we want
s/IP/IPv6
+ *@addr: filled with the IPv6 address + * + *This function gets the IPv6 address for the interface @ifname + *and stores it in @addr + * + *Returns 0 on success, -1 on failure. + */
Each of the lines above needs s/*/* /
+#if defined(HAVE_GETIFADDRS) +int virNetDevGetIPv6Address(const char *ifname, + virSocketAddrPtr addr) +{ + struct ifaddrs *ifap, *ifa; + int ret = -1; + bool found_one = false; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list"));
s/list$/list for '%s'/ and of course an ifname parameter ...
+ return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (STRNEQ(ifa->ifa_name, ifname)) + continue; + found_one = true; + if (ifa->ifa_addr->sa_family == AF_INET6) + break;
From the getifaddrs(3) man page:
"The ifa_addr field points to a structure containing the interface address. (The sa_family subfield should be consulted to determine the format of the address structure.) This field may contain a null pointer." So you need to check that here before de-referencing a NULL pointer Also I'm assuming these API's don't care how usable the address is, just that it's the first one found. See 'checkProtocols()' for what I mean about usable - there's an additional getaddrinfo() call there that handles a special IPv6 case (I forget all the details, but the ::1 has some special characteristics...).
+ } + + if (!ifa) { + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' do not have a IPv6 address"),
s/do/does/
+ ifname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + goto cleanup; + } + + addr->data.stor.ss_family = AF_INET6; + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len);
I'd also move this chunk inside that loop above replacing the "break;" with this code plus of course the following ret = 0; and a goto cleanup. Allowing then the fall of the end of the loop code to just check if (found_one) or not (eg, no need for "if (!ifa)"... Leaving the following - including the capability to get either IPv6 or IPv4 address depending upon what's desired/required: for (ifa = ifap; ifa; ifa = ifa->ifa_next) { if (!ifa->ifa_addr || STRNEQ(ifa->ifa_name, ifname)) continue; found_one = true; if (want_ipv6 && ifa->ifa_addr->sa_family != AF_INET6) continue; /* Found an address to return */ addr->data.stor.ss_family = ifa->ifa_addr->sa_family; if (ifa->ifa_addr->sa_family == AF_INET6) 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); } ret = 0; goto cleanup; } if (found_one) virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' does not have an %s address"), ifname, want_ipv6 ? "IPv6" : "IPv4"); else virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' not found"), ifname); cleanup:
+ ret = 0; + + cleanup: + freeifaddrs(ifap); + return ret; +} + +#else + +int virNetDevGetIPv6Address(const char *ifname ATTRIBUTE_UNUSED, + virSocketAddrPtr addr ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get IPv6 address on this platform")); + return -1; +} + +#endif +
/** * virNetDevValidateConfig: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index de8b480..c065381 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -104,6 +104,8 @@ 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 virNetDevGetIPv6Address(const char *ifname, virSocketAddrPtr addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
int virNetDevSetMAC(const char *ifname,
On 02/24/2015 11:12 AM, John Ferlan wrote: <...snip...> As I'm reading the next patch I'm thinking I forgot something in the previous one... The "default" has been IPv4 and would seem to be the "right" thing to return once we find an address; however, if we wanted an IPv6 address and returned an IPv4 one, that wouldn't be good...
Leaving the following - including the capability to get either IPv6 or IPv4 address depending upon what's desired/required:
for (ifa = ifap; ifa; ifa = ifa->ifa_next) { if (!ifa->ifa_addr || STRNEQ(ifa->ifa_name, ifname)) continue; found_one = true; if (want_ipv6 && ifa->ifa_addr->sa_family != AF_INET6) continue;
/* Found an address to return */ addr->data.stor.ss_family = ifa->ifa_addr->sa_family; if (ifa->ifa_addr->sa_family == AF_INET6) addr->len = sizeof(addr->data.inet6); memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); } else {
} else if (!want_ipv6 && ifa->ifa_addr->sa_family == AF_INET) {
addr->len = sizeof(addr->data.inet4); memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len); } ret = 0; goto cleanup; }
On 02/25/2015 01:42 AM, John Ferlan wrote:
On 02/24/2015 11:12 AM, John Ferlan wrote:
<...snip...>
As I'm reading the next patch I'm thinking I forgot something in the previous one...
The "default" has been IPv4 and would seem to be the "right" thing to return once we find an address; however, if we wanted an IPv6 address and returned an IPv4 one, that wouldn't be good...
Thanks your review and help and i have changed some place for this part, please see the email i replied before(just now).
Leaving the following - including the capability to get either IPv6 or IPv4 address depending upon what's desired/required:
for (ifa = ifap; ifa; ifa = ifa->ifa_next) { if (!ifa->ifa_addr || STRNEQ(ifa->ifa_name, ifname)) continue; found_one = true; if (want_ipv6 && ifa->ifa_addr->sa_family != AF_INET6) continue;
/* Found an address to return */ addr->data.stor.ss_family = ifa->ifa_addr->sa_family; if (ifa->ifa_addr->sa_family == AF_INET6) addr->len = sizeof(addr->data.inet6); memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); } else { } else if (!want_ipv6 && ifa->ifa_addr->sa_family == AF_INET) {
addr->len = sizeof(addr->data.inet4); memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len); } ret = 0; goto cleanup; }
Luyao
On 02/25/2015 12:12 AM, John Ferlan wrote:
On 02/13/2015 02:17 AM, Luyao Huang wrote:
Introduce a new function to help to get interface IPv6 address.
s/Introduce a new function to help to/Introduce virNetDevGetIPv6Address/
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 2 ++ 3 files changed, 73 insertions(+)
Hmm... maybe rather than introducing a new IPv6 specific routine and forcing the caller to handle the logic of knowing how/whether to return an IPv4 or IPv6 address...
Why not change the existing GetIPv4Address into a "shim" virNetDevGetIPAddress which then makes the decisions regarding returning only one family or allowing a failed fetch of IPv4 to use the IPv6 routine...
This way you hide the details. Your first patch would just change the IPv4 into an GetIPAddress API and that would just call the now local/static IPv4 API. You won't have a #if #else #endif for the new API - it would return 0 or -1.
Check out how "safezero" has multiple ways in order to zero out a file based on what's present. I suspect your new API could follow that methodology.
In the long run since getifaddrs() can return an IPv4 or IPv6 address it could be theoretically called first. If it returns nothing, fall back to the IPv4 API
Your new API could be something like:
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; }
The virNetDevGetIfaddrsAddress would follow safezero_posix_fallocate returning -2 in the #else of #if defined(HAVE_GETIFADDRS). The logic in the function would return the first found address of IPv6 if that's desired or IPv4 otherwise.
Good idea! I just thought add a new functions for ipv6 address but forgot getifaddrs() can also get the IPv4 address :) Thanks for pointing out and i will rework this patch in next version.
The virNetDevGetIPv4Address() wouldn't need the two stolen lines to clear addr, but would otherwise function as it does today.
Hopefully this makes sense - you'll be adding more patches, but I think in the long run based on the following patches it will make it "easier" on the caller to just get "an" address and force it to be IPv6 only if that's what's desired.
Yes, i had thought about this before, maybe we can add a new function to get(or chose) the IPv6 address we desired, because one interface can have many IPv6 address and sometimes we just need one of them.
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 645aef1..f60911c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1659,6 +1659,7 @@ virNetDevDelMulti; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetIPv6Address; virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43..c1a588e 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,72 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFADDR */
+/** + *virNetDevGetIPv6Address: + *@ifname: name of the interface whose IP address we want s/IP/IPv6
thanks, but seems this function will be removed (or renamed) in next version :)
+ *@addr: filled with the IPv6 address + * + *This function gets the IPv6 address for the interface @ifname + *and stores it in @addr + * + *Returns 0 on success, -1 on failure. + */ Each of the lines above needs s/*/* /
Sorry for my careless.
+#if defined(HAVE_GETIFADDRS) +int virNetDevGetIPv6Address(const char *ifname, + virSocketAddrPtr addr) +{ + struct ifaddrs *ifap, *ifa; + int ret = -1; + bool found_one = false; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list")); s/list$/list for '%s'/ and of course an ifname parameter ...
Hmm, seems it is not the interface issue when getifaddrs cannot get interface list, however it will give a more nice error. Thanks your advise and i will change it in next version.
+ return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (STRNEQ(ifa->ifa_name, ifname)) + continue; + found_one = true; + if (ifa->ifa_addr->sa_family == AF_INET6) + break; From the getifaddrs(3) man page:
"The ifa_addr field points to a structure containing the interface address. (The sa_family subfield should be consulted to determine the format of the address structure.) This field may contain a null pointer."
So you need to check that here before de-referencing a NULL pointer
Oh, thanks for pointing out this.
Also I'm assuming these API's don't care how usable the address is, just that it's the first one found. See 'checkProtocols()' for what I mean about usable - there's an additional getaddrinfo() call there that handles a special IPv6 case (I forget all the details, but the ::1 has some special characteristics...).
Thanks for your remind, i checked checkProtocols() and i guess your mean we can use getaddrinfo() to make sure the address we get can be used for a socket bind ? And i also thought about another issue after your reminding: An interface can have more than one IPv6 address. But i still couldn't find a good way until now to chose which IPv6 address if we find more than one IPv6 address in one interface (maybe users should use "listen type=address" instead of "listen type=network" in this case ;)). There are some special addresses anddefault address selection for IPv6 address, i don't know if it is a good idea to guess which address is the caller really desired if we get some IPv6 address in one interface.
+ } + + if (!ifa) { + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' do not have a IPv6 address"), s/do/does/
+ ifname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + goto cleanup; + } + + addr->data.stor.ss_family = AF_INET6; + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len);
I'd also move this chunk inside that loop above replacing the "break;" with this code plus of course the following ret = 0; and a goto cleanup.
Allowing then the fall of the end of the loop code to just check if (found_one) or not (eg, no need for "if (!ifa)"...
Leaving the following - including the capability to get either IPv6 or IPv4 address depending upon what's desired/required:
for (ifa = ifap; ifa; ifa = ifa->ifa_next) { if (!ifa->ifa_addr || STRNEQ(ifa->ifa_name, ifname)) continue; found_one = true; if (want_ipv6 && ifa->ifa_addr->sa_family != AF_INET6) continue;
/* Found an address to return */ addr->data.stor.ss_family = ifa->ifa_addr->sa_family; if (ifa->ifa_addr->sa_family == AF_INET6) 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); } ret = 0; goto cleanup; }
if (found_one) virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' does not have an %s address"), ifname, want_ipv6 ? "IPv6" : "IPv4"); else virReportError(VIR_ERR_INTERNAL_ERROR, _("Interface '%s' not found"), ifname);
cleanup:
Thanks for your code and i have changed some place, and now : + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (!ifa->ifa_addr || + STRNEQ(ifa->ifa_name, ifname)) { + continue; + } + found_one = true; + if (want_ipv6 && ifa->ifa_addr->sa_family == AF_INET6) { + addr->len = sizeof(addr->data.inet6); + memcpy(&addr->data.inet6, ifa->ifa_addr, addr->len); + } else if (!want_ipv6 && ifa->ifa_addr->sa_family == AF_INET) { + addr->len = sizeof(addr->data.inet4); + memcpy(&addr->data.inet4, ifa->ifa_addr, addr->len); + } else { + continue; + } + addr->data.stor.ss_family = ifa->ifa_addr->sa_family; + ret = 0; + goto cleanup; + } + + if (found_one) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' does not have a %s address"), + ifname, want_ipv6 ? "IPv6" : "IPv4"); + else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + + cleanup: Luyao
On 02/25/2015 04:50 AM, lhuang wrote:
And i also thought about another issue after your reminding: An interface can have more than one IPv6 address. But i still couldn't find a good way until now to chose which IPv6 address if we find more than one IPv6 address in one interface (maybe users should use "listen type=address" instead of "listen type=network" in this case ;)).
There are some special addresses anddefault address selection for IPv6 address, i don't know if it is a good idea to guess which address is the caller really desired if we get some IPv6 address in one interface.
If they are using listen type='network', then either they will have taken care to have the network only have a single IP, or they won't care. Otherwise, as you say, they should use listen type='address'. BTW, some history for listen type='network' - this feature was added to allow a way to remove the explicit listen address from the domain config, thus allowing the domain to be migrated from one host to another, as long as there is a network with the same name on both hosts. Often the network used for this is created only for this purpose. If the physical interface named in the network has multiple IP addresses, there really isn't a good way for libvirt to indicate preference for any one of the addresses over another (you can't specify it with an index, because the index of the desired IP address may be different between the hosts. Because it's impossible to do it right, I say we shouldn't even try; it would be worse for it to work halfway.
On 02/25/2015 11:55 PM, Laine Stump wrote:
On 02/25/2015 04:50 AM, lhuang wrote:
And i also thought about another issue after your reminding: An interface can have more than one IPv6 address. But i still couldn't find a good way until now to chose which IPv6 address if we find more than one IPv6 address in one interface (maybe users should use "listen type=address" instead of "listen type=network" in this case ;)).
There are some special addresses anddefault address selection for IPv6 address, i don't know if it is a good idea to guess which address is the caller really desired if we get some IPv6 address in one interface.
If they are using listen type='network', then either they will have taken care to have the network only have a single IP, or they won't care. Otherwise, as you say, they should use listen type='address'.
Yes, this is what i thought at the first time, thanks for your help.
BTW, some history for listen type='network' - this feature was added to allow a way to remove the explicit listen address from the domain config, thus allowing the domain to be migrated from one host to another, as long as there is a network with the same name on both hosts. Often the network used for this is created only for this purpose. If the physical interface named in the network has multiple IP addresses, there really isn't a good way for libvirt to indicate preference for any one of the addresses over another (you can't specify it with an index, because the index of the desired IP address may be different between the hosts. Because it's impossible to do it right, I say we shouldn't even try; it would be worse for it to work halfway.
Okay, got it, so maybe report error if host physical interface has multiple IP addresses will be a good choice in this scene, but i found a host physical interface will have 2 Ipv6 address, one is 2001...or 2002... for public and another fe80... address for internal. just like this: inet6 2001:db8:ca2:2::1/64 scope global valid_lft forever preferred_lft forever inet6 fe80::5054:ff:fe7f:f047/64 scope link valid_lft forever preferred_lft forever i think maybe we can use 2001... ipv6 address instead of output error in this scene ? Luyao
On 02/25/2015 11:55 PM, Laine Stump wrote:
On 02/25/2015 04:50 AM, lhuang wrote:
And i also thought about another issue after your reminding: An interface can have more than one IPv6 address. But i still couldn't find a good way until now to chose which IPv6 address if we find more than one IPv6 address in one interface (maybe users should use "listen type=address" instead of "listen type=network" in this case ;)).
There are some special addresses anddefault address selection for IPv6 address, i don't know if it is a good idea to guess which address is the caller really desired if we get some IPv6 address in one interface.
If they are using listen type='network', then either they will have taken care to have the network only have a single IP, or they won't care. Otherwise, as you say, they should use listen type='address'.
BTW, some history for listen type='network' - this feature was added to allow a way to remove the explicit listen address from the domain config, thus allowing the domain to be migrated from one host to another, as long as there is a network with the same name on both hosts. Often the network used for this is created only for this purpose. If the physical interface named in the network has multiple IP addresses, there really isn't a good way for libvirt to indicate preference for any one of the addresses over another (you can't specify it with an index, because the index of the desired IP address may be different between the hosts. Because it's impossible to do it right, I say we shouldn't even try; it would be worse for it to work halfway.
After i read some doc and self check , i agree with your opinion: output error instead of chose one of them. i found it is not a good idea for libvirt to chose use which IP address if the hosts interface have multiple IP address (both ipv6 and ipv4), because if the caller won't want this spice open to public, he will want to use fe80 address in this scene. However if the caller want to use a address for public, he will want to use 2XXX address. Thanks a lot for your help.
Luyao
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='default'/> </graphics> and this ip family can be set 3 type: default: check if the interface or network have a can be used ipv4 address, if yes use this address, if not will try to get a ipv6 address. 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. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- docs/formatdomain.html.in | 10 +++++++++- docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 21 +++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++++ .../qemuxml2argv-graphics-listen-network.xml | 2 +- .../qemuxml2argv-graphics-listen-network2.xml | 2 +- .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +- 7 files changed, 52 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fcf5984..7a07ca0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4512,7 +4512,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='default'/> </graphics> </devices> ...</pre> @@ -4752,6 +4752,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 default, 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 7a1d299..fb8d084 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2888,6 +2888,15 @@ <ref name="addrIPorName"/> </attribute> </optional> + <optional> + <attribute name="family"> + <choice> + <value>default</value> + <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 fd36063..307801d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -522,6 +522,12 @@ VIR_ENUM_IMPL(virDomainGraphicsListen, VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST, "address", "network") +VIR_ENUM_IMPL(virDomainGraphicsListenFamily, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST, + "default", + "ipv4", + "ipv6") + VIR_ENUM_IMPL(virDomainGraphicsAuthConnected, VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST, "default", @@ -9396,6 +9402,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) { @@ -9433,6 +9440,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, network = NULL; } + if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) { + if (!family) { + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT; + } 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) { @@ -9452,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, VIR_FREE(address); VIR_FREE(network); VIR_FREE(fromConfig); + VIR_FREE(family); return ret; } @@ -19044,6 +19062,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 da21bce..7806bc6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1438,6 +1438,14 @@ typedef enum { } virDomainGraphicsListenType; typedef enum { + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT = 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 @@ -1450,6 +1458,7 @@ struct _virDomainGraphicsListenDef { char *address; char *network; bool fromConfig; /* true if the @address is config file originated */ + int family; /*enum virDomainGraphicsListenFamily*/ }; struct _virDomainGraphicsDef { @@ -2862,6 +2871,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.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml index bf78ca8..1962474 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml index 62353e9..d11eead 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml @@ -25,7 +25,7 @@ <input type='keyboard' bus='ps2'/> <graphics type='vnc' listen='1.2.3.4' autoport='yes'> <listen type='address' address='1.2.3.4'/> - <listen type='network' network='Bobsnetwork'/> + <listen type='network' network='Bobsnetwork' family='default'/> </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..e69c29a 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> -- 1.8.3.1
On 02/13/2015 02:17 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='default'/> </graphics>
Generally don't want to see 'default' in there - I would say if not provided, then on output we don't print default, especially since if someone tried to port this XML to a version of libvirt that doesn't support the family attribute, then you've got other issues.
and this ip family can be set 3 type:
default: check if the interface or network have a can be used ipv4 address, if yes use this address, if not will try to get a ipv6 address.
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 this is why you don't add "family='default'"... I see family as a "tristate" value. That is not provided is 0 and not printed... Thus, the value turns into tristate where of ipv6='yes|no', where the default is no. The problem you have is that existing configurations don't have the value *and* the documentation states it'll return the first IPv4 address found - which means by default (eg, when ipv6 is not provided or if it's set to 'no'), we have to return an ipv4 address.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- docs/formatdomain.html.in | 10 +++++++++- docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 21 +++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++++ .../qemuxml2argv-graphics-listen-network.xml | 2 +- .../qemuxml2argv-graphics-listen-network2.xml | 2 +- .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +- 7 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fcf5984..7a07ca0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4512,7 +4512,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='default'/>
This changes to ipv6='yes'
</graphics> </devices> ...</pre> @@ -4752,6 +4752,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 default, ipv4 + or ipv6. + </dd> + </dl>
This then describes that if the attribute "ipv6='yes'" is provided, then rather than searching for the first IPv4 address, the code will search for the first IPv6 address. If there is no IPv6 address, then failure will occur and no fall back to IPv4 happens.
<h4><a name="elementsVideo">Video devices</a></h4> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a1d299..fb8d084 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2888,6 +2888,15 @@ <ref name="addrIPorName"/> </attribute> </optional> + <optional> + <attribute name="family"> + <choice> + <value>default</value> + <value>ipv4</value> + <value>ipv6</value> + </choice> + </attribute> + </optional>
This becomes: <attribute name='ipv6'> <ref name="virYesNo"/> </attribute>
</group> </choice> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd36063..307801d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -522,6 +522,12 @@ VIR_ENUM_IMPL(virDomainGraphicsListen, VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST, "address", "network")
+VIR_ENUM_IMPL(virDomainGraphicsListenFamily, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST, + "default", + "ipv4", + "ipv6") +
won't be necessary...
VIR_ENUM_IMPL(virDomainGraphicsAuthConnected, VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST, "default", @@ -9396,6 +9402,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, char *address = virXMLPropString(node, "address"); char *network = virXMLPropString(node, "network"); char *fromConfig = virXMLPropString(node, "fromConfig"); + char *family = virXMLPropString(node, "family");
Changes to char *ipv6 = virXMLPropString(node, "ipv6");
int tmp;
if (!type) { @@ -9433,6 +9440,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, network = NULL; }
+ if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) { + if (!family) { + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT; + } else if ((def->family = virDomainGraphicsListenFamilyTypeFromString(family)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown graphics listen IP family '%s'"), family); + goto error; + }
All this changes to: if (rawio) { if ((def->ipv6 = virTristateBoolTypeFromString(ipv6)) <= 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown hostdev ipv6 setting '%s'"), ipv6); goto error; }
+ } + if (fromConfig && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) { @@ -9452,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, VIR_FREE(address); VIR_FREE(network); VIR_FREE(fromConfig); + VIR_FREE(family);
Changes to VIR_FREE(ipv6);
return ret; }
@@ -19044,6 +19062,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));
This becomes if (def->ipv6) virBufferAsprintf(buf, " ipv6='%s'", virTristateBoolTypeToString(def->ipv6));
}
if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index da21bce..7806bc6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1438,6 +1438,14 @@ typedef enum { } virDomainGraphicsListenType;
typedef enum { + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT = 0, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6, + + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST +} virDomainGraphicsListenFamily;
Unnecessary
+ +typedef enum { VIR_DOMAIN_HUB_TYPE_USB,
VIR_DOMAIN_HUB_TYPE_LAST @@ -1450,6 +1458,7 @@ struct _virDomainGraphicsListenDef { char *address; char *network; bool fromConfig; /* true if the @address is config file originated */ + int family; /*enum virDomainGraphicsListenFamily*/
This becomes int ipv6; /* enum virTristateBool */
};
struct _virDomainGraphicsDef { @@ -2862,6 +2871,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.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml index bf78ca8..1962474 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml index 62353e9..d11eead 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml @@ -25,7 +25,7 @@ <input type='keyboard' bus='ps2'/> <graphics type='vnc' listen='1.2.3.4' autoport='yes'> <listen type='address' address='1.2.3.4'/> - <listen type='network' network='Bobsnetwork'/> + <listen type='network' network='Bobsnetwork' family='default'/> </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..e69c29a 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/>
None of these are necessary... BUT, you should add one for IPv6... That is - you need to add an "ipv6=yes" and an "ipv6=no" test case in order to make sure both are processed properly. John
On 02/24/2015 12:42 PM, John Ferlan 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='default'/> </graphics> Generally don't want to see 'default' in there - I would say if not
On 02/13/2015 02:17 AM, Luyao Huang wrote: provided, then on output we don't print default, especially since if someone tried to port this XML to a version of libvirt that doesn't support the family attribute, then you've got other issues.
and this ip family can be set 3 type:
default: check if the interface or network have a can be used ipv4 address, if yes use this address, if not will try to get a ipv6 address.
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 this is why you don't add "family='default'"...
I see family as a "tristate" value. That is not provided is 0 and not printed... Thus, the value turns into tristate where of ipv6='yes|no', where the default is no.
There is precedent in libvirt config for a "family" which can be set to ipv6 or ipv4 or not be set at all (e.g. see the <ip> and <route> elements in a libvirt network). I agree that "default" shouldn't be an allowed setting, but I think we can/should stick with family for the attribute rather than ipv6='(yes|no)'
The problem you have is that existing configurations don't have the value *and* the documentation states it'll return the first IPv4 address found - which means by default (eg, when ipv6 is not provided or if it's set to 'no'), we have to return an ipv4 address.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- docs/formatdomain.html.in | 10 +++++++++- docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 21 +++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++++++ .../qemuxml2argv-graphics-listen-network.xml | 2 +- .../qemuxml2argv-graphics-listen-network2.xml | 2 +- .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +- 7 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fcf5984..7a07ca0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4512,7 +4512,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='default'/> This changes to ipv6='yes'
</graphics> </devices> ...</pre> @@ -4752,6 +4752,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 default, ipv4 + or ipv6. + </dd> + </dl>
This then describes that if the attribute "ipv6='yes'" is provided, then rather than searching for the first IPv4 address, the code will search for the first IPv6 address. If there is no IPv6 address, then failure will occur and no fall back to IPv4 happens.
<h4><a name="elementsVideo">Video devices</a></h4> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a1d299..fb8d084 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2888,6 +2888,15 @@ <ref name="addrIPorName"/> </attribute> </optional> + <optional> + <attribute name="family"> + <choice> + <value>default</value> + <value>ipv4</value> + <value>ipv6</value> + </choice> + </attribute> + </optional>
This becomes:
<attribute name='ipv6'> <ref name="virYesNo"/> </attribute>
</group> </choice> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd36063..307801d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -522,6 +522,12 @@ VIR_ENUM_IMPL(virDomainGraphicsListen, VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST, "address", "network")
+VIR_ENUM_IMPL(virDomainGraphicsListenFamily, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST, + "default", + "ipv4", + "ipv6") +
won't be necessary...
VIR_ENUM_IMPL(virDomainGraphicsAuthConnected, VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST, "default", @@ -9396,6 +9402,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, char *address = virXMLPropString(node, "address"); char *network = virXMLPropString(node, "network"); char *fromConfig = virXMLPropString(node, "fromConfig"); + char *family = virXMLPropString(node, "family"); Changes to
char *ipv6 = virXMLPropString(node, "ipv6");
int tmp;
if (!type) { @@ -9433,6 +9440,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, network = NULL; }
+ if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) { + if (!family) { + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT; + } else if ((def->family = virDomainGraphicsListenFamilyTypeFromString(family)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown graphics listen IP family '%s'"), family); + goto error; + }
All this changes to:
if (rawio) { if ((def->ipv6 = virTristateBoolTypeFromString(ipv6)) <= 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown hostdev ipv6 setting '%s'"), ipv6); goto error; }
+ } + if (fromConfig && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) { @@ -9452,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, VIR_FREE(address); VIR_FREE(network); VIR_FREE(fromConfig); + VIR_FREE(family); Changes to VIR_FREE(ipv6);
return ret; }
@@ -19044,6 +19062,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));
This becomes if (def->ipv6) virBufferAsprintf(buf, " ipv6='%s'", virTristateBoolTypeToString(def->ipv6));
}
if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index da21bce..7806bc6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1438,6 +1438,14 @@ typedef enum { } virDomainGraphicsListenType;
typedef enum { + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT = 0, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6, + + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST +} virDomainGraphicsListenFamily;
Unnecessary
+ +typedef enum { VIR_DOMAIN_HUB_TYPE_USB,
VIR_DOMAIN_HUB_TYPE_LAST @@ -1450,6 +1458,7 @@ struct _virDomainGraphicsListenDef { char *address; char *network; bool fromConfig; /* true if the @address is config file originated */ + int family; /*enum virDomainGraphicsListenFamily*/ This becomes
int ipv6; /* enum virTristateBool */
};
struct _virDomainGraphicsDef { @@ -2862,6 +2871,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.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml index bf78ca8..1962474 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml index 62353e9..d11eead 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network2.xml @@ -25,7 +25,7 @@ <input type='keyboard' bus='ps2'/> <graphics type='vnc' listen='1.2.3.4' autoport='yes'> <listen type='address' address='1.2.3.4'/> - <listen type='network' network='Bobsnetwork'/> + <listen type='network' network='Bobsnetwork' family='default'/> </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..e69c29a 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='default'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/>
None of these are necessary... BUT, you should add one for IPv6...
That is - you need to add an "ipv6=yes" and an "ipv6=no" test case in order to make sure both are processed properly.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/24/2015 01:09 PM, Laine Stump wrote:
On 02/24/2015 12:42 PM, John Ferlan wrote:
<...snip...>
I see family as a "tristate" value. That is not provided is 0 and not printed... Thus, the value turns into tristate where of ipv6='yes|no', where the default is no.
There is precedent in libvirt config for a "family" which can be set to ipv6 or ipv4 or not be set at all (e.g. see the <ip> and <route> elements in a libvirt network). I agree that "default" shouldn't be an allowed setting, but I think we can/should stick with family for the attribute rather than ipv6='(yes|no)'
OK - fair enough. Going the family=ipv4|ipv6 is fine - perhaps a mix and match of what I've looked at and responded to so far. If IPv4 is provided, then we require to find an IPv4 address, same for IPv6... If the attribute is not provided, then we have to assume return of IPv4 address because the point I raised in patch 3/4 where the caller should be the one to decide whether it can accept/process a returned IPv6 address in the (unlikely?) scenario that an IPv4 address wasn't found. Since the default today is to attempt to return an IPv4 address, we're almost forced to ensure that the caller/user lets us know they want and can handle either address style. The problem I see with generating "family='ipv4'" or "family=" on output if not provided on input is that we end up with test case regressions and the possibility of issues with someone trying to "move" their XML to some other system that doesn't yet understand the new attribute. Dealing with new attributes is always a bit tricky... John <...snip...>
On 02/24/2015 01:48 PM, John Ferlan wrote:
On 02/24/2015 01:09 PM, Laine Stump wrote:
On 02/24/2015 12:42 PM, John Ferlan wrote: <...snip...>
I see family as a "tristate" value. That is not provided is 0 and not printed... Thus, the value turns into tristate where of ipv6='yes|no', where the default is no. There is precedent in libvirt config for a "family" which can be set to ipv6 or ipv4 or not be set at all (e.g. see the <ip> and <route> elements in a libvirt network). I agree that "default" shouldn't be an allowed setting, but I think we can/should stick with family for the attribute rather than ipv6='(yes|no)'
OK - fair enough. Going the family=ipv4|ipv6 is fine - perhaps a mix and match of what I've looked at and responded to so far. If IPv4 is provided, then we require to find an IPv4 address, same for IPv6... If the attribute is not provided, then we have to assume return of IPv4 address because the point I raised in patch 3/4 where the caller should be the one to decide whether it can accept/process a returned IPv6 address in the (unlikely?) scenario that an IPv4 address wasn't found.
Yes, I agree with this. In the other uses of "family", when it isn't specified it is assumed to be ipv4, not "either" (mostly for historical reasons, but it makes sense).
Since the default today is to attempt to return an IPv4 address, we're almost forced to ensure that the caller/user lets us know they want and can handle either address style.
The problem I see with generating "family='ipv4'" or "family=" on output if not provided on input is that we end up with test case regressions and the possibility of issues with someone trying to "move" their XML to some other system that doesn't yet understand the new attribute.
Definitely.
Dealing with new attributes is always a bit tricky...
On 02/25/2015 03:03 AM, Laine Stump wrote:
On 02/24/2015 01:48 PM, John Ferlan wrote:
On 02/24/2015 01:09 PM, Laine Stump wrote:
On 02/24/2015 12:42 PM, John Ferlan wrote: <...snip...>
I see family as a "tristate" value. That is not provided is 0 and not printed... Thus, the value turns into tristate where of ipv6='yes|no', where the default is no. There is precedent in libvirt config for a "family" which can be set to ipv6 or ipv4 or not be set at all (e.g. see the <ip> and <route> elements in a libvirt network). I agree that "default" shouldn't be an allowed setting, but I think we can/should stick with family for the attribute rather than ipv6='(yes|no)'
OK - fair enough. Going the family=ipv4|ipv6 is fine - perhaps a mix and match of what I've looked at and responded to so far. If IPv4 is provided, then we require to find an IPv4 address, same for IPv6... If the attribute is not provided, then we have to assume return of IPv4 address because the point I raised in patch 3/4 where the caller should be the one to decide whether it can accept/process a returned IPv6 address in the (unlikely?) scenario that an IPv4 address wasn't found. Yes, I agree with this. In the other uses of "family", when it isn't specified it is assumed to be ipv4, not "either" (mostly for historical reasons, but it makes sense).
Okay, thanks a lot for your help i will use family=ipv4|ipv6
Since the default today is to attempt to return an IPv4 address, we're almost forced to ensure that the caller/user lets us know they want and can handle either address style.
The problem I see with generating "family='ipv4'" or "family=" on output if not provided on input is that we end up with test case regressions and the possibility of issues with someone trying to "move" their XML to some other system that doesn't yet understand the new attribute. Definitely.
Dealing with new attributes is always a bit tricky...
Luyao
Export the required helpers and rework networkGetNetworkAddress to make it can get IPv6 address. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 50 ++++++++++++++++++++++++++++++++------------- src/network/bridge_driver.h | 6 ++++-- src/qemu/qemu_command.c | 4 ++-- 6 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f947d89..9eb640b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -148,7 +148,7 @@ virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def) VIR_FREE(def->name); } -static void +void virNetworkIpDefClear(virNetworkIpDefPtr def) { VIR_FREE(def->family); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 484522e..0c4b559 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -310,6 +310,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); void virNetworkObjListFree(virNetworkObjListPtr vms); +void virNetworkIpDefClear(virNetworkIpDefPtr def); typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f60911c..ff942d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -559,6 +559,7 @@ virNetworkDeleteConfig; virNetworkFindByName; virNetworkFindByUUID; virNetworkForwardTypeToString; +virNetworkIpDefClear; virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkLoadAllConfigs; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2798010..d1afd16 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4487,6 +4487,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, * networkGetNetworkAddress: * @netname: the name of a network * @netaddr: string representation of IP address for that network. + * @family: IP family * * Attempt to return an IP (v4) address associated with the named * network. If a libvirt virtual network, that will be provided in the @@ -4503,12 +4504,13 @@ networkReleaseActualDevice(virDomainDefPtr dom, * completely unsupported. */ int -networkGetNetworkAddress(const char *netname, char **netaddr) +networkGetNetworkAddress(const char *netname, char **netaddr, int family) { int ret = -1; virNetworkObjPtr network; virNetworkDefPtr netdef; - virNetworkIpDefPtr ipdef; + virNetworkIpDefPtr ipdef = NULL; + virNetworkIpDefPtr ipdef6 = NULL; virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; @@ -4529,15 +4531,9 @@ 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 */ + /* if there's an ipv4def or ipv6def, get it's address */ ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); - if (!ipdef) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' doesn't have an IPv4 address"), - netdef->name); - break; - } - addrptr = &ipdef->address; + ipdef6 = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0); break; case VIR_NETWORK_FORWARD_BRIDGE: @@ -4561,10 +4557,32 @@ networkGetNetworkAddress(const char *netname, char **netaddr) break; } - if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) - goto error; - addrptr = &addr; + switch ((virDomainGraphicsListenFamily) family) { + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT: + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4: + if (ipdef) + addrptr = &ipdef->address; + if (dev_name) { + if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + /*if the family is default and we do not get a ipv4 address + *in this place, we will try to get a ipv6 address + */ + if (addrptr || family == VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4) + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6: + if (ipdef6) + addrptr = &ipdef6->address; + if (dev_name) { + if (virNetDevGetIPv6Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST: + break; } if (!(addrptr && @@ -4579,6 +4597,10 @@ networkGetNetworkAddress(const char *netname, char **netaddr) return ret; error: + if (ipdef6) + virNetworkIpDefClear(ipdef6); + if (ipdef) + virNetworkIpDefClear(ipdef); goto cleanup; } diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 2f801ee..c684c15 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, + int family) 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, family) (-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 9c25788..6bd8f69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7277,7 +7277,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, " @@ -7441,7 +7441,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, " -- 1.8.3.1
On 02/13/2015 02:17 AM, Luyao Huang wrote:
Export the required helpers and rework networkGetNetworkAddress to make it can get IPv6 address.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 50 ++++++++++++++++++++++++++++++++------------- src/network/bridge_driver.h | 6 ++++-- src/qemu/qemu_command.c | 4 ++-- 6 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f947d89..9eb640b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -148,7 +148,7 @@ virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def) VIR_FREE(def->name); }
-static void +void
I believe this is unnecessary
virNetworkIpDefClear(virNetworkIpDefPtr def) { VIR_FREE(def->family); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 484522e..0c4b559 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -310,6 +310,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); void virNetworkObjListFree(virNetworkObjListPtr vms); +void virNetworkIpDefClear(virNetworkIpDefPtr def);
Same unnecessary
typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f60911c..ff942d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -559,6 +559,7 @@ virNetworkDeleteConfig; virNetworkFindByName; virNetworkFindByUUID; virNetworkForwardTypeToString; +virNetworkIpDefClear;
Unnecessary
virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkLoadAllConfigs; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2798010..d1afd16 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4487,6 +4487,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, * networkGetNetworkAddress: * @netname: the name of a network * @netaddr: string representation of IP address for that network. + * @family: IP family
@want_ipv6: boolean to force return of IPv6 address for that network
* * Attempt to return an IP (v4) address associated with the named * network. If a libvirt virtual network, that will be provided in the @@ -4503,12 +4504,13 @@ networkReleaseActualDevice(virDomainDefPtr dom, * completely unsupported. */ int -networkGetNetworkAddress(const char *netname, char **netaddr) +networkGetNetworkAddress(const char *netname, char **netaddr, int family)
s/int family/bool want_ipv6/
{ int ret = -1; virNetworkObjPtr network; virNetworkDefPtr netdef; - virNetworkIpDefPtr ipdef; + virNetworkIpDefPtr ipdef = NULL; + virNetworkIpDefPtr ipdef6 = NULL;
The ipdef6 is unnecessary
virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; @@ -4529,15 +4531,9 @@ 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 */ + /* if there's an ipv4def or ipv6def, get it's address */ ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); - if (!ipdef) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' doesn't have an IPv4 address"), - netdef->name); - break; - } - addrptr = &ipdef->address; + ipdef6 = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);
Not sure I follow why we're losing the error reporting here... I'd change this to: if (want_ipv6) ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0); else ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' doesn't have an '%s' address"), netdef->name, want_ipv6 ? "IPv6" : "IPv4"); break; } addrptr = &ipdef->address; NB: virNetworkDefGetIpByIndex() doesn't return a COPY that we must FREE, it returns a pointer to something owned elsewhere
break;
case VIR_NETWORK_FORWARD_BRIDGE: @@ -4561,10 +4557,32 @@ networkGetNetworkAddress(const char *netname, char **netaddr) break; }
- if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) - goto error; - addrptr = &addr; + switch ((virDomainGraphicsListenFamily) family) { + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT: + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4: + if (ipdef) + addrptr = &ipdef->address; + if (dev_name) { + if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + /*if the family is default and we do not get a ipv4 address + *in this place, we will try to get a ipv6 address + */ + if (addrptr || family == VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4) + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6: + if (ipdef6) + addrptr = &ipdef6->address; + if (dev_name) { + if (virNetDevGetIPv6Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST: + break;
This code would then just be: if (dev_name) { if (virNetDevGetIPAddress(dev_name, &addr, want_ipv6) < 0) goto error; addrptr = &addr; } While I can appreciate the logic to "default to" returning the IPv6 address if no IPv4 address was found, I think those details can/should be left up to the caller to decide whether returning an IPv6 address is acceptable. Falling through to a try to find/return an IPv6 address while perhaps being "kind" could result in some caller getting something it didn't expect and even worse, didn't know how to parse!
}
if (!(addrptr && @@ -4579,6 +4597,10 @@ networkGetNetworkAddress(const char *netname, char **netaddr) return ret;
error: + if (ipdef6) + virNetworkIpDefClear(ipdef6); + if (ipdef) + virNetworkIpDefClear(ipdef);
Since virNetworkDefGetIpByIndex is not creating a copy, but rather is returning the "&def->ips[i];" value, I don't believe we want to do any sort of clear here as it then becomes unusable for no reason.
goto cleanup; }
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 2f801ee..c684c15 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, + int family) 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, family) (-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 9c25788..6bd8f69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7277,7 +7277,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, " @@ -7441,7 +7441,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, "
These two turn into a boolean 3rd parameter: (graphics->listens->ipv6 == VIR_TRISTATE_BOOL_YES) and should be on a separate line to avoid the longer than 80 characters in a line... John
On 02/25/2015 02:22 AM, John Ferlan wrote:
On 02/13/2015 02:17 AM, Luyao Huang wrote:
Export the required helpers and rework networkGetNetworkAddress to make it can get IPv6 address.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 50 ++++++++++++++++++++++++++++++++------------- src/network/bridge_driver.h | 6 ++++-- src/qemu/qemu_command.c | 4 ++-- 6 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f947d89..9eb640b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -148,7 +148,7 @@ virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def) VIR_FREE(def->name); }
-static void +void I believe this is unnecessary
virNetworkIpDefClear(virNetworkIpDefPtr def) { VIR_FREE(def->family); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 484522e..0c4b559 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -310,6 +310,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); void virNetworkObjListFree(virNetworkObjListPtr vms); +void virNetworkIpDefClear(virNetworkIpDefPtr def); Same unnecessary
typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f60911c..ff942d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -559,6 +559,7 @@ virNetworkDeleteConfig; virNetworkFindByName; virNetworkFindByUUID; virNetworkForwardTypeToString; +virNetworkIpDefClear; Unnecessary
Yes, thanks for pointing out these place, i forgot check the code in virNetworkDefGetIpByIndex.
virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkLoadAllConfigs; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2798010..d1afd16 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4487,6 +4487,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, * networkGetNetworkAddress: * @netname: the name of a network * @netaddr: string representation of IP address for that network. + * @family: IP family @want_ipv6: boolean to force return of IPv6 address for that network
* * Attempt to return an IP (v4) address associated with the named * network. If a libvirt virtual network, that will be provided in the @@ -4503,12 +4504,13 @@ networkReleaseActualDevice(virDomainDefPtr dom, * completely unsupported. */ int -networkGetNetworkAddress(const char *netname, char **netaddr) +networkGetNetworkAddress(const char *netname, char **netaddr, int family) s/int family/bool want_ipv6/
{ int ret = -1; virNetworkObjPtr network; virNetworkDefPtr netdef; - virNetworkIpDefPtr ipdef; + virNetworkIpDefPtr ipdef = NULL; + virNetworkIpDefPtr ipdef6 = NULL; The ipdef6 is unnecessary
virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; @@ -4529,15 +4531,9 @@ 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 */ + /* if there's an ipv4def or ipv6def, get it's address */ ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); - if (!ipdef) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' doesn't have an IPv4 address"), - netdef->name); - break; - } - addrptr = &ipdef->address; + ipdef6 = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);
Not sure I follow why we're losing the error reporting here...
I'd change this to:
if (want_ipv6) ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0); else ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' doesn't have an '%s' address"), netdef->name, want_ipv6 ? "IPv6" : "IPv4"); break; } addrptr = &ipdef->address;
NB: virNetworkDefGetIpByIndex() doesn't return a COPY that we must FREE, it returns a pointer to something owned elsewhere
Thanks for your advise and i will changed this in next version: ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : AF_INET, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' doesn't have an '%s' address"), netdef->name, want_ipv6 ? "IPv6" : "IPv4"); break; } addrptr = &ipdef->address;
break;
case VIR_NETWORK_FORWARD_BRIDGE: @@ -4561,10 +4557,32 @@ networkGetNetworkAddress(const char *netname, char **netaddr) break; }
- if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) - goto error; - addrptr = &addr; + switch ((virDomainGraphicsListenFamily) family) { + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT: + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4: + if (ipdef) + addrptr = &ipdef->address; + if (dev_name) { + if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + /*if the family is default and we do not get a ipv4 address + *in this place, we will try to get a ipv6 address + */ + if (addrptr || family == VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4) + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6: + if (ipdef6) + addrptr = &ipdef6->address; + if (dev_name) { + if (virNetDevGetIPv6Address(dev_name, &addr) < 0) + goto error; + addrptr = &addr; + } + break; + case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST: + break;
This code would then just be:
if (dev_name) { if (virNetDevGetIPAddress(dev_name, &addr, want_ipv6) < 0) goto error; addrptr = &addr; }
While I can appreciate the logic to "default to" returning the IPv6 address if no IPv4 address was found, I think those details can/should be left up to the caller to decide whether returning an IPv6 address is acceptable. Falling through to a try to find/return an IPv6 address while perhaps being "kind" could result in some caller getting something it didn't expect and even worse, didn't know how to parse!
Thanks for your pointing out.
}
if (!(addrptr && @@ -4579,6 +4597,10 @@ networkGetNetworkAddress(const char *netname, char **netaddr) return ret;
error: + if (ipdef6) + virNetworkIpDefClear(ipdef6); + if (ipdef) + virNetworkIpDefClear(ipdef);
Since virNetworkDefGetIpByIndex is not creating a copy, but rather is returning the "&def->ips[i];" value, I don't believe we want to do any sort of clear here as it then becomes unusable for no reason.
goto cleanup; }
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 2f801ee..c684c15 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, + int family) 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, family) (-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 9c25788..6bd8f69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7277,7 +7277,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, " @@ -7441,7 +7441,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, "
These two turn into a boolean 3rd parameter:
(graphics->listens->ipv6 == VIR_TRISTATE_BOOL_YES)
and should be on a separate line to avoid the longer than 80 characters in a line...
Hmm, after i saw the discussion around patch 2/4, i do some small change about this part: + ret = networkGetNetworkAddress(listenNetwork, &netAddr, + graphics->listens->family == + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6);
John
Luyao
we already set a more clearly error in networkGetNetworkAddress, so this error will cover our error in networkGetNetworkAddress. Signed-off-by: Luyao Huang <lhuang@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 6bd8f69..b42e0f4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7284,12 +7284,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. */ @@ -7448,12 +7445,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
On 02/13/2015 02:17 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@redhat.com> --- src/qemu/qemu_command.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
I didn't check every exit patch from networkGetNetworkAddress, but perhaps these should change to : if (ret < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_XML_ERROR,...); goto error; } Just to be sure we don't leave without setting some sort of error. John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6bd8f69..b42e0f4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7284,12 +7284,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. */ @@ -7448,12 +7445,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. */
On 02/25/2015 02:52 AM, John Ferlan wrote:
On 02/13/2015 02:17 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@redhat.com> --- src/qemu/qemu_command.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
I didn't check every exit patch from networkGetNetworkAddress, but perhaps these should change to :
if (ret < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_XML_ERROR,...); goto error; }
Just to be sure we don't leave without setting some sort of error.
Okay, thanks for your advise, i will change them in next version.
John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6bd8f69..b42e0f4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7284,12 +7284,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. */ @@ -7448,12 +7445,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
On Tue, Feb 24, 2015 at 01:52:08PM -0500, John Ferlan wrote:
On 02/13/2015 02:17 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@redhat.com> --- src/qemu/qemu_command.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
I didn't check every exit patch from networkGetNetworkAddress, but perhaps these should change to :
if (ret < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_XML_ERROR,...); goto error; }
Just to be sure we don't leave without setting some sort of error.
No. All the exit paths should be checked to make sure an error was reported in all cases where we return -1. Jan
On 02/25/2015 10:51 PM, Ján Tomko wrote:
On Tue, Feb 24, 2015 at 01:52:08PM -0500, John Ferlan wrote:
On 02/13/2015 02:17 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@redhat.com> --- src/qemu/qemu_command.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
I didn't check every exit patch from networkGetNetworkAddress, but perhaps these should change to :
if (ret < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_XML_ERROR,...); goto error; }
Just to be sure we don't leave without setting some sort of error.
No.
All the exit paths should be checked to make sure an error was reported in all cases where we return -1.
Okay, i have checked the code and think we will return -1 with an error setting. So i will keep the code like this: if (ret < 0) goto error; Thanks for your review
Jan
Luyao
participants (5)
-
John Ferlan -
Ján Tomko -
Laine Stump -
lhuang -
Luyao Huang