Re: [libvirt] [PATCH 5/5] qemu: Implement usernet address

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&gt; > --- > 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 :-/)