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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux