[libvirt] [PATCH 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address
lhuang
lhuang at redhat.com
Wed Feb 25 10:08:59 UTC 2015
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 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
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
More information about the libvir-list
mailing list