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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
