On 03/09/2015 07:50 AM, John Ferlan wrote:

On 02/28/2015 04:08 AM, Luyao Huang wrote:

...

Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6 and IPv4 address.

Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress and virNetDevGetIPv4Address to get IP address.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: add a new function virNetDevGetIPAddress to call virNetDevGetIfaddrsAddress , and make virNetDevGetIfaddrsAddress can get ipv4 address for a interface. Also add a error output in virNetDevGetIfaddrsAddress when get multiple ip address for a host interface.

src/libvirt_private.syms | 1 + src/util/virnetdev.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 4 ++ 3 files changed, 103 insertions(+)

Closer... But still missing a couple of things, which I can add for you. I'll make my comments and do the changes locally but not commit until Mon afternoon (Eastern US) so if Laine wants to comment he can and of course that you agree with my comments...

Thanks in advance for your help

Going to split this commit into two -

The first commit will just make the virNetDevIPAddress shim, moving the virNetDevIPv4Address to a static function.

The second commit will add the IPv6 option to virNetDevIPAddress

No problem

...

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ba05cc6..f88626a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1670,6 +1670,7 @@ virNetDevClearIPAddress; virNetDevDelMulti; virNetDevExists; virNetDevGetIndex; +virNetDevGetIPAddress; virNetDevGetIPv4Address; We can remove the GetIPv4Address and make it static to virtnetdev.c

Yes

...

virNetDevGetLinkInfo; virNetDevGetMAC; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2a0eb43..318c3b6 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,100 @@ int virNetDevGetIPv4Address(const char *ifname ATTRIBUTE_UNUSED,

#endif /* ! SIOCGIFADDR */

+/** + * virNetDevGetIfaddrsAddress: + * @ifname: name of the interface whose IP address we want + * @want_ipv6: get IPv4 or IPv6 address + * @addr: filled with the IP address + * + * This function gets the IP address for the interface @ifname + * and stores it in @addr + * + * Returns 0 on success, -1 on failure, -2 on unsupported. + */ +#if defined(HAVE_GETIFADDRS) +static int virNetDevGetIfaddrsAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) Although not a rule - we try to make newer API's as:

static int fname(param1, param2)

oh, i should noticed this

...

+{ + struct ifaddrs *ifap, *ifa; + int ret = -1; + int nIPaddr = 0; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, + _("Could not get interface list for '%s'"), + ifname); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (!ifa->ifa_addr || + STRNEQ(ifa->ifa_name, ifname)) { + continue; + } STRNEQ_NULLABLE does the trick...

...

+ if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + if (++nIPaddr > 1) + break; [1]... hmm not sure about this...

...

+ if (want_ipv6) { + 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); + } + addr->data.stor.ss_family = ifa->ifa_addr->sa_family; + } + } + + if (nIPaddr == 1) + ret = 0; + else if (nIPaddr > 1) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' has multiple %s address"), s/address/addresses

...

+ ifname, want_ipv6 ? "IPv6" : "IPv4"); [1] But is this really desired... It seems from the previous review, if someone wants a specific address they use "<listen type='address'...".

Otherwise, they use this function. Since you'll be returning either an IPv4 or IPv6 address here you'd be causing an error here if the network had more than one IPv4 address defined; whereas, the previous code just returned the "first" from the ioctl(SIOCGIFADDR...).

I think once you have an address you just return the first one found and document it that way avoiding this path completely. You could also note that if you want a specific address use the type='address'

Hmm, yes, I agree it is not a good idea to report error when we get multiple addresses in this function (In some case, caller just want one ip address and not care about which one we chose), so remove the check and error output here is reasonable (maybe we can use a DEBUG or WARNING here complain about this and set ret to 0). However this check and error output is a result from last review, i think maybe we can move them to a right place (should in another patch). Because we use listen type='network' for migration in the most case, and if a host interface has multiple address than we always chose the first one, this will make things worse in some case. An example: In host A, we have a happy vm which use listen type='network' and use a spice client to connect to it (listen address is "fe80::7ae3:b5ff:fec5:189e%enp1s0" and address is get from an interface via virNetDevGetIfaddrsAddress() ), than we migrate this vm to a another host B, we found a network have the same name and use a host interface in host B, we use virNetDevGetIfaddrsAddress() get the first ipv6 address and we get "2001:db8:ca2:2::1/64", unfortunately this address is not a "good" address (the good one is the second one :-P ) and spice connection will be broken, and the user will say : "why libvirt chose a wrong address and broke my connection :-(". NB: the comment from Laine in this mail: https://www.redhat.com/archives/libvir-list/2015-February/msg01080.html

...

+ else + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + + freeifaddrs(ifap); + return ret; +} + +#else + +static int virNetDevGetIfaddrsAddress(const char *ifname ATTRIBUTE_UNUSED, + bool want_ipv6, + virSocketAddrPtr addr ATTRIBUTE_UNUSED) ditto on function definition

...

+{ + virReportSystemError(ENOSYS, + _("Unable to get %s address on this platform"), + want_ipv6 ? "IPv6" : "IPv4"); While this seems appropriate, because you potentially call virNetDevGetIPv4Address below that means it's possible we return with this error message set, but a good status or we overwrite some other message... So I'll move it...

Yes, i forgot remove error message when wrote v2, thanks for pointing out.

...

+ return -2; +} + +#endif + + +int virNetDevGetIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) same

...

+{ + 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); Here if we have virNetDevGetIPv4Address() return -2, then we can take our message above and place it right here while also adjusting the "! SIOCGIFADDR" to just return -2.

...

+ + return -1; +} NOTE: This does not return -2 in any

should be return -2 (and this only happen when want_ipv6 is true and the ret is -2)

...

/** * virNetDevValidateConfig: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index de8b480..faf3b2f 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -104,6 +104,10 @@ int virNetDevClearIPAddress(const char *ifname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevGetIPv4Address(const char *ifname, virSocketAddrPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;

Removing the IPv4Address API

...

+int virNetDevGetIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;

int virNetDevSetMAC(const char *ifname,

These changes require modifying src/network/bridge_driver.c (temporarily until we add the IPv6 aware code later):

Yes, i should move this modifying from another patch to this patch.

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2a61991..7d6e173 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4608,7 +4608,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) }

if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + if (virNetDevGetIPAddress(dev_name, false, &addr) < 0) goto cleanup; addrptr = &addr; }

John Thanks for your review.

Luyao

On 03/09/2015 11:37 AM, Luyao Huang wrote:

On 03/09/2015 07:50 AM, John Ferlan wrote:

...

On 02/28/2015 04:08 AM, Luyao Huang wrote:

...

Introduce virNetDevGetIfaddrsAddress to help to get interface IPv6 and IPv4 address.

Introduce virNetDevGetIPAddress to use virNetDevGetIfaddrsAddress and virNetDevGetIPv4Address to get IP address.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: add a new function virNetDevGetIPAddress to call virNetDevGetIfaddrsAddress , and make virNetDevGetIfaddrsAddress can get ipv4 address for a interface. Also add a error output in virNetDevGetIfaddrsAddress when get multiple ip address for a host interface.

src/libvirt_private.syms | 1 + src/util/virnetdev.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 4 ++ 3 files changed, 103 insertions(+)

...

...

+ if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + if (++nIPaddr > 1) + break; [1]... hmm not sure about this...

...

+ if (want_ipv6) { + 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); + } + addr->data.stor.ss_family = ifa->ifa_addr->sa_family; + } + } + + if (nIPaddr == 1) + ret = 0; + else if (nIPaddr > 1) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' has multiple %s address"), s/address/addresses

...

+ ifname, want_ipv6 ? "IPv6" : "IPv4"); [1] But is this really desired... It seems from the previous review, if someone wants a specific address they use "<listen type='address'...".

Otherwise, they use this function. Since you'll be returning either an IPv4 or IPv6 address here you'd be causing an error here if the network had more than one IPv4 address defined; whereas, the previous code just returned the "first" from the ioctl(SIOCGIFADDR...).

I think once you have an address you just return the first one found and document it that way avoiding this path completely. You could also note that if you want a specific address use the type='address'

I had an idea but my eyes almost close ;) so i write here without test them.

I think it will be better if we make this function to get mutli address and return the number of address we get, like this:

+static int +virNetDevGetIfaddrsAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr *addrs)

We'd need to have an naddrs to know how many we can stuff in... or you'd need to do the *REALLOC on the fly in this module for each address found. Interesting idea, but you'd be just throwing things away. I could perhaps having some code "periodically" cache addresses... or cache addresses, subscribe to some event to let you know when a new address is generated so you can add it to your list (if there is one)... But, how do you use it? John

+{ + struct ifaddrs *ifap, *ifa; + int nIPaddr = 0; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, + _("Could not get interface list for '%s'"), + ifname); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + virSocketAddrPtr tmpaddr; + + if (!ifa->ifa_addr || + STRNEQ_NULLABLE(ifa->ifa_name, ifname)) { + continue; + } + + if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + memset(tmpaddr, 0, sizeof(*tmpaddr)); + + if (!ifa->ifa_addr || + STRNEQ_NULLABLE(ifa->ifa_name, ifname)) { + continue; + } + + if (ifa->ifa_addr->sa_family == (want_ipv6 ? AF_INET6 : AF_INET)) { + memset(tmpaddr, 0, sizeof(*tmpaddr)); + + if (want_ipv6) { + tmpaddr->len = sizeof(tmpaddr->data.inet6); + memcpy(&tmpaddr->data.inet6, ifa->ifa_addr, tmpaddr->len); + } else { + tmpaddr->len = sizeof(tmpaddr->data.inet4); + memcpy(&tmpaddr->data.inet4, ifa->ifa_addr, tmpaddr->len); + } + tmpaddr->data.stor.ss_family = ifa->ifa_addr->sa_family; + addrs[nIPaddr++] = tmpaddr; + } + } + if (nIPaddr == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Interface '%s' not found"), + ifname); + return -1; + } + freeifaddrs(ifap); + return nIPaddr; +}

...

...

+int virNetDevGetIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) and then rename this function to virNetDevGetOneIPAddress()

and rework the code like this:

+int virNetDevGetOneIPAddress(const char *ifname, + bool want_ipv6, + virSocketAddrPtr addr) +{ + int ret; + virSocketAddrPtr *addrs = NULL; + + ret = virNetDevGetIfaddrsAddress(ifname, want_ipv6, addrs); + if (ret > 0) { + addr = addrs[0]; + } else if (ret == -2 && !want_ipv6) { + return virNetDevGetIPv4Address(ifname, addr); + } + + return ret; +}

...

...

These changes require modifying src/network/bridge_driver.c (temporarily until we add the IPv6 aware code later):

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2a61991..7d6e173 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4608,7 +4608,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) }

if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + if (virNetDevGetIPAddress(dev_name, false, &addr) < 0) goto cleanup; addrptr = &addr; }

At last, add the multiple address check to here and this place will like this:

+ int num = virNetDevGetOneIPAddress(dev_name, want_ipv6, &addr); + if (num > 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("interface '%s' has multiple %s addresses"), + dev_name, want_ipv6 ? "IPv6" : "IPv4"); + goto cleanup; + } else if (num < 0) { goto cleanup; + }

because i am not test them so there will be some mistake, I will test them tomorrow, hope it will work :)

...

John

Luyao

On 03/09/2015 07:50 AM, John Ferlan wrote:

On 02/28/2015 04:08 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='ipv4'/> </graphics>

and this ip family can be set 2 type, and the default one is ipv4:

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 add a new test.

Adjusting the commit text slightly to match the review below

...

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: remove family='default' and add a test.

docs/formatdomain.html.in | 10 ++++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 20 +++++++++++++ src/conf/domain_conf.h | 9 ++++++ .../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++++++++++++++++ .../qemuxml2argv-graphics-listen-network.xml | 2 +- .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +- tests/qemuxml2xmltest.c | 1 + 8 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fb0a0d1..e8ea6fa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null <graphics type='rdp' autoport='yes' multiUser='yes' /> <graphics type='desktop' fullscreen='yes'/> <graphics type='spice'> - <listen type='network' network='rednet'/> + <listen type='network' network='rednet' family='ipv4'/> </graphics> </devices> ...</pre> @@ -4785,6 +4785,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 ipv4 + or ipv6. Adjusted to:

<dd>if <code>type='network'</code>, the <code>family</code> attribute may contain the IP family. The <code>family</code> can be set to either <code>ipv4</code> or <code>ipv6</code>. This advises the graphics device which IP address family to use as listen address for the network. The listen address used will be the first found address of the <code>family</code> type defined for the host. <span class="since">Since 1.2.14</span>

Looks good to me.

...

+ </dd> + </dl>

<h4><a name="elementsVideo">Video devices</a></h4> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4e4fe9f..e84b213 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2909,6 +2909,14 @@ <ref name="addrIPorName"/> </attribute> </optional> + <optional> + <attribute name="family"> + <choice> + <value>ipv4</value> + <value>ipv6</value> + </choice> + </attribute> + </optional> </group> </choice> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b7ae3f..97f1250 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen, VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST, "address", "network")

+VIR_ENUM_IMPL(virDomainGraphicsListenFamily, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST, + "ipv4", + "ipv6") + Need to add a "none"... As with the previous review - adding an attribute that's optional, but then generating it on output isn't good. So we need a way to signify it didn't exist on input. If provided that's fine, we can output it. If not provided, then sure we're going to eventually default to IPv4, but that's why I said to have the boolean be "if ipv6 desired"...

yes, thanks for pointing out, add a "none" is better than auto generate it to ipv4. Hmm, use family='ipv4|ipv6' in this place , another reason is : maybe there will be a new ip family in the future (ipv7? :-P ), so use ip family in this place will be a good choice in that day ( seems so far away ;) )

...

VIR_ENUM_IMPL(virDomainGraphicsAuthConnected, VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST, "default", @@ -9464,6 +9469,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, char *address = virXMLPropString(node, "address"); char *network = virXMLPropString(node, "network"); char *fromConfig = virXMLPropString(node, "fromConfig"); + char *family = virXMLPropString(node, "family"); int tmp;

if (!type) { @@ -9501,6 +9507,16 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, network = NULL; }

+ if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) { + if (!family) { + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4; strike this check

...

+ } else if ((def->family = virDomainGraphicsListenFamilyTypeFromString(family)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown graphics listen IP family '%s'"), family); + goto error; + } + } + if (fromConfig && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) { @@ -9520,6 +9536,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, VIR_FREE(address); VIR_FREE(network); VIR_FREE(fromConfig); + VIR_FREE(family); return ret; }

@@ -19084,6 +19101,9 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf, if (def->network && (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) { virBufferEscapeString(buf, " network='%s'", def->network); + Add a condition if (def->family != VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE)

...

+ virBufferAsprintf(buf, " family='%s'", + virDomainGraphicsListenFamilyTypeToString(def->family)); }

if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 02ddd93..afffe9c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1441,6 +1441,13 @@ typedef enum { } virDomainGraphicsListenType;

typedef enum { Add: VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE = 0, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4, + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6, + + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST +} virDomainGraphicsListenFamily; + +typedef enum { VIR_DOMAIN_HUB_TYPE_USB,

VIR_DOMAIN_HUB_TYPE_LAST @@ -1453,6 +1460,7 @@ struct _virDomainGraphicsListenDef { char *address; char *network; bool fromConfig; /* true if the @address is config file originated */ + int family; /*enum virDomainGraphicsListenFamily*/ };

struct _virDomainGraphicsDef { @@ -2866,6 +2874,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-ipv6.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml new file mode 100644 index 0000000..6cce7a8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no'> + <listen type='network' network='Bobsnetwork' family='ipv6'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='virtio'/> + </devices> +</domain> Also create an -ipv4 specific file...

...

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml index bf78ca8..3b5c2de 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='ipv4'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> Revert this to having no change

...

diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml index abee7b6..2b2d78a 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='ipv4'/> </graphics> <video> <model type='cirrus' vram='16384' heads='1'/> Revert to having no change

...

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 99d4629..c966eb5 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -248,6 +248,7 @@ mymain(void) DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE); DO_TEST("graphics-listen-network"); Add DO_TEST("graphics-listen-network-ipv4");

and of course the corresponding -ipv4.xml file.

Yes, need add a test for ipv4 and no need fix the existing tests if we won't auto generate family='ipv4'. Thanks for your review.

...

+ DO_TEST("graphics-listen-network-ipv6"); DO_TEST("graphics-vnc"); DO_TEST("graphics-vnc-websocket"); DO_TEST("graphics-vnc-sasl");

John

Luyao

$SUBJ: network: Allow networkGetNetworkAddress to return IPv6 address On 02/28/2015 04:08 AM, Luyao Huang wrote:

Export the required helpers and rework networkGetNetworkAddress to make it can get IPv6 address.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: rework the code and make networkGetNetworkAddress call virNetDevGetIPAddress instead of virNetDevGetIPv4Address.

src/network/bridge_driver.c | 15 ++++++++------- src/network/bridge_driver.h | 6 ++++-- src/qemu/qemu_command.c | 8 ++++++-- 3 files changed, 18 insertions(+), 11 deletions(-)

This one I believe had some merge conflicts with recent upstream changes..

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1209609..5aeb168 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4524,6 +4524,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, * networkGetNetworkAddress: * @netname: the name of a network * @netaddr: string representation of IP address for that network. + * @want_ipv6: if true will get IPv6 address, false for IPv4. * * Attempt to return an IP (v4) address associated with the named

s/(v4)/(v4 or v6)

* network. If a libvirt virtual network, that will be provided in the @@ -4540,12 +4541,12 @@ networkReleaseActualDevice(virDomainDefPtr dom, * completely unsupported. */ int -networkGetNetworkAddress(const char *netname, char **netaddr) +networkGetNetworkAddress(const char *netname, char **netaddr, bool want_ipv6) { int ret = -1; virNetworkObjPtr network; virNetworkDefPtr netdef; - virNetworkIpDefPtr ipdef; + virNetworkIpDefPtr ipdef = NULL;

This is unecessary since virNetworkDefGetIpByIndex returns NULL if not found. Other than that the rest seems OK. John

virSocketAddr addr; virSocketAddrPtr addrptr = NULL; char *dev_name = NULL; @@ -4566,12 +4567,12 @@ 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 */ - ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0); + /* if there's an ipdef, get it's IPv4 or IPv6 address */ + ipdef = virNetworkDefGetIpByIndex(netdef, want_ipv6 ? AF_INET6 : AF_INET, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("network '%s' doesn't have an IPv4 address"), - netdef->name); + _("network '%s' doesn't have an '%s' address"), + netdef->name, want_ipv6 ? "IPv6" : "IPv4"); break; } addrptr = &ipdef->address; @@ -4599,7 +4600,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) }

if (dev_name) { - if (virNetDevGetIPv4Address(dev_name, &addr) < 0) + if (virNetDevGetIPAddress(dev_name, want_ipv6, &addr) < 0) goto error; addrptr = &addr; } diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 2f801ee..465ab18 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, + bool want_ipv6) 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, want_ipv6) (-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 24b2ad9..82a4ce3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7286,7 +7286,9 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, + graphics->listens->family == + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, " @@ -7450,7 +7452,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0); if (!listenNetwork) break; - ret = networkGetNetworkAddress(listenNetwork, &netAddr); + ret = networkGetNetworkAddress(listenNetwork, &netAddr, + graphics->listens->family == + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6); if (ret <= -2) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("network-based listen not possible, "

On 03/09/2015 07:51 AM, John Ferlan wrote:

On 02/28/2015 04:08 AM, Luyao Huang wrote:

...

we already set a more clearly error in networkGetNetworkAddress, so this error will cover our error in networkGetNetworkAddress.

Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)

Change comment to:

Remove unnecessary virReportError on networkGetNetworkAddress return

More specific errors are set in the called functions when -1 is returned, so don't overwrite those messages.

Code seems fine and I traced functions as well.

Thanks for your review and help. New comment looks good for me :)

John

...

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 82a4ce3..cd0758d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7295,12 +7295,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. */ @@ -7461,12 +7458,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. */

Luyao