On 09/12/2017 05:53 AM, Martin Kletzander wrote:

On Tue, Sep 12, 2017 at 11:32:52AM +0200, Michal Privoznik wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1075520

Currently, all that users can specify for an interface type of 'user' is the common attributes: PCI address, NIC model (and that's basically it). However, some need to configure other address range than the default one.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---

Notes:    Frankly, I'm not that convinced about this one. I mean,    this puts IP addresses into net->hostIP while we are    configuring guest side of the SLIRP. However, it just    feels better to have the IP addresses under <source/>    than under <interface/>. Which actually is the other    option. So it's either:

   <interface type='user'>      <source>        <ip address='1.2.2.4'/>      </source>    </interface>

   for net->hostIP, or it's:

   <interface type='user'>      <ip address='1.2.2.4'/>    </interface>

In order to be consistent between all different hypervisors and interface connection types, any ip addresses and routes that are set to be visible on the host side of the interface "apparatus" must be under <source>, and those that are set to be visible on the guest side must be directly under the top level of <interface>. Since the IP you're configuring in this patch will be seen in the guest, it needs to be at the toplevel (i.e. it will be parsed into net->guestIP).

I'm not convinced either.  If you don't like the fact that it's in hostIP (even though we're setting up the backend of that device, not the device itself, it's just the IP that the internal DHCP server should send to the guest) and want to have it in guestIP (which would make sense as well) then it should be:

  <interface type='user'>     <target>       <ip address='1.2.2.4'/>     </target>   </interface>

When I dropped into this code to add support for specifying the IP address and routes on the *host* side of a tap device/veth device (see commit 98fa8f3ef) for QEMU and LXC (along with the "peer IP" for the other end of a tap/veth, which is *not* the same thing!), I *wished* that we had added the new <ip> and <route> elements for specifying guest-side <ip> and <route> under <target> as you suggest, since <target> is intended to contain items configuring how the device appears on the guest side[*]. Unfortunately, the decision on the location of guest-side <ip> was made *long* ago (at least prior to July of 2008, I didn't feel like spelunking any further), when <ip> was added directly under <interface> to support setting the guest-side IP address on Xen. Many years later (2014), when cbossdonnat added support for setting the guest-side IP of  an LXC veth pair, he also used the same element in order to provide consistency. So, we are unable to put it under <target>, and that decision was made sometime befor 2008, [*] Of course, the <target> element is used inconsistently within <interface> when compared to other uses - <target dev='blah'/> is used to specify the name of the tap/macvtap device *as it is known on the host side*. <target> has been described to me as a place to configure what things look like to the guest (e.g. the intended name for a disk device on the guest)(which is anyway ignored everywhere except Xen, but that's a different topic!), so using it to configure a name that's visible only to the host seems wrong, but it's been that way since sometime long before even I was involved with libvirt, so there is no changing it now :-P

Which would cleanly correlate to that (and I also don't like the look of it).  So I'll leave this to a further discussion.

I think it *would* have been good under <target>, if anyone had foreseen the need for a <target> grouping (and consistently used it) from the beginning. But that train sailed into space long ago, so in order to be backward compatible and consistent, the guest-side IP/routes need to be directly under <interface> (just like the guest-side PCI address for PCI devices is), and the host-side IP/routes need to go under <source> (just as host-side PCI addresses of <hostdev> devices are).

...

   for net->guestIP. I went for the former one, but I    don't have a strong opinion on that.

That's good, because I *do*! :-P

...

docs/formatdomain.html.in                          | 13 ++++++- docs/schemas/domaincommon.rng                      |  5 +++ src/conf/domain_conf.c                             |  5 +-- .../qemuxml2argv-net-user-addr.xml                 | 42 ++++++++++++++++++++++ .../qemuxml2xmlout-net-user-addr.xml               |  1 + tests/qemuxml2xmltest.c                            |  1 + 6 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-user-addr.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8ca7637a4..65a8886ee 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4701,7 +4701,14 @@       starting from <code>10.0.2.15</code>. The default router will be       <code>10.0.2.2</code> and the DNS server will be <code>10.0.2.3</code>.       This networking is the only option for unprivileged users who need their -      VMs to have outgoing access. +      VMs to have outgoing access. However, <span class="since">since +        3.8.0</span>, it is possible to override the default network by +      including <code>source</code> element. The element can have <code>ip</code> +      element for each IPv4 and IPv6. The element has <code>family</code> +      attribute which accepts <code>ipv4</code> and <code>ipv6</code> values. +      Then there's <code>address</code> attribute for specifying IP address +      corresponding to the family. Optionally, the default prefix length can be +      overridden via <code>prefix</code> attribute.

This will of course need to be rewritten (and grammar cleaned up). You realize now that you've added support for setting the IP address, inevitably someone will expect you to write patches to support setting of the DHCP server, DNS server, default route, DHCP range, hostname, NMBD server, domain name, tftp directory and bootfile,.... :-O)

...

    </p>

<pre> @@ -4711,6 +4718,10 @@   ...   <interface type='user'>     <mac address="00:11:22:33:44:55"/> +    <source> +      <ip family='ipv4' address='172.17.2.0' prefix='24'/> +      <ip family='ipv6' address='2001:db8:ac10:fd01::' prefix='64'/> +    </source>   </interface> </devices> ...</pre> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c9a4f7a9a..7b5292bd3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2428,6 +2428,11 @@             <value>user</value>           </attribute>           <interleave> +            <optional> +              <element name="source"> +                <ref name="interface-ip-info"/>

This also allows <route/> here, I would split the definition and disallow that.  Just for the sake of pointless bugs flying by.

Or maybe just check for presence of <route> during parsing and issue an ENOTSUPP log if it's found (that will be needed anyway in case XML validation is turned off). There's a certain point of detail when we stop caring about the exactness of the RNG. Eventually it becomes more difficult to read the RNG than it's worth, and the error messages provided when validation fails.

...

+              </element> +            </optional>             <ref name="interface-options"/>           </interleave>         </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 676fc0f34..ef236757a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5214,12 +5214,13 @@ static int virDomainNetDefValidate(const virDomainNetDef *net) {     if ((net->hostIP.nroutes || net->hostIP.nips) && -        net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) { +        net->type != VIR_DOMAIN_NET_TYPE_ETHERNET && +        net->type != VIR_DOMAIN_NET_TYPE_USER) {         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,                        _("Invalid attempt to set network interface "                          "host-side IP route and/or address info on "                          "interface of type '%s'. This is only supported " -                         "on interfaces of type 'ethernet'"), +                         "on interfaces of type 'ethernet' and 'user'"),

Same here, you even give the hint that routes are supported for the type='user'

Yeah, this should be separated. It was previously combined for brevity, only because the two were both only supported in exactly the same cases. Also, it looks like qemu only supports a single IPv4 and single IPv6 address on each interface, so you need to validate there is at most one of each type.

...

                       virDomainNetTypeToString(net->type));         return -1;     } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.xml new file mode 100644 index 000000000..65362d8aa --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> +  <name>QEMUGuest1</name> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> +  <memory unit='KiB'>219136</memory> +  <currentMemory unit='KiB'>219136</currentMemory> +  <vcpu placement='static'>1</vcpu> +  <os> +    <type arch='i686' machine='pc'>hvm</type> +    <boot dev='hd'/> +  </os> +  <clock offset='utc'/> +  <on_poweroff>destroy</on_poweroff> +  <on_reboot>restart</on_reboot> +  <on_crash>destroy</on_crash> +  <devices> +    <emulator>/usr/bin/qemu-system-i686</emulator> +    <disk type='block' device='disk'> +      <driver name='qemu' type='raw'/> +      <source dev='/dev/HostVG/QEMUGuest1'/> +      <target dev='hda' bus='ide'/> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/> +    </disk> +    <controller type='usb' index='0'> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> +    </controller> +    <controller type='ide' index='0'> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> +    </controlleer> +    <controller type='pci' index='0' model='pci-root'/> +    <interface type='user'> +      <mac address='00:11:22:33:44:55'/> +      <source> +        <ip address='172.17.2.0' family='ipv4' prefix='24'/>

Also add ipv6 into the test.

...

+      </source> +      <model type='rtl8139'/> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> +    </interface> +    <input type='mouse' bus='ps2'/> +    <input type='keyboard' bus='ps2'/> +    <memballoon model='none'/> +  </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-user-addr.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-user-addr.xml new file mode 120000 index 000000000..fd909ec24 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-user-addr.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-net-user-addr.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0a87cedf2..99b15ad0c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -531,6 +531,7 @@ mymain(void)     DO_TEST("misc-uuid", NONE);     DO_TEST("net-vhostuser", NONE);     DO_TEST("net-user", NONE); +    DO_TEST("net-user-addr", NONE);     DO_TEST("net-virtio", NONE);     DO_TEST("net-virtio-device", NONE);     DO_TEST("net-virtio-disable-offloads", NONE); -- 2.13.5

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 09/12/2017 07:35 PM, Laine Stump wrote:

On 09/12/2017 05:53 AM, Martin Kletzander wrote:

...

On Tue, Sep 12, 2017 at 11:32:52AM +0200, Michal Privoznik wrote:

...

https://bugzilla.redhat.com/show_bug.cgi?id=1075520

Currently, all that users can specify for an interface type of 'user' is the common attributes: PCI address, NIC model (and that's basically it). However, some need to configure other address range than the default one.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---

Notes:    Frankly, I'm not that convinced about this one. I mean,    this puts IP addresses into net->hostIP while we are    configuring guest side of the SLIRP. However, it just    feels better to have the IP addresses under <source/>    than under <interface/>. Which actually is the other    option. So it's either:

   <interface type='user'>      <source>        <ip address='1.2.2.4'/>      </source>    </interface>

   for net->hostIP, or it's:

   <interface type='user'>      <ip address='1.2.2.4'/>    </interface>

In order to be consistent between all different hypervisors and interface connection types, any ip addresses and routes that are set to be visible on the host side of the interface "apparatus" must be under <source>, and those that are set to be visible on the guest side must be directly under the top level of <interface>. Since the IP you're configuring in this patch will be seen in the guest, it needs to be at the toplevel (i.e. it will be parsed into net->guestIP).

...

I'm not convinced either.  If you don't like the fact that it's in hostIP (even though we're setting up the backend of that device, not the device itself, it's just the IP that the internal DHCP server should send to the guest) and want to have it in guestIP (which would make sense as well) then it should be:

  <interface type='user'>     <target>       <ip address='1.2.2.4'/>     </target>   </interface>

When I dropped into this code to add support for specifying the IP address and routes on the *host* side of a tap device/veth device (see commit 98fa8f3ef) for QEMU and LXC (along with the "peer IP" for the other end of a tap/veth, which is *not* the same thing!), I *wished* that we had added the new <ip> and <route> elements for specifying guest-side <ip> and <route> under <target> as you suggest, since <target> is intended to contain items configuring how the device appears on the guest side[*]. Unfortunately, the decision on the location of guest-side <ip> was made *long* ago (at least prior to July of 2008, I didn't feel like spelunking any further), when <ip> was added directly under <interface> to support setting the guest-side IP address on Xen.

Many years later (2014), when cbossdonnat added support for setting the guest-side IP of  an LXC veth pair, he also used the same element in order to provide consistency. So, we are unable to put it under <target>, and that decision was made sometime befor 2008,

[*] Of course, the <target> element is used inconsistently within <interface> when compared to other uses - <target dev='blah'/> is used to specify the name of the tap/macvtap device *as it is known on the host side*. <target> has been described to me as a place to configure what things look like to the guest (e.g. the intended name for a disk device on the guest)(which is anyway ignored everywhere except Xen, but that's a different topic!), so using it to configure a name that's visible only to the host seems wrong, but it's been that way since sometime long before even I was involved with libvirt, so there is no changing it now :-P

Okay, I'll move it up so that it's under <interface/> directly.

...

Which would cleanly correlate to that (and I also don't like the look of it).  So I'll leave this to a further discussion.

I think it *would* have been good under <target>, if anyone had foreseen the need for a <target> grouping (and consistently used it) from the beginning. But that train sailed into space long ago, so in order to be backward compatible and consistent, the guest-side IP/routes need to be directly under <interface> (just like the guest-side PCI address for PCI devices is), and the host-side IP/routes need to go under <source> (just as host-side PCI addresses of <hostdev> devices are).

...

...

   for net->guestIP. I went for the former one, but I    don't have a strong opinion on that.

That's good, because I *do*! :-P

...

...

docs/formatdomain.html.in                          | 13 ++++++- docs/schemas/domaincommon.rng                      |  5 +++ src/conf/domain_conf.c                             |  5 +-- .../qemuxml2argv-net-user-addr.xml                 | 42 ++++++++++++++++++++++ .../qemuxml2xmlout-net-user-addr.xml               |  1 + tests/qemuxml2xmltest.c                            |  1 + 6 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-addr.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-user-addr.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8ca7637a4..65a8886ee 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4701,7 +4701,14 @@       starting from <code>10.0.2.15</code>. The default router will be       <code>10.0.2.2</code> and the DNS server will be <code>10.0.2.3</code>.       This networking is the only option for unprivileged users who need their -      VMs to have outgoing access. +      VMs to have outgoing access. However, <span class="since">since +        3.8.0</span>, it is possible to override the default network by +      including <code>source</code> element. The element can have <code>ip</code> +      element for each IPv4 and IPv6. The element has <code>family</code> +      attribute which accepts <code>ipv4</code> and <code>ipv6</code> values. +      Then there's <code>address</code> attribute for specifying IP address +      corresponding to the family. Optionally, the default prefix length can be +      overridden via <code>prefix</code> attribute.

This will of course need to be rewritten (and grammar cleaned up).

You realize now that you've added support for setting the IP address, inevitably someone will expect you to write patches to support setting of the DHCP server, DNS server, default route, DHCP range, hostname, NMBD server, domain name, tftp directory and bootfile,.... :-O)

For that I have already an answer prepared: Patches are welcome :-P

...

...

    </p>

<pre> @@ -4711,6 +4718,10 @@   ...   <interface type='user'>     <mac address="00:11:22:33:44:55"/> +    <source> +      <ip family='ipv4' address='172.17.2.0' prefix='24'/> +      <ip family='ipv6' address='2001:db8:ac10:fd01::' prefix='64'/> +    </source>   </interface> </devices> ...</pre> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c9a4f7a9a..7b5292bd3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2428,6 +2428,11 @@             <value>user</value>           </attribute>           <interleave> +            <optional> +              <element name="source"> +                <ref name="interface-ip-info"/>

This also allows <route/> here, I would split the definition and disallow that.  Just for the sake of pointless bugs flying by.

Or maybe just check for presence of <route> during parsing and issue an ENOTSUPP log if it's found (that will be needed anyway in case XML validation is turned off). There's a certain point of detail when we stop caring about the exactness of the RNG. Eventually it becomes more difficult to read the RNG than it's worth, and the error messages provided when validation fails.

Agreed. There's a lot of places where our schema is looser than our parser. And I don't think that's a problem. The only way we could have 1:1 correspondence is if we generated the parser from RNG schema. And even then there are per driver nuances (what's allowed in driver X is forbidden/not yet implemented in driver Y).

...

...

+              </element> +            </optional>             <ref name="interface-options"/>           </interleave>         </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 676fc0f34..ef236757a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5214,12 +5214,13 @@ static int virDomainNetDefValidate(const virDomainNetDef *net) {     if ((net->hostIP.nroutes || net->hostIP.nips) && -        net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) { +        net->type != VIR_DOMAIN_NET_TYPE_ETHERNET && +        net->type != VIR_DOMAIN_NET_TYPE_USER) {         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,                        _("Invalid attempt to set network interface "                          "host-side IP route and/or address info on "                          "interface of type '%s'. This is only supported " -                         "on interfaces of type 'ethernet'"), +                         "on interfaces of type 'ethernet' and 'user'"),

Same here, you even give the hint that routes are supported for the type='user'

Yeah, this should be separated. It was previously combined for brevity, only because the two were both only supported in exactly the same cases.

Also, it looks like qemu only supports a single IPv4 and single IPv6 address on each interface, so you need to validate there is at most one of each type.

Yep. I'm doing that in the next patch. Since this patch aims on config only, I wanted to keep it generic enough and check only obviously wrong values, e.g. IP addresses from movies like 218.108.149.373 (Mr Robot ftw! But apart from that I was surprised how quite accurate it was). Then, if a driver poses stricter limits those should be checked there now that we have per driver per device validation callbacks. One exception to this rule is, if all drivers forbid certain value/combination, it can go to the generic src/conf/ callback instead of copying the same lines all over the place. Michal

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

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