[libvirt] [PATCH 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address
John Ferlan
jferlan at redhat.com
Tue Feb 24 18:22:03 UTC 2015
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 at 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
More information about the libvir-list
mailing list