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

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(a)redhat.com&gt; > --- > 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 > &lt;graphics type='rdp' autoport='yes' multiUser='yes' /&gt; > &lt;graphics type='desktop' fullscreen='yes'/&gt; > &lt;graphics type='spice'&gt; > - &lt;listen type='network' network='rednet'/&gt; > + &lt;listen type='network' network='rednet' family='default'/&gt; This changes to ipv6='yes' > &lt;/graphics&gt; > &lt;/devices&gt; > ...</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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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

we already set a more clearly error in networkGetNetworkAddress, so this error will cover our error in networkGetNetworkAddress. Signed-off-by: Luyao Huang <lhuang(a)redhat.com&gt; --- 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. */