Re: [libvirt PATCH 5/9] conf: parse/format passt-related XML additions

["Ján Tomko" <jtomko@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

On a Sunday in 2023, Laine Stump wrote:
>This implements XML config to represent a subset of the features
>supported by 'passt' (https://passt.top), which is an alternative
>backend for emulated network devices that requires no elevated
>privileges (similar to slirp, but "better").
>
>Along with setting the backend to use passt (via <backend
>type='passt'/> when the interface type='user'), we also support
>passt's --log-file and --interface options (via the <backend>
>subelement logFile and upstream attributes) and its --tcp-ports and
>--udp-ports options (which selectively forward incoming connections to
>the host on to the guest) via the new <portForward> subelement of
><interface>. Here is an example of the config for a network interface
>that uses passt to connect:
>
>    <interface type='user'>
>      <mac address='52:54:00:a8:33:fc'/>
>      <ip address='192.168.221.122' family='ipv4'/>
>      <model type='virtio'/>
>      <backend type='passt' logFile='/tmp/xyzzy.log' upstream='eth0'/>
>      <portForward address='10.0.0.1' proto='tcp' dev='eth0'>
>        <range start='2022' to='22'/>
>        <range start='5000' end='5099' to='1000'/>
>        <range start='5010' end='5029' exclude='yes'/>
>      </portForward>
>      <portForward proto='udp'>
>        <range start='10101'/>
>      </portForward>
>    </interface>
>
>In this case:
>
>* the guest will be offered address 192.168.221.122 for its interface
>  via DHCP
>
>* the passt process will write all log messages to /tmp/xyzzy.log
>
>* routes to the outside for the guest will be derived from the
>  addresses and routes associated with the host interface "eth0".
>
>* incoming tcp port 2022 to the host will be forwarded to port 22
>  on the guest.
>
>* incoming tcp ports 5000-5099 (with the exception of ports 5010-5029)
>  to the host will be forwarded to port 1000-1099 on the guest.
>
>* incoming udp packets on port 10101 will be forwarded (unchanged) to
>  the guest.
>
>Signed-off-by: Laine Stump <laine@redhat.com>
>---
> docs/formatdomain.rst                       |  95 +++++++-
> src/conf/domain_conf.c                      | 242 +++++++++++++++++++-
> src/conf/domain_conf.h                      |  40 ++++
> src/conf/domain_validate.c                  |  32 ++-
> src/conf/virconftypes.h                     |   4 +
> src/libvirt_private.syms                    |   1 +
> tests/qemuxml2xmloutdata/net-user-passt.xml |   1 +
> tests/qemuxml2xmltest.c                     |   1 +
> 8 files changed, 401 insertions(+), 15 deletions(-)
> create mode 120000 tests/qemuxml2xmloutdata/net-user-passt.xml
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

The XML looks reasonable to me. All my comments below are just
nitpicking.

>+static int
>+virDomainNetPortForwardRangesParseXML(virDomainNetPortForward *def,
>+                                      xmlXPathContextPtr ctxt)
>+{
>+    int nRanges;
>+    g_autofree xmlNodePtr *ranges = NULL;
>+    size_t i;
>+
>+    if ((nRanges = virXPathNodeSet("./range",
>+                                   ctxt, &ranges)) <= 0) {

This would fit on one line. Also, the braces are not necessary,
but allowed per our coding style.

>+        return nRanges;
>+    }
>+
>+    def->ranges = g_new0(virDomainNetPortForwardRange *, nRanges);
>+
>+    for (i = 0; i < nRanges; i++) {
>+        g_autofree virDomainNetPortForwardRange *range = NULL;
>+

[...]

>@@ -23274,17 +23435,91 @@ static void
> virDomainNetBackendFormat(virBuffer *buf,
>                           virDomainNetBackend *backend)
> {
>-
>-    if (!(backend->tap || backend->vhost))
>+    if (!(backend->type || backend->tap || backend->vhost
>+          || backend->logFile || backend->upstream)) {

The prevalent style is to put the operator on the preceding line.
Best way to avoid it here is to use virXMLFormatElement.

>         return;
>+    }
>

[...]

>
>+    if (net->type != VIR_DOMAIN_NET_TYPE_USER) {
>+        if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) {
>+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>+                           _("\"<backend type='passt'/>\" can only be used with \"<interface type='user'>\""));

Including XML in the error message can be confusing if the error reaches
users that did not use XML to configure the domain.

>+            return -1;
>+        }
>+    }
>+
>     switch (net->type) {
>     case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>         if (!virDomainNetIsVirtioModel(net)) {

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]