On 09/12/2017 05:32 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1075520
Apart from generic checks, we need to constrain netmask/prefix
lenght a bit. Thing is, with current implementation QEMU needs to
be able to 'assign' some IP addresses to the virtual network. For
instance, the default gateway is at x.x.x.2, dns is at x.x.x.3,
the default DHCP range is x.x.x.15-x.x.x.30. Since we don't
expose these settings yet, it's safer to require shorter prefix
to have room for the defaults.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_command.c | 22 ++++++++++
src/qemu/qemu_domain.c | 49 ++++++++++++++++++++++
.../qemuxml2argv-net-user-addr.args | 25 +++++++++++
tests/qemuxml2argvtest.c | 1 +
4 files changed, 97 insertions(+)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d553df57f..3e1bc5fa8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3805,6 +3805,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
virDomainNetType netType = virDomainNetGetActualType(net);
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
size_t i;
+ char *addr = NULL;
char *ret = NULL;
if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
@@ -3873,6 +3874,26 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
break;
case VIR_DOMAIN_NET_TYPE_USER:
+ virBufferAsprintf(&buf, "user%c", type_sep);
+ for (i = 0; i < net->hostIP.nips; i++) {
+ const virNetDevIPAddr *ip = net->hostIP.ips[i];
+ const char *prefix = "";
+
+ if (!(addr = virSocketAddrFormat(&ip->address)))
+ goto cleanup;
+
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET))
+ prefix = "net=";
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6))
+ prefix = "ipv6-net";
You're missing an "=" after ipv6-net.
+
+ virBufferAsprintf(&buf, "%s%s", prefix, addr);
+ if (ip->prefix)
+ virBufferAsprintf(&buf, "/%u", ip->prefix);
+ virBufferAddChar(&buf, ',');
+ }
+ break;
+
case VIR_DOMAIN_NET_TYPE_INTERNAL:
virBufferAsprintf(&buf, "user%c", type_sep);
break;
@@ -3928,6 +3949,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
cleanup:
virBufferFreeAndReset(&buf);
virObjectUnref(cfg);
+ VIR_FREE(addr);
You could have made addr local to the NET_TYPE_USER case, but then we
would have to move it out if we added any additional error checking in
the future (i.e. I agree with you putting it in the scope of the entire
function)
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 72031893f..bf0c7300c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3338,9 +3338,11 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
void *opaque ATTRIBUTE_UNUSED)
{
int ret = -1;
+ size_t i;
if (dev->type == VIR_DOMAIN_DEVICE_NET) {
const virDomainNetDef *net =
dev->data.net;
+ bool hasIPv4 = false, hasIPv6 = false;
if (net->guestIP.nroutes || net->guestIP.nips) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -3350,6 +3352,53 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
goto cleanup;
}
+ if (net->type == VIR_DOMAIN_NET_TYPE_USER) {
+ if (net->hostIP.nroutes) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Invalid attempt to set network interface "
+ "guest-side IP address info, "
+ "not supported by QEMU"));
I agree with Martin that you should specifically say that you can't set
<route>, not "IP address info". Of course this will need to change,
since it's guestIP.nips that we want to allow.
+ goto cleanup;
+ }
+
+ for (i = 0; i < net->hostIP.nips; i++) {
+ const virNetDevIPAddr *ip = net->hostIP.ips[i];
+
+ if (VIR_SOCKET_ADDR_VALID(&ip->peer)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("peers are not supported by QEMU"));
+ goto cleanup;
Aside from the humor that Martin finds in this message, it's also not
correct. It *is* allowed to set a peer IP on the host side of a
*tap-based* network device with qemu. (and not allowed to set the peer
IP on the guest side of *any* type of network device).
+ }
+
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET)) {
+ if (hasIPv4) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only one IPv4 address allowed"));
+ goto cleanup;
+ }
+ hasIPv4 = true;
Aha! You *are* checking for this! (maybe say "... per interface ...")
+ }
+
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6)) {
+ if (hasIPv6) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only one IPv6 address allowed"));
+ goto cleanup;
+ }
+ hasIPv6 = true;
+ }
+
+ /* QEMU needs some space to have
+ * some other 'hosts' on the network. */
+ if ((hasIPv4 && ip->prefix > 27) ||
+ (hasIPv6 && ip->prefix > 120)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("prefix too long"));
You should also probably check for conflict with the addresses that are
automatically used by slirp for DNS, default route, dhcp server, etc.
(Although I *hate* needing to hard code stuff like that :-/)
+ goto cleanup;
+ }
+ }
+ }
+
if (STREQ_NULLABLE(net->model, "virtio")) {
if (net->driver.virtio.rx_queue_size &
(net->driver.virtio.rx_queue_size - 1)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.args
b/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.args
new file mode 100644
index 000000000..51aacedb3
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device rtl8139,vlan=0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \
+-net user,net=172.17.2.0/24,vlan=0,name=hostnet0
You really should add QEMU_CAPS_NETDEV to the test case (in the previous
patch, too, for consistency) so that this test represents what is
produced on a more modern qemu. (actually, I would propose that all
other tests for network devices be changed to do the same thing - right
now the network test cases are verifying that we generate correct
commandlines for a version of qemu that probably hasn't existed in the
wild for a very long time).
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 2c040e4c0..584df017a 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1159,6 +1159,7 @@ mymain(void)
QEMU_CAPS_NETDEV,
QEMU_CAPS_VHOSTUSER_MULTIQUEUE);
DO_TEST("net-user", NONE);
+ DO_TEST("net-user-addr", NONE);
DO_TEST("net-virtio", NONE);
DO_TEST("net-virtio-device",
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_TX_ALG);