Re: [libvirt] [PATCH 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address

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(a)redhat.com&gt; > --- > 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