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";
+
+ 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);
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"));
+ 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;
+ }
+
+ 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;
+ }
+
+ 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"));
+ 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
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);
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Sep 12, 2017 at 11:32:53AM +0200, 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";
>+
>+ 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);
> 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"));
>+ goto cleanup;
Oh, here it is, I'd just say that routes are not supported. But
probably omit the previous message for other devices. Join them nicely
together, please.
>+ }
>+
>+ 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"));
I'm afraid this error message is pointless to a random observer. No
matter how funny I find it, this could be better...
>+ goto cleanup;
>+ }
>+
>+ 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;
>+ }
>+
>+ 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)) {
This could fail if you have:
<ip family='ipv6' prefix='119'/>
<ip family='ipv4' prefix='24'/>
I hope you see why ;)
>+ virReportError(VIR_ERR_XML_ERROR, "%s",
>+ _("prefix too long"));
>+ 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
>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);
>--
>2.13.5
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :-/)
> + 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/13/2017 06:46 AM, Michal Privoznik wrote:
> 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
>>>
>>> + /* 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
>
Right. So let's say the user requests x.x.x.3 for the IP - that would
conflict with the DNS server address.
I would really hate to have those specific checks hardcoded though (in
case qemu changed the addresses for some reason. Maybe if we're lucky,
qemu itself will error out if there is a conflict. If that's the case,
then we can just leave it to them to report the error (as long as we're
able to adequately scrape the error message so we can report something
sensible).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/13/2017 06:02 PM, Laine Stump wrote:
> On 09/13/2017 06:46 AM, Michal Privoznik wrote:
>> 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
>>>>
>>>> + /* 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
>>
>
> Right. So let's say the user requests x.x.x.3 for the IP - that would
> conflict with the DNS server address.
>
> I would really hate to have those specific checks hardcoded though (in
> case qemu changed the addresses for some reason. Maybe if we're lucky,
> qemu itself will error out if there is a conflict. If that's the case,
> then we can just leave it to them to report the error (as long as we're
> able to adequately scrape the error message so we can report something
> sensible).
>
Ah, qemu does not report any error. They merely take the default netmask
for given IP address and zero out the subnet part. This is what I tried:
<interface type='user'>
<mac address='00:11:22:33:44:55'/>
<ip address='172.17.2.2' family='ipv4'/>
<ip address='2001:db8:ac10:fd01::3' family='ipv6'/>
<model type='rtl8139'/>
</interface>
and this is how guest sees the interface:
3: ens8: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff
inet 172.16.2.15/12 brd 172.31.255.255 scope global dynamic ens8
valid_lft 86373sec preferred_lft 86373sec
inet6 2001:db8:ac10:fd01:a37e:8d3e:c189:d4f1/64 scope global noprefixroute dynamic
valid_lft 86374sec preferred_lft 14374sec
So, what if while parsing the IP address we would do the same? I mean,
zero out the subnet part of the address. If prefix was provided use that
otherwise use the default. Users are required to reload the domain XML
after any edit anyway. This also makes more sense IMO because users are
providing network address, not a host one.
Having said that, thanks for review on v2, but I'll postpone the push
until we have a clear agreement here.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.