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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list