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

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

 



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 :-/)


> +                    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);


--
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