On 09/12/2017 09:16 PM, Laine Stump wrote:
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@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 :-/)
Do I? If the prefix allows sufficiently enough room for them then the defaults shouldn't clash. Or you mean checking against host's IPs? The defaults according to the qemu docs are as follows: host = x.x.x.2 dhcpstart = x.x.x.15 - x.x.x.30 dns = x.x.x.3 smb = x.x.x.4 Michal