[libvirt] [PATCH 0/8] (incomplete) fix of "peer" address feature
This patch series fixes this feature enoough that it works: 1) emits "peer" attribute in formatted XML when present in the NetDef object, so that the config will "stick" 2) swaps "address" and "peer" for qemu, so that "address" consistently refers to the IP address used by the guest, and "peer" to the address used by the host. 3) ... and the rest *BUT* it doesn't address the sub-optimal naming of the new attribute, nor does it fix the documentation which is incorrect not only in its description, but also in the starting version number for QEMU support. Also, I'm skeptical that this new feature is useful for the types of lxc interfaces that are supported (macvtap i.e. "direct", and a veth connected to a bridge) - from my understanding, it would only be useful for a type='ethernet' interface (a tap/veth pair not connected to any bridge), and that isn't supported by lxc yet; for type='bridge' and type='network' (which is also connecting to a bridge) I don't see the use case. So I'm torn about whether these patches should be put in for this release in order to made the already-pushed code work, or if we should just hold off until we: 1) find/agree on a better name for the new attribute (see my earlier mail titled 'interface "peer address" patches are broken for details on my opinion) 2) decide if it's actually useful to support the "peer" address for type='network|bridge" in lxc (it isn't in qemu). 3) fix the documentation (I started into that when I realized the By not pushing the fixes, we guarantee that nobody can use the feature, and thus will technically still be able to change the name of the attribute even after arelease has passed (because we won't break anyone's usable config). Opinions on what to do? (I would consider reverting the original patches temporarily until it's all sorted out, but I don't know what kind of conflicts that would cause; I know that there has been at least one bugfix patch) Laine Stump (8): conf: clean up virDomainNetIpParseXML() tests: mock virNetDevSetIPAddress conf: emit an IP address "peer" attribute in XML when present util: allow calling virSocketAddrGetIpPrefix with NULL netmask or address qemu/lxc: use correct prefix when setting tap/veth IP address qemu/lxc: log peer address when setting a domain interface's IP qemu: swap "address" and "peer" when setting IP address for tap interfaces qemu: require "peer" if ip address is supplied for an interface src/conf/domain_conf.c | 46 +++++++++++++--------- src/lxc/lxc_container.c | 36 +++++++++++++---- src/qemu/qemu_domain.c | 44 ++++++++++++++++++--- src/qemu/qemu_interface.c | 44 +++++++++++++++------ src/util/virsocketaddr.c | 8 ++-- src/util/virsocketaddr.h | 3 +- .../qemuxml2argv-net-eth-peer.args | 20 ++++++++++ .../qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml | 30 ++++++++++++++ tests/qemuxml2argvmock.c | 10 ++++- tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-net-eth-peer.xml | 33 ++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 226 insertions(+), 50 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-peer.xml -- 2.5.5
Rearrange this function to be better organized and more correct: * the error codes were changed from the incorrect INVALID_ARG to XML_ERROR * prefix still isn't required, but if present it must be valid or an error will be logged. * don't emit a debug log just because prefix or peer is missing - this is valid. * group everything related to setting peer in one place rather than scattered through the function. Same for peer. --- src/conf/domain_conf.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cb21e4..f7ee52b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5746,21 +5746,12 @@ virDomainNetIpParseXML(xmlNodePtr node) int family = AF_UNSPEC; char *address = NULL, *peer = NULL; - if (!(prefixStr = virXMLPropString(node, "prefix")) || - (virStrToLong_ui(prefixStr, NULL, 10, &prefixValue) < 0)) { - // Don't shout, as some old config may not have a prefix - VIR_DEBUG("Missing or invalid network prefix"); - } - if (!(address = virXMLPropString(node, "address"))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("Missing network address")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing required address in <ip>")); goto cleanup; } - if ((peer = virXMLPropString(node, "peer")) == NULL) - VIR_DEBUG("Peer is empty"); - familyStr = virXMLPropString(node, "family"); if (familyStr && STREQ(familyStr, "ipv4")) family = AF_INET; @@ -5773,21 +5764,32 @@ virDomainNetIpParseXML(xmlNodePtr node) goto cleanup; if (virSocketAddrParse(&ip->address, address, family) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), + virReportError(VIR_ERR_XML_ERROR, + _("Invalid address '%s' in <ip>"), address); goto cleanup; } - if ((peer != NULL) && (virSocketAddrParse(&ip->peer, peer, family) < 0)) { - virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse IP address: '%s'"), + prefixStr = virXMLPropString(node, "prefix"); + if (prefixStr && + ((virStrToLong_ui(prefixStr, NULL, 10, &prefixValue) < 0) || + (family == AF_INET6 && prefixValue > 128) || + (family == AF_INET && prefixValue > 32))) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid prefix value '%s' in <ip>"), + prefixStr); + goto cleanup; + } + ip->prefix = prefixValue; + + peer = virXMLPropString(node, "peer"); + if (peer && (virSocketAddrParse(&ip->peer, peer, family) < 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid peer address '%s' in <ip>"), peer); goto cleanup; } - ip->prefix = prefixValue; - ret = ip; ip = NULL; -- 2.5.5
Now that we can include <interface type='ethernet'> in tests, we could almost test XML that has an <ip> element in an interface. Except that the test fails when it tries to actually set the IP address for the interface's tap device. This patch mocks virNetDevSetIPAddress() to just return success. --- tests/qemuxml2argvmock.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..b1f1f6d 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -124,6 +124,14 @@ virNetDevSetMAC(const char *ifname ATTRIBUTE_UNUSED, return 0; } +int virNetDevSetIPAddress(const char *ifname ATTRIBUTE_UNUSED, + virSocketAddr *addr ATTRIBUTE_UNUSED, + virSocketAddr *peer ATTRIBUTE_UNUSED, + unsigned int prefix ATTRIBUTE_UNUSED) +{ + return 0; +} + int virNetDevSetOnline(const char *ifname ATTRIBUTE_UNUSED, bool online ATTRIBUTE_UNUSED) -- 2.5.5
This was accidentally left out of the patch that added support for setting a tap device's "peer" address, which rendered the new feature DOA. xml2argv and xml2xml tests were added to assure it doesn't disappear in the future. --- src/conf/domain_conf.c | 8 +++++- .../qemuxml2argv-net-eth-peer.args | 20 +++++++++++++ .../qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml | 30 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-net-eth-peer.xml | 33 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-peer.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f7ee52b..a7ae8ec 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19657,8 +19657,14 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips) VIR_FREE(ipStr); if (familyStr) virBufferAsprintf(buf, " family='%s'", familyStr); - if (ips[i]->prefix != 0) + if (ips[i]->prefix) virBufferAsprintf(buf, " prefix='%u'", ips[i]->prefix); + if (VIR_SOCKET_ADDR_VALID(&ips[i]->peer)) { + if (!(ipStr = virSocketAddrFormat(&ips[i]->peer))) + return -1; + virBufferAsprintf(buf, " peer='%s'", ipStr); + VIR_FREE(ipStr); + } virBufferAddLit(buf, "/>\n"); } return 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.args new file mode 100644 index 0000000..ab6230f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.args @@ -0,0 +1,20 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device virtio-net-pci,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \ +-net tap,fd=3,vlan=0,name=hostnet0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml new file mode 100644 index 0000000..8c22071 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-peer.xml @@ -0,0 +1,30 @@ +<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> + <controller type='usb' index='0' model='none'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='ethernet'> + <mac address='00:11:22:33:44:55'/> + <ip address='192.168.125.1' family='ipv4' prefix='24' peer='192.168.125.2'/> + <script path='/etc/qemu-ifup'/> + <model type='virtio'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b43f03c..eed2da9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -969,6 +969,7 @@ mymain(void) DO_TEST("net-eth", NONE); DO_TEST("net-eth-ifname", NONE); DO_TEST("net-eth-names", NONE); + DO_TEST("net-eth-peer", NONE); DO_TEST("net-client", NONE); DO_TEST("net-server", NONE); DO_TEST("net-mcast", NONE); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-peer.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-peer.xml new file mode 100644 index 0000000..11e56db --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-peer.xml @@ -0,0 +1,33 @@ +<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> + <controller type='usb' index='0' model='none'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <interface type='ethernet'> + <mac address='00:11:22:33:44:55'/> + <ip address='192.168.125.1' family='ipv4' prefix='24' peer='192.168.125.2'/> + <script path='/etc/qemu-ifup'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index f766f4d..5172865 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -454,6 +454,7 @@ mymain(void) DO_TEST("net-virtio-disable-offloads"); DO_TEST("net-eth"); DO_TEST("net-eth-ifname"); + DO_TEST("net-eth-peer"); DO_TEST("net-virtio-network-portgroup"); DO_TEST("net-hostdev"); DO_TEST("net-hostdev-vfio"); -- 2.5.5
There are times when we don't have a netmask pointer to give to virSocketAddrGetIpPrefix() (e.g. the IP addresses in domain interfaces only have a prefix, no netmask), but it would have caused a segv if we called it with NULL instead of a pointer to a netmask. This patch qualifies the code that would use the netmask or address pointers to check for NULL first. --- src/util/virsocketaddr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 4b45681..4a7e443 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2015 Red Hat, Inc. + * Copyright (C) 2009-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1010,9 +1010,9 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, { if (prefix > 0) { return prefix; - } else if (VIR_SOCKET_ADDR_VALID(netmask)) { + } else if (netmask && VIR_SOCKET_ADDR_VALID(netmask)) { return virSocketAddrGetNumNetmaskBits(netmask); - } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) { + } else if (address && VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) { /* Return the natural prefix for the network's ip address. * On Linux we could use the IN_CLASSx() macros, but those * aren't guaranteed on all platforms, so we just deal with @@ -1037,7 +1037,7 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, return 24; } return -1; - } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) { + } else if (address && VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) { if (virSocketAddrIsWildcard(address)) return 0; return 64; -- 2.5.5
Commit c9a641 (first appearred in 1.2.12) added support for setting the guest-side IP address of veth devices in lxc domains, and commit 6e244c (not yet in a release) used that as a model to allow setting the IP address of tap devices in qemu domains. Unfortunately, the original hardcoded the assumption that the proper prefix for any IP address with no explicit prefix in the config should be "24"; that is only correct for class C IPv4 addresses, but not for any other IPv4 address, nor for any IPv6 address. The good news is that there is already a function in libvirt that will determine the proper default prefix for any IP address. This patch replaces the two uses of the ill-fated VIR_SOCKET_ADDR_DEFAULT_PREFIX with calls to virSocketAddrGetIpPrefix(). --- src/lxc/lxc_container.c | 15 +++++++++++---- src/qemu/qemu_interface.c | 13 ++++++++++--- src/util/virsocketaddr.h | 3 +-- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index a1deb0c..15dacf1 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2015 Red Hat, Inc. + * Copyright (C) 2008-2016 Red Hat, Inc. * Copyright (C) 2008 IBM Corp. * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. * @@ -514,11 +514,18 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, for (j = 0; j < netDef->nips; j++) { virDomainNetIpDefPtr ip = netDef->ips[j]; - unsigned int prefix = (ip->prefix > 0) ? ip->prefix : - VIR_SOCKET_ADDR_DEFAULT_PREFIX; + int prefix; char *ipStr = virSocketAddrFormat(&ip->address); - VIR_DEBUG("Adding IP address '%s/%u' to '%s'", + if ((prefix = virSocketAddrGetIpPrefix(&ip->address, + NULL, ip->prefix)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to determine prefix for IP address '%s'"), + ipStr); + goto error_out; + } + + VIR_DEBUG("Adding IP address '%s/%d' to '%s'", ipStr, ip->prefix, newname); if (virNetDevSetIPAddress(newname, &ip->address, &ip->peer, prefix) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index a4e9d86..34fe30e 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -450,11 +450,18 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, for (i = 0; i < net->nips; i++) { virDomainNetIpDefPtr ip = net->ips[i]; - unsigned int prefix = (ip->prefix > 0) ? ip->prefix : - VIR_SOCKET_ADDR_DEFAULT_PREFIX; + int prefix; char *ipStr = virSocketAddrFormat(&ip->address); - VIR_DEBUG("Adding IP address '%s/%u' to '%s'", + if ((prefix = virSocketAddrGetIpPrefix(&ip->address, + NULL, ip->prefix)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to determine prefix for IP address '%s'"), + ipStr); + goto cleanup; + } + + VIR_DEBUG("Adding IP address '%s/%d' to '%s'", ipStr, ip->prefix, net->ifname); if (virNetDevSetIPAddress(net->ifname, &ip->address, &ip->peer, prefix) < 0) { diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index c7aaa61..a7bec5d 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2013, 2015 Red Hat, Inc. + * Copyright (C) 2009-2013, 2015-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -54,7 +54,6 @@ typedef struct { # define VIR_SOCKET_ADDR_FAMILY(s) \ ((s)->data.sa.sa_family) -# define VIR_SOCKET_ADDR_DEFAULT_PREFIX 24 # define VIR_SOCKET_ADDR_IPV4_ALL "0.0.0.0" # define VIR_SOCKET_ADDR_IPV6_ALL "::" -- 2.5.5
--- src/lxc/lxc_container.c | 23 ++++++++++++++++++----- src/qemu/qemu_interface.c | 24 ++++++++++++++++-------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 15dacf1..7f4ce72 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -494,6 +494,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, char *toStr = NULL; char *viaStr = NULL; virDomainNetDefPtr netDef; + char *ipStr = NULL, *peerStr = NULL; bool privNet = vmDef->features[VIR_DOMAIN_FEATURE_PRIVNET] == VIR_TRISTATE_SWITCH_ON; @@ -515,7 +516,16 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, for (j = 0; j < netDef->nips; j++) { virDomainNetIpDefPtr ip = netDef->ips[j]; int prefix; - char *ipStr = virSocketAddrFormat(&ip->address); + + VIR_FREE(ipStr); + VIR_FREE(peerStr); + + + ipStr = virSocketAddrFormat(&ip->address); + if (VIR_SOCKET_ADDR_VALID(&ip->peer)) + peerStr = virSocketAddrFormat(&ip->peer); + else if (VIR_STRDUP(peerStr, "[none]") < 0) + goto error_out; if ((prefix = virSocketAddrGetIpPrefix(&ip->address, NULL, ip->prefix)) < 0) { @@ -525,12 +535,13 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, goto error_out; } - VIR_DEBUG("Adding IP address '%s/%d' to '%s'", - ipStr, ip->prefix, newname); + VIR_DEBUG("Adding IP address '%s/%d' (peer %s) to '%s'", + ipStr, prefix, peerStr, newname); + if (virNetDevSetIPAddress(newname, &ip->address, &ip->peer, prefix) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to set IP address '%s' on %s"), - ipStr, newname); + _("Failed to set IP address '%s/%d' (peer %s) on %s"), + ipStr, prefix, peerStr, newname); VIR_FREE(ipStr); goto error_out; } @@ -568,6 +579,8 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, rc = virNetDevSetOnline("lo", true); error_out: + VIR_FREE(ipStr); + VIR_FREE(peerStr); VIR_FREE(toStr); VIR_FREE(viaStr); VIR_FREE(newname); diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 34fe30e..58bbe71 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -411,6 +411,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, bool template_ifname = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun"; + char *ipStr = NULL, *peerStr = NULL; size_t i; if (net->backend.tap) { @@ -451,7 +452,15 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, for (i = 0; i < net->nips; i++) { virDomainNetIpDefPtr ip = net->ips[i]; int prefix; - char *ipStr = virSocketAddrFormat(&ip->address); + + VIR_FREE(ipStr); + VIR_FREE(peerStr); + + ipStr = virSocketAddrFormat(&ip->address); + if (VIR_SOCKET_ADDR_VALID(&ip->peer)) + peerStr = virSocketAddrFormat(&ip->peer); + else if (VIR_STRDUP(peerStr, "[none]") < 0) + goto cleanup; if ((prefix = virSocketAddrGetIpPrefix(&ip->address, NULL, ip->prefix)) < 0) { @@ -461,17 +470,15 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, goto cleanup; } - VIR_DEBUG("Adding IP address '%s/%d' to '%s'", - ipStr, ip->prefix, net->ifname); + VIR_DEBUG("Adding IP address '%s/%d' (peer %s) to '%s'", + ipStr, prefix, peerStr, net->ifname); if (virNetDevSetIPAddress(net->ifname, &ip->address, &ip->peer, prefix) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to set IP address '%s' on %s"), - ipStr, net->ifname); - VIR_FREE(ipStr); + _("Failed to set IP address '%s/%u' (peer %s) on %s"), + ipStr, prefix, peerStr, net->ifname); goto cleanup; } - VIR_FREE(ipStr); } if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP || @@ -519,7 +526,8 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, VIR_FREE(net->ifname); } virObjectUnref(cfg); - + VIR_FREE(ipStr); + VIR_FREE(peerStr); return ret; } -- 2.5.5
Commit 6e244c added the ability to set the IP address and peer address for the tap interface used by <interface type='ethernet'> in qemu. However, it sets the "address" on the host-side of the tap, and the "peer" on the guest side. This is opposite to what is done with veth interfaces in the lxc driver, where "address" is the guest veth of the pair, and "peer" is the host side veth. This patch makes the two operate consistently from the user's point of view, by swapping peer and address when setting the IP info of the tap device. Although the peer device is optional in the virNetDevSetIPAddress call, once we've swapped them, it is the "address" from the config that is being set as the peer in the device, and vice versa; Since address is *not* optional when calling virNetDevSetIPAddress, this means that "peer" is required in the config, so we check that "peer" is present, and log an error if it isn't. --- src/qemu/qemu_interface.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 58bbe71..6326b0a 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -457,10 +457,17 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, VIR_FREE(peerStr); ipStr = virSocketAddrFormat(&ip->address); - if (VIR_SOCKET_ADDR_VALID(&ip->peer)) + if (VIR_SOCKET_ADDR_VALID(&ip->peer)) { peerStr = virSocketAddrFormat(&ip->peer); - else if (VIR_STRDUP(peerStr, "[none]") < 0) + } else { + /* for qemu tap devices, the "peer" address is actually + * the address of the interface itself, so it must be set + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing required peer for address '%s'"), + ipStr); goto cleanup; + } if ((prefix = virSocketAddrGetIpPrefix(&ip->address, NULL, ip->prefix)) < 0) { @@ -473,7 +480,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, VIR_DEBUG("Adding IP address '%s/%d' (peer %s) to '%s'", ipStr, prefix, peerStr, net->ifname); - if (virNetDevSetIPAddress(net->ifname, &ip->address, &ip->peer, prefix) < 0) { + if (virNetDevSetIPAddress(net->ifname, &ip->peer, &ip->address, prefix) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, _("Failed to set IP address '%s/%u' (peer %s) on %s"), ipStr, prefix, peerStr, net->ifname); -- 2.5.5
Since "peer" is really the address of the tap interface, while "address" is actually the peer address of the tap interface, and since you can't set the peer address of a POINTOPOINT tap device without setting the local address, we need to require "peer" in the config. --- src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f2488ad..03baa5a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1788,15 +1788,49 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virQEMUCapsPtr qemuCaps = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; + size_t i; qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator); - if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !net->model && + VIR_STRDUP(net->model, + qemuDomainDefaultNetModel(def, qemuCaps)) < 0) { goto cleanup; + } + + if (net->nips) { + /* we currently only support setting an IP address + * for <interface type='ethernet'> + */ + if (net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("ip addresses cannot be set in config " + "for interfaces of type '%s'"), + virDomainNetTypeToString(net->type)); + goto cleanup; + } + /* verify that every ip has a peer address set, + * since the "peer" attribute in the config becomes + * the tap device's local IP address, which is required + */ + for (i = 0; i < net->nips; i++) { + virDomainNetIpDefPtr ip = net->ips[i]; + + if (!VIR_SOCKET_ADDR_VALID(&ip->peer)) { + char *ipStr = virSocketAddrFormat(&ip->address); + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("interface ip address '%s' must " + "have a peer address set in config"), + ipStr ? ipStr : "(unknown)"); + VIR_FREE(ipStr); + goto cleanup; + } + } + } } /* set default disk types and drivers */ -- 2.5.5
On Thu, 2016-04-28 at 10:51 -0400, Laine Stump wrote:
This patch series fixes this feature enoough that it works: 1) emits "peer" attribute in formatted XML when present in the NetDef object, so that the config will "stick" 2) swaps "address" and "peer" for qemu, so that "address" consistently refers to the IP address used by the guest, and "peer" to the address used by the host. 3) ... and the rest *BUT* it doesn't address the sub-optimal naming of the new attribute, nor does it fix the documentation which is incorrect not only in its description, but also in the starting version number for QEMU support. Also, I'm skeptical that this new feature is useful for the types of lxc interfaces that are supported (macvtap i.e. "direct", and a veth connected to a bridge) - from my understanding, it would only be useful for a type='ethernet' interface (a tap/veth pair not connected to any bridge), and that isn't supported by lxc yet; for type='bridge' and type='network' (which is also connecting to a bridge) I don't see the use case. So I'm torn about whether these patches should be put in for this release in order to made the already-pushed code work, or if we should just hold off until we: 1) find/agree on a better name for the new attribute (see my earlier mail titled 'interface "peer address" patches are broken for details on my opinion) 2) decide if it's actually useful to support the "peer" address for type='network|bridge" in lxc (it isn't in qemu). 3) fix the documentation (I started into that when I realized the By not pushing the fixes, we guarantee that nobody can use the feature, and thus will technically still be able to change the name of the attribute even after arelease has passed (because we won't break anyone's usable config). Opinions on what to do? (I would consider reverting the original patches temporarily until it's all sorted out, but I don't know what kind of conflicts that would cause; I know that there has been at least one bugfix patch)
I think reverting would be the best choice, assuming the result looks safe enough; failing that, shipping the feature in a shape that makes it unusable in practice seems preferable than rushing naming decisions we'll have to live with forever. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Laine Stump