A typical XML representation of the virNWFilterBindingDefPtr struct
looks like this:

  <filterbinding>
    <owner>
      <name>f25arm7</name>
      <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
    </owner>
    <portdev name='vnet1'/>
    <mac address='52:54:00:9d:81:b1'/>
    <filterref filter='clean-traffic'>
      <parameter name='MAC' value='52:54:00:9d:81:b1'/>
    </filterref>
  </filterbinding>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/virnwfilterbindingdef.c              | 196 ++++++++++++++++++
 src/conf/virnwfilterbindingdef.h              |  18 ++
 src/libvirt_private.syms                      |   5 +
 tests/Makefile.am                             |   7 +
 .../filter-vars.xml                           |  11 +
 .../virnwfilterbindingxml2xmldata/simple.xml  |   9 +
 tests/virnwfilterbindingxml2xmltest.c         | 113 ++++++++++
 7 files changed, 359 insertions(+)
 create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml
 create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml
 create mode 100644 tests/virnwfilterbindingxml2xmltest.c

diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c
index c7533d4063..23c040ab05 100644
--- a/src/conf/virnwfilterbindingdef.c
+++ b/src/conf/virnwfilterbindingdef.c
@@ -25,6 +25,7 @@
 #include "virstring.h"
 #include "nwfilter_params.h"
 #include "virnwfilterbindingdef.h"
+#include "viruuid.h"
 
 
 #define VIR_FROM_THIS VIR_FROM_NWFILTER
@@ -81,3 +82,198 @@ virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src)
     virNWFilterBindingDefFree(ret);
     return NULL;
 }
+
+
+static virNWFilterBindingDefPtr
+virNWFilterBindingDefParseXML(xmlXPathContextPtr ctxt)
+{
+    virNWFilterBindingDefPtr ret;
+    char *uuid = NULL;
+    char *mac = NULL;
+    xmlNodePtr node;
+
+    if (VIR_ALLOC(ret) < 0)
+        return NULL;
+
+    ret->portdevname = virXPathString("string(./portdev/@name)", ctxt);
+    if (!ret->portdevname) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("filter binding has no port dev name"));
+        goto cleanup;
+    }
+
+    if (virXPathNode("./linkdev", ctxt)) {
+        ret->linkdevname = virXPathString("string(./linkdev/@name)", ctxt);
+        if (!ret->linkdevname) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           "%s", _("filter binding has no link dev name"));
+            goto cleanup;
+        }
+    }
+
+    ret->ownername = virXPathString("string(./owner/name)", ctxt);
+    if (!ret->ownername) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("filter binding has no owner name"));
+        goto cleanup;
+    }
+
+    uuid = virXPathString("string(./owner/uuid)", ctxt);
+    if (!uuid) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("filter binding has no owner UUID"));
+        goto cleanup;
+    }
+
+    if (virUUIDParse(uuid, ret->owneruuid) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to parse UUID '%s'"), uuid);
+        VIR_FREE(uuid);
+        goto cleanup;
+    }
+    VIR_FREE(uuid);
+
+    mac = virXPathString("string(./mac/@address)", ctxt);
+    if (!mac) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("filter binding has no MAC address"));
+        goto cleanup;
+    }
+
+    if (virMacAddrParse(mac, &ret->mac) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to parse MAC '%s'"), mac);
+        VIR_FREE(mac);
+        goto cleanup;
+    }
+    VIR_FREE(mac);
+
+    ret->filter = virXPathString("string(./filterref/@filter)", ctxt);
+    if (!ret->filter) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("filter binding has no filter reference"));
+        goto cleanup;
+    }
+
+    node = virXPathNode("./filterref", ctxt);
+    if (node &&
+        !(ret->filterparams = virNWFilterParseParamAttributes(node)))
+        goto cleanup;
+
+    return ret;
+
+ cleanup:
+    virNWFilterBindingDefFree(ret);
+    return NULL;
+}
+
+
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseNode(xmlDocPtr xml,
+                               xmlNodePtr root)
+{
+    xmlXPathContextPtr ctxt = NULL;
+    virNWFilterBindingDefPtr def = NULL;
+
+    if (STRNEQ((const char *)root->name, "filterbinding")) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s",
+                       _("unknown root element for nwfilter binding"));
+        goto cleanup;
+    }
+
+    ctxt = xmlXPathNewContext(xml);
+    if (ctxt == NULL) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    ctxt->node = root;
+    def = virNWFilterBindingDefParseXML(ctxt);
+
+ cleanup:
+    xmlXPathFreeContext(ctxt);
+    return def;
+}
+
+
+static virNWFilterBindingDefPtr
+virNWFilterBindingDefParse(const char *xmlStr,
+                           const char *filename)
+{
+    virNWFilterBindingDefPtr def = NULL;
+    xmlDocPtr xml;
+
+    if ((xml = virXMLParse(filename, xmlStr, _("(nwfilterbinding_definition)")))) {
+        def = virNWFilterBindingDefParseNode(xml, xmlDocGetRootElement(xml));
+        xmlFreeDoc(xml);
+    }
+
+    return def;
+}
+
+
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseString(const char *xmlStr)
+{
+    return virNWFilterBindingDefParse(xmlStr, NULL);
+}
+
+
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseFile(const char *filename)
+{
+    return virNWFilterBindingDefParse(NULL, filename);
+}
+
+char *
+virNWFilterBindingDefFormat(const virNWFilterBindingDef *def)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+    if (virNWFilterBindingDefFormatBuf(&buf, def) < 0) {
+        virBufferFreeAndReset(&buf);
+        return NULL;
+    }
+
+    if (virBufferCheckError(&buf) < 0)
+        return NULL;
+
+    return virBufferContentAndReset(&buf);
+}
+
+
+int
+virNWFilterBindingDefFormatBuf(virBufferPtr buf,
+                               const virNWFilterBindingDef *def)
+{
+    char uuid[VIR_UUID_STRING_BUFLEN];
+    char mac[VIR_MAC_STRING_BUFLEN];
+
+    virBufferAddLit(buf, "<filterbinding>\n");
+
+    virBufferAdjustIndent(buf, 2);
+
+    virBufferAddLit(buf, "<owner>\n");
+    virBufferAdjustIndent(buf, 2);
+    virBufferEscapeString(buf, "<name>%s</name>\n", def->ownername);
+    virUUIDFormat(def->owneruuid, uuid);
+    virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid);
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</owner>\n");
+
+    virBufferEscapeString(buf, "<portdev name='%s'/>\n", def->portdevname);
+    if (def->linkdevname)
+        virBufferEscapeString(buf, "<linkdev name='%s'/>\n", def->linkdevname);
+
+    virMacAddrFormat(&def->mac, mac);
+    virBufferAsprintf(buf, "<mac address='%s'/>\n", mac);
+
+    if (virNWFilterFormatParamAttributes(buf, def->filterparams, def->filter) < 0)
+        return -1;
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</filterbinding>\n");
+
+    return 0;
+}
diff --git a/src/conf/virnwfilterbindingdef.h b/src/conf/virnwfilterbindingdef.h
index e3b18af151..af7fab6064 100644
--- a/src/conf/virnwfilterbindingdef.h
+++ b/src/conf/virnwfilterbindingdef.h
@@ -24,6 +24,7 @@
 # include "internal.h"
 # include "virmacaddr.h"
 # include "virhash.h"
+# include "virbuffer.h"
 
 typedef struct _virNWFilterBindingDef virNWFilterBindingDef;
 typedef virNWFilterBindingDef *virNWFilterBindingDefPtr;
@@ -44,4 +45,21 @@ virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding);
 virNWFilterBindingDefPtr
 virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src);
 
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseNode(xmlDocPtr xml,
+                               xmlNodePtr root);
+
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseString(const char *xml);
+
+virNWFilterBindingDefPtr
+virNWFilterBindingDefParseFile(const char *filename);
+
+char *
+virNWFilterBindingDefFormat(const virNWFilterBindingDef *def);
+
+int
+virNWFilterBindingDefFormatBuf(virBufferPtr buf,
+                               const virNWFilterBindingDef *def);
+
 #endif /* VIR_NWFILTER_BINDING_DEF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb754fbfea..03145c70d5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1043,7 +1043,12 @@ virNodeDeviceObjListRemove;
 
 # conf/virnwfilterbindingdef.h
 virNWFilterBindingDefCopy;
+virNWFilterBindingDefFormat;
+virNWFilterBindingDefFormatBuf;
 virNWFilterBindingDefFree;
+virNWFilterBindingDefParseFile;
+virNWFilterBindingDefParseNode;
+virNWFilterBindingDefParseString;
 
 
 # conf/virnwfilterobj.h
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 621480dd0c..036335e770 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -156,6 +156,7 @@ EXTRA_DIST = \
 	virmock.h \
 	virnetdaemondata \
 	virnetdevtestdata \
+	virnwfilterbindingxml2xmldata \
 	virpcitestdata \
 	virscsidata \
 	virsh-uriprecedence \
@@ -352,6 +353,7 @@ test_programs += storagebackendsheepdogtest
 endif WITH_STORAGE_SHEEPDOG
 
 test_programs += nwfilterxml2xmltest
+test_programs += virnwfilterbindingxml2xmltest
 
 if WITH_NWFILTER
 test_programs += nwfilterebiptablestest
@@ -855,6 +857,11 @@ nwfilterxml2xmltest_SOURCES = \
 	testutils.c testutils.h
 nwfilterxml2xmltest_LDADD = $(LDADDS)
 
+virnwfilterbindingxml2xmltest_SOURCES = \
+	virnwfilterbindingxml2xmltest.c \
+	testutils.c testutils.h
+virnwfilterbindingxml2xmltest_LDADD = $(LDADDS)
+
 if WITH_NWFILTER
 nwfilterebiptablestest_SOURCES = \
 	nwfilterebiptablestest.c \
diff --git a/tests/virnwfilterbindingxml2xmldata/filter-vars.xml b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml
new file mode 100644
index 0000000000..dcff9640ce
--- /dev/null
+++ b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml
@@ -0,0 +1,11 @@
+<filterbinding>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <portdev name='vnet0'/>
+  <mac address='52:54:00:7b:35:93'/>
+  <filterref filter='clean-traffic'>
+    <parameter name='MAC' value='52:54:00:7b:35:93'/>
+  </filterref>
+</filterbinding>
diff --git a/tests/virnwfilterbindingxml2xmldata/simple.xml b/tests/virnwfilterbindingxml2xmldata/simple.xml
new file mode 100644
index 0000000000..4577729a3c
--- /dev/null
+++ b/tests/virnwfilterbindingxml2xmldata/simple.xml
@@ -0,0 +1,9 @@
+<filterbinding>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <portdev name='vnet0'/>
+  <mac address='52:54:00:7b:35:93'/>
+  <filterref filter='clean-traffic'/>
+</filterbinding>
diff --git a/tests/virnwfilterbindingxml2xmltest.c b/tests/virnwfilterbindingxml2xmltest.c
new file mode 100644
index 0000000000..96edbdcf59
--- /dev/null
+++ b/tests/virnwfilterbindingxml2xmltest.c
@@ -0,0 +1,113 @@
+/*
+ * virnwfilterbindingxml2xmltest.h: network filter binding XML testing
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <fcntl.h>
+
+#include "internal.h"
+#include "testutils.h"
+#include "virxml.h"
+#include "virnwfilterbindingdef.h"
+#include "testutilsqemu.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+static int
+testCompareXMLToXMLFiles(const char *xml)
+{
+    char *actual = NULL;
+    int ret = -1;
+    virNWFilterBindingDefPtr dev = NULL;
+
+    virResetLastError();
+
+    if (!(dev = virNWFilterBindingDefParseFile(xml))) {
+        goto fail;
+    }
+
+    if (!(actual = virNWFilterBindingDefFormat(dev)))
+        goto fail;
+
+    if (virTestCompareToFile(actual, xml) < 0)
+        goto fail;
+
+    ret = 0;
+
+ fail:
+    VIR_FREE(actual);
+    virNWFilterBindingDefFree(dev);
+    return ret;
+}
+
+typedef struct test_parms {
+    const char *name;
+} test_parms;
+
+static int
+testCompareXMLToXMLHelper(const void *data)
+{
+    int result = -1;
+    const test_parms *tp = data;
+    char *xml = NULL;
+
+    if (virAsprintf(&xml, "%s/virnwfilterbindingxml2xmldata/%s.xml",
+                    abs_srcdir, tp->name) < 0) {
+        goto cleanup;
+    }
+
+    result = testCompareXMLToXMLFiles(xml);
+
+ cleanup:
+    VIR_FREE(xml);
+
+    return result;
+}
+
+static int
+mymain(void)
+{
+    int ret = 0;
+
+#define DO_TEST(NAME) \
+    do { \
+        test_parms tp = { \
+            .name = NAME, \
+        }; \
+        if (virTestRun("NWFilter XML-2-XML " NAME, \
+                       testCompareXMLToXMLHelper, (&tp)) < 0) \
+            ret = -1; \
+    } while (0)
+
+    DO_TEST("simple");
+    DO_TEST("filter-vars");
+
+    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIR_TEST_MAIN(mymain)
-- 
2.17.0

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

On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> A typical XML representation of the virNWFilterBindingDefPtr struct
> looks like this:
> 
>   <filterbinding>
>     <owner>
>       <name>f25arm7</name>
>       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
>     </owner>
>     <portdev name='vnet1'/>
>     <mac address='52:54:00:9d:81:b1'/>
>     <filterref filter='clean-traffic'>
>       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
>     </filterref>
>   </filterbinding>
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/virnwfilterbindingdef.c              | 196 ++++++++++++++++++
>  src/conf/virnwfilterbindingdef.h              |  18 ++
>  src/libvirt_private.syms                      |   5 +
>  tests/Makefile.am                             |   7 +
>  .../filter-vars.xml                           |  11 +
>  .../virnwfilterbindingxml2xmldata/simple.xml  |   9 +
>  tests/virnwfilterbindingxml2xmltest.c         | 113 ++++++++++
>  7 files changed, 359 insertions(+)
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmltest.c
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

FWIW: 1 nit...

> diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c
> index c7533d4063..23c040ab05 100644
> --- a/src/conf/virnwfilterbindingdef.c
> +++ b/src/conf/virnwfilterbindingdef.c

[...]

> +
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseFile(const char *filename)
> +{
> +    return virNWFilterBindingDefParse(NULL, filename);
> +}
> +

2 blank lines

> +char *
> +virNWFilterBindingDefFormat(const virNWFilterBindingDef *def)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (virNWFilterBindingDefFormatBuf(&buf, def) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        return NULL;
> +    }
> +
> +    if (virBufferCheckError(&buf) < 0)
> +        return NULL;
> +
> +    return virBufferContentAndReset(&buf);
> +}

[...]

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

On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> A typical XML representation of the virNWFilterBindingDefPtr struct
> looks like this:
> 
>   <filterbinding>
>     <owner>
>       <name>f25arm7</name>
>       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
>     </owner>
>     <portdev name='vnet1'/>
>     <mac address='52:54:00:9d:81:b1'/>
>     <filterref filter='clean-traffic'>
>       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
>     </filterref>
>   </filterbinding>
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/virnwfilterbindingdef.c              | 196 ++++++++++++++++++
>  src/conf/virnwfilterbindingdef.h              |  18 ++
>  src/libvirt_private.syms                      |   5 +
>  tests/Makefile.am                             |   7 +
>  .../filter-vars.xml                           |  11 +
>  .../virnwfilterbindingxml2xmldata/simple.xml  |   9 +
>  tests/virnwfilterbindingxml2xmltest.c         | 113 ++++++++++
>  7 files changed, 359 insertions(+)
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmltest.c
> 

whoops - forgot the syntax-check things:

[...]

> diff --git a/tests/virnwfilterbindingxml2xmltest.c b/tests/virnwfilterbindingxml2xmltest.c
> new file mode 100644
> index 0000000000..96edbdcf59
> --- /dev/null
> +++ b/tests/virnwfilterbindingxml2xmltest.c
> @@ -0,0 +1,113 @@
> +/*
> + * virnwfilterbindingxml2xmltest.h: network filter binding XML testing

virnwfilterbindingxml2xmltest.c

> + *
> + * Copyright (C) 2018 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +
> +#include <sys/types.h>
> +#include <fcntl.h>
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "virxml.h"
> +#include "virnwfilterbindingdef.h"
> +#include "testutilsqemu.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +static int
> +testCompareXMLToXMLFiles(const char *xml)
> +{
> +    char *actual = NULL;
> +    int ret = -1;
> +    virNWFilterBindingDefPtr dev = NULL;
> +
> +    virResetLastError();
> +
> +    if (!(dev = virNWFilterBindingDefParseFile(xml))) {
> +        goto fail;
> +    }

brackets unnecessary

> +
> +    if (!(actual = virNWFilterBindingDefFormat(dev)))
> +        goto fail;
> +
> +    if (virTestCompareToFile(actual, xml) < 0)
> +        goto fail;
> +
> +    ret = 0;
> +
> + fail:
> +    VIR_FREE(actual);
> +    virNWFilterBindingDefFree(dev);
> +    return ret;
> +}
> +

[...]

R-by applies...

John

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

On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> A typical XML representation of the virNWFilterBindingDefPtr struct
> looks like this:
>
>   <filterbinding>
>     <owner>
>       <name>f25arm7</name>
>       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
>     </owner>
>     <portdev name='vnet1'/>
>     <mac address='52:54:00:9d:81:b1'/>
>     <filterref filter='clean-traffic'>
>       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
>     </filterref>
>   </filterbinding>


I haven't had time to look at this in detail yet, or to really think
about the comment I'm going to make, but I wanted to be sure I said it
right away in case there are any public API implications


By now we all know the horror by which OpenStack's networking creates a
separate bridge device, and connects the guest's tap device to that
bridge so that (in addition to other reasons) there is a place for
libvirt's nwfilter rules to attach (what they *really* want to connect
to is an Open vSwitch, but ipfilter rules aren't in the data path when a
tap device is connected to OVS). This atrocity could be avoided if
nwfilter supported OVN (OVS's ipfilter analog) directly. In order to
support it though, nwfilter would need to know more details about the
network device that it's adding rules for. (because some guests may be
connected to OVS and others may be connected to a standard host bridge
(or nothing at all) we can't just have a system-wide config to decide).

I can't recall if there is a reasonable API to figure out what a tap
device is connected to, but I think such a thing may not exist and, if
so, we'll likely need to send some sort of indicator in the
NWFilterBinding XML. It *might* be simpler / more futureproof if we
included the <virtualport> element that is already in the domain XML
<interface> information - that's currently what we use to decide how to
connect the tap device; hopefully in the future it will continue to
conain everything that's needed (if we think that's inadequate, we could
just go for broke and include the entire <interface>, but that's
probably overkill. (although..... - thinking about the potential case
where some SRIOV network card supported some sort of filtering, if we
sent the entire <interface>, we would know that it was a hostdev...)

I started typing this wondering if the C API might need any change, but
now that I've typed this, I realize it would only require additions to
the XML, which can always be done later, so


   nevermind (kind of) </EmilyLittela>

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/virnwfilterbindingdef.c              | 196 ++++++++++++++++++
>  src/conf/virnwfilterbindingdef.h              |  18 ++
>  src/libvirt_private.syms                      |   5 +
>  tests/Makefile.am                             |   7 +
>  .../filter-vars.xml                           |  11 +
>  .../virnwfilterbindingxml2xmldata/simple.xml  |   9 +
>  tests/virnwfilterbindingxml2xmltest.c         | 113 ++++++++++
>  7 files changed, 359 insertions(+)
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml
>  create mode 100644 tests/virnwfilterbindingxml2xmltest.c
>
> diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c
> index c7533d4063..23c040ab05 100644
> --- a/src/conf/virnwfilterbindingdef.c
> +++ b/src/conf/virnwfilterbindingdef.c
> @@ -25,6 +25,7 @@
>  #include "virstring.h"
>  #include "nwfilter_params.h"
>  #include "virnwfilterbindingdef.h"
> +#include "viruuid.h"
>  
>  
>  #define VIR_FROM_THIS VIR_FROM_NWFILTER
> @@ -81,3 +82,198 @@ virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src)
>      virNWFilterBindingDefFree(ret);
>      return NULL;
>  }
> +
> +
> +static virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseXML(xmlXPathContextPtr ctxt)
> +{
> +    virNWFilterBindingDefPtr ret;
> +    char *uuid = NULL;
> +    char *mac = NULL;
> +    xmlNodePtr node;
> +
> +    if (VIR_ALLOC(ret) < 0)
> +        return NULL;
> +
> +    ret->portdevname = virXPathString("string(./portdev/@name)", ctxt);
> +    if (!ret->portdevname) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("filter binding has no port dev name"));
> +        goto cleanup;
> +    }
> +
> +    if (virXPathNode("./linkdev", ctxt)) {
> +        ret->linkdevname = virXPathString("string(./linkdev/@name)", ctxt);
> +        if (!ret->linkdevname) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("filter binding has no link dev name"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret->ownername = virXPathString("string(./owner/name)", ctxt);
> +    if (!ret->ownername) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("filter binding has no owner name"));
> +        goto cleanup;
> +    }
> +
> +    uuid = virXPathString("string(./owner/uuid)", ctxt);
> +    if (!uuid) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("filter binding has no owner UUID"));
> +        goto cleanup;
> +    }
> +
> +    if (virUUIDParse(uuid, ret->owneruuid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse UUID '%s'"), uuid);
> +        VIR_FREE(uuid);
> +        goto cleanup;
> +    }
> +    VIR_FREE(uuid);
> +
> +    mac = virXPathString("string(./mac/@address)", ctxt);
> +    if (!mac) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("filter binding has no MAC address"));
> +        goto cleanup;
> +    }
> +
> +    if (virMacAddrParse(mac, &ret->mac) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse MAC '%s'"), mac);
> +        VIR_FREE(mac);
> +        goto cleanup;
> +    }
> +    VIR_FREE(mac);
> +
> +    ret->filter = virXPathString("string(./filterref/@filter)", ctxt);
> +    if (!ret->filter) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("filter binding has no filter reference"));
> +        goto cleanup;
> +    }
> +
> +    node = virXPathNode("./filterref", ctxt);
> +    if (node &&
> +        !(ret->filterparams = virNWFilterParseParamAttributes(node)))
> +        goto cleanup;
> +
> +    return ret;
> +
> + cleanup:
> +    virNWFilterBindingDefFree(ret);
> +    return NULL;
> +}
> +
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseNode(xmlDocPtr xml,
> +                               xmlNodePtr root)
> +{
> +    xmlXPathContextPtr ctxt = NULL;
> +    virNWFilterBindingDefPtr def = NULL;
> +
> +    if (STRNEQ((const char *)root->name, "filterbinding")) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       "%s",
> +                       _("unknown root element for nwfilter binding"));
> +        goto cleanup;
> +    }
> +
> +    ctxt = xmlXPathNewContext(xml);
> +    if (ctxt == NULL) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    ctxt->node = root;
> +    def = virNWFilterBindingDefParseXML(ctxt);
> +
> + cleanup:
> +    xmlXPathFreeContext(ctxt);
> +    return def;
> +}
> +
> +
> +static virNWFilterBindingDefPtr
> +virNWFilterBindingDefParse(const char *xmlStr,
> +                           const char *filename)
> +{
> +    virNWFilterBindingDefPtr def = NULL;
> +    xmlDocPtr xml;
> +
> +    if ((xml = virXMLParse(filename, xmlStr, _("(nwfilterbinding_definition)")))) {
> +        def = virNWFilterBindingDefParseNode(xml, xmlDocGetRootElement(xml));
> +        xmlFreeDoc(xml);
> +    }
> +
> +    return def;
> +}
> +
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseString(const char *xmlStr)
> +{
> +    return virNWFilterBindingDefParse(xmlStr, NULL);
> +}
> +
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseFile(const char *filename)
> +{
> +    return virNWFilterBindingDefParse(NULL, filename);
> +}
> +
> +char *
> +virNWFilterBindingDefFormat(const virNWFilterBindingDef *def)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (virNWFilterBindingDefFormatBuf(&buf, def) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        return NULL;
> +    }
> +
> +    if (virBufferCheckError(&buf) < 0)
> +        return NULL;
> +
> +    return virBufferContentAndReset(&buf);
> +}
> +
> +
> +int
> +virNWFilterBindingDefFormatBuf(virBufferPtr buf,
> +                               const virNWFilterBindingDef *def)
> +{
> +    char uuid[VIR_UUID_STRING_BUFLEN];
> +    char mac[VIR_MAC_STRING_BUFLEN];
> +
> +    virBufferAddLit(buf, "<filterbinding>\n");
> +
> +    virBufferAdjustIndent(buf, 2);
> +
> +    virBufferAddLit(buf, "<owner>\n");
> +    virBufferAdjustIndent(buf, 2);
> +    virBufferEscapeString(buf, "<name>%s</name>\n", def->ownername);
> +    virUUIDFormat(def->owneruuid, uuid);
> +    virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid);
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</owner>\n");
> +
> +    virBufferEscapeString(buf, "<portdev name='%s'/>\n", def->portdevname);
> +    if (def->linkdevname)
> +        virBufferEscapeString(buf, "<linkdev name='%s'/>\n", def->linkdevname);
> +
> +    virMacAddrFormat(&def->mac, mac);
> +    virBufferAsprintf(buf, "<mac address='%s'/>\n", mac);
> +
> +    if (virNWFilterFormatParamAttributes(buf, def->filterparams, def->filter) < 0)
> +        return -1;
> +
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</filterbinding>\n");
> +
> +    return 0;
> +}
> diff --git a/src/conf/virnwfilterbindingdef.h b/src/conf/virnwfilterbindingdef.h
> index e3b18af151..af7fab6064 100644
> --- a/src/conf/virnwfilterbindingdef.h
> +++ b/src/conf/virnwfilterbindingdef.h
> @@ -24,6 +24,7 @@
>  # include "internal.h"
>  # include "virmacaddr.h"
>  # include "virhash.h"
> +# include "virbuffer.h"
>  
>  typedef struct _virNWFilterBindingDef virNWFilterBindingDef;
>  typedef virNWFilterBindingDef *virNWFilterBindingDefPtr;
> @@ -44,4 +45,21 @@ virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding);
>  virNWFilterBindingDefPtr
>  virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src);
>  
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseNode(xmlDocPtr xml,
> +                               xmlNodePtr root);
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseString(const char *xml);
> +
> +virNWFilterBindingDefPtr
> +virNWFilterBindingDefParseFile(const char *filename);
> +
> +char *
> +virNWFilterBindingDefFormat(const virNWFilterBindingDef *def);
> +
> +int
> +virNWFilterBindingDefFormatBuf(virBufferPtr buf,
> +                               const virNWFilterBindingDef *def);
> +
>  #endif /* VIR_NWFILTER_BINDING_DEF_H */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fb754fbfea..03145c70d5 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1043,7 +1043,12 @@ virNodeDeviceObjListRemove;
>  
>  # conf/virnwfilterbindingdef.h
>  virNWFilterBindingDefCopy;
> +virNWFilterBindingDefFormat;
> +virNWFilterBindingDefFormatBuf;
>  virNWFilterBindingDefFree;
> +virNWFilterBindingDefParseFile;
> +virNWFilterBindingDefParseNode;
> +virNWFilterBindingDefParseString;
>  
>  
>  # conf/virnwfilterobj.h
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 621480dd0c..036335e770 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -156,6 +156,7 @@ EXTRA_DIST = \
>  	virmock.h \
>  	virnetdaemondata \
>  	virnetdevtestdata \
> +	virnwfilterbindingxml2xmldata \
>  	virpcitestdata \
>  	virscsidata \
>  	virsh-uriprecedence \
> @@ -352,6 +353,7 @@ test_programs += storagebackendsheepdogtest
>  endif WITH_STORAGE_SHEEPDOG
>  
>  test_programs += nwfilterxml2xmltest
> +test_programs += virnwfilterbindingxml2xmltest
>  
>  if WITH_NWFILTER
>  test_programs += nwfilterebiptablestest
> @@ -855,6 +857,11 @@ nwfilterxml2xmltest_SOURCES = \
>  	testutils.c testutils.h
>  nwfilterxml2xmltest_LDADD = $(LDADDS)
>  
> +virnwfilterbindingxml2xmltest_SOURCES = \
> +	virnwfilterbindingxml2xmltest.c \
> +	testutils.c testutils.h
> +virnwfilterbindingxml2xmltest_LDADD = $(LDADDS)
> +
>  if WITH_NWFILTER
>  nwfilterebiptablestest_SOURCES = \
>  	nwfilterebiptablestest.c \
> diff --git a/tests/virnwfilterbindingxml2xmldata/filter-vars.xml b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml
> new file mode 100644
> index 0000000000..dcff9640ce
> --- /dev/null
> +++ b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml
> @@ -0,0 +1,11 @@
> +<filterbinding>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <portdev name='vnet0'/>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <filterref filter='clean-traffic'>
> +    <parameter name='MAC' value='52:54:00:7b:35:93'/>
> +  </filterref>
> +</filterbinding>
> diff --git a/tests/virnwfilterbindingxml2xmldata/simple.xml b/tests/virnwfilterbindingxml2xmldata/simple.xml
> new file mode 100644
> index 0000000000..4577729a3c
> --- /dev/null
> +++ b/tests/virnwfilterbindingxml2xmldata/simple.xml
> @@ -0,0 +1,9 @@
> +<filterbinding>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <portdev name='vnet0'/>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <filterref filter='clean-traffic'/>
> +</filterbinding>
> diff --git a/tests/virnwfilterbindingxml2xmltest.c b/tests/virnwfilterbindingxml2xmltest.c
> new file mode 100644
> index 0000000000..96edbdcf59
> --- /dev/null
> +++ b/tests/virnwfilterbindingxml2xmltest.c
> @@ -0,0 +1,113 @@
> +/*
> + * virnwfilterbindingxml2xmltest.h: network filter binding XML testing
> + *
> + * Copyright (C) 2018 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +
> +#include <sys/types.h>
> +#include <fcntl.h>
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "virxml.h"
> +#include "virnwfilterbindingdef.h"
> +#include "testutilsqemu.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +static int
> +testCompareXMLToXMLFiles(const char *xml)
> +{
> +    char *actual = NULL;
> +    int ret = -1;
> +    virNWFilterBindingDefPtr dev = NULL;
> +
> +    virResetLastError();
> +
> +    if (!(dev = virNWFilterBindingDefParseFile(xml))) {
> +        goto fail;
> +    }
> +
> +    if (!(actual = virNWFilterBindingDefFormat(dev)))
> +        goto fail;
> +
> +    if (virTestCompareToFile(actual, xml) < 0)
> +        goto fail;
> +
> +    ret = 0;
> +
> + fail:
> +    VIR_FREE(actual);
> +    virNWFilterBindingDefFree(dev);
> +    return ret;
> +}
> +
> +typedef struct test_parms {
> +    const char *name;
> +} test_parms;
> +
> +static int
> +testCompareXMLToXMLHelper(const void *data)
> +{
> +    int result = -1;
> +    const test_parms *tp = data;
> +    char *xml = NULL;
> +
> +    if (virAsprintf(&xml, "%s/virnwfilterbindingxml2xmldata/%s.xml",
> +                    abs_srcdir, tp->name) < 0) {
> +        goto cleanup;
> +    }
> +
> +    result = testCompareXMLToXMLFiles(xml);
> +
> + cleanup:
> +    VIR_FREE(xml);
> +
> +    return result;
> +}
> +
> +static int
> +mymain(void)
> +{
> +    int ret = 0;
> +
> +#define DO_TEST(NAME) \
> +    do { \
> +        test_parms tp = { \
> +            .name = NAME, \
> +        }; \
> +        if (virTestRun("NWFilter XML-2-XML " NAME, \
> +                       testCompareXMLToXMLHelper, (&tp)) < 0) \
> +            ret = -1; \
> +    } while (0)
> +
> +    DO_TEST("simple");
> +    DO_TEST("filter-vars");
> +
> +    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +VIR_TEST_MAIN(mymain)


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

On Thu, May 17, 2018 at 01:31:04PM -0400, Laine Stump wrote:
> On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> > A typical XML representation of the virNWFilterBindingDefPtr struct
> > looks like this:
> >
> >   <filterbinding>
> >     <owner>
> >       <name>f25arm7</name>
> >       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
> >     </owner>
> >     <portdev name='vnet1'/>
> >     <mac address='52:54:00:9d:81:b1'/>
> >     <filterref filter='clean-traffic'>
> >       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
> >     </filterref>
> >   </filterbinding>
> 
> 
> I haven't had time to look at this in detail yet, or to really think
> about the comment I'm going to make, but I wanted to be sure I said it
> right away in case there are any public API implications
> 
> 
> By now we all know the horror by which OpenStack's networking creates a
> separate bridge device, and connects the guest's tap device to that
> bridge so that (in addition to other reasons) there is a place for
> libvirt's nwfilter rules to attach (what they *really* want to connect
> to is an Open vSwitch, but ipfilter rules aren't in the data path when a
> tap device is connected to OVS). This atrocity could be avoided if
> nwfilter supported OVN (OVS's ipfilter analog) directly. In order to
> support it though, nwfilter would need to know more details about the
> network device that it's adding rules for. (because some guests may be
> connected to OVS and others may be connected to a standard host bridge
> (or nothing at all) we can't just have a system-wide config to decide).
> 
> I can't recall if there is a reasonable API to figure out what a tap
> device is connected to, but I think such a thing may not exist and, if
> so, we'll likely need to send some sort of indicator in the
> NWFilterBinding XML. It *might* be simpler / more futureproof if we
> included the <virtualport> element that is already in the domain XML
> <interface> information - that's currently what we use to decide how to
> connect the tap device; hopefully in the future it will continue to
> conain everything that's needed (if we think that's inadequate, we could
> just go for broke and include the entire <interface>, but that's
> probably overkill. (although..... - thinking about the potential case
> where some SRIOV network card supported some sort of filtering, if we
> sent the entire <interface>, we would know that it was a hostdev...)
> 
> I started typing this wondering if the C API might need any change, but
> now that I've typed this, I realize it would only require additions to
> the XML, which can always be done later, so

Yes, that's exactly why I ended up using XML for this - originally I
had just used virTypedParameters but felt XML would give us better
future proofing.  Esstentially any part of the domain XML <interface>
that is related to the /backend/ of the device is fair game for us
to include in the filterbinding XML. I just started with the minimal
set of data, rather than try to wire up everything, so it is simpler.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On 05/17/2018 01:37 PM, Daniel P. Berrangé wrote:
> On Thu, May 17, 2018 at 01:31:04PM -0400, Laine Stump wrote:
>> On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
>>> A typical XML representation of the virNWFilterBindingDefPtr struct
>>> looks like this:
>>>
>>>   <filterbinding>
>>>     <owner>
>>>       <name>f25arm7</name>
>>>       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
>>>     </owner>
>>>     <portdev name='vnet1'/>
>>>     <mac address='52:54:00:9d:81:b1'/>
>>>     <filterref filter='clean-traffic'>
>>>       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
>>>     </filterref>
>>>   </filterbinding>
>>
>>
>> I haven't had time to look at this in detail yet, or to really think
>> about the comment I'm going to make, but I wanted to be sure I said it
>> right away in case there are any public API implications
>>
>>
>> By now we all know the horror by which OpenStack's networking creates a
>> separate bridge device, and connects the guest's tap device to that
>> bridge so that (in addition to other reasons) there is a place for
>> libvirt's nwfilter rules to attach (what they *really* want to connect
>> to is an Open vSwitch, but ipfilter rules aren't in the data path when a
>> tap device is connected to OVS). This atrocity could be avoided if
>> nwfilter supported OVN (OVS's ipfilter analog) directly. In order to
>> support it though, nwfilter would need to know more details about the
>> network device that it's adding rules for. (because some guests may be
>> connected to OVS and others may be connected to a standard host bridge
>> (or nothing at all) we can't just have a system-wide config to decide).
>>
>> I can't recall if there is a reasonable API to figure out what a tap
>> device is connected to, but I think such a thing may not exist and, if
>> so, we'll likely need to send some sort of indicator in the
>> NWFilterBinding XML. It *might* be simpler / more futureproof if we
>> included the <virtualport> element that is already in the domain XML
>> <interface> information - that's currently what we use to decide how to
>> connect the tap device; hopefully in the future it will continue to
>> conain everything that's needed (if we think that's inadequate, we could
>> just go for broke and include the entire <interface>, but that's
>> probably overkill. (although..... - thinking about the potential case
>> where some SRIOV network card supported some sort of filtering, if we
>> sent the entire <interface>, we would know that it was a hostdev...)
>>
>> I started typing this wondering if the C API might need any change, but
>> now that I've typed this, I realize it would only require additions to
>> the XML, which can always be done later, so
> 
> Yes, that's exactly why I ended up using XML for this - originally I
> had just used virTypedParameters but felt XML would give us better
> future proofing.  Esstentially any part of the domain XML <interface>
> that is related to the /backend/ of the device is fair game for us
> to include in the filterbinding XML. I just started with the minimal
> set of data, rather than try to wire up everything, so it is simpler.
> 

So in the future , will the <filterbinding> possibly have a "type"
attribute?  Should we go there now or just leave it for the future?

Would it be something that we'd need for one of the API's to come soon.

All this code is mind-numbing.

John

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

On Thu, May 17, 2018 at 05:55:39PM -0400, John Ferlan wrote:
> 
> 
> On 05/17/2018 01:37 PM, Daniel P. Berrangé wrote:
> > On Thu, May 17, 2018 at 01:31:04PM -0400, Laine Stump wrote:
> >> On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> >>> A typical XML representation of the virNWFilterBindingDefPtr struct
> >>> looks like this:
> >>>
> >>>   <filterbinding>
> >>>     <owner>
> >>>       <name>f25arm7</name>
> >>>       <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
> >>>     </owner>
> >>>     <portdev name='vnet1'/>
> >>>     <mac address='52:54:00:9d:81:b1'/>
> >>>     <filterref filter='clean-traffic'>
> >>>       <parameter name='MAC' value='52:54:00:9d:81:b1'/>
> >>>     </filterref>
> >>>   </filterbinding>
> >>
> >>
> >> I haven't had time to look at this in detail yet, or to really think
> >> about the comment I'm going to make, but I wanted to be sure I said it
> >> right away in case there are any public API implications
> >>
> >>
> >> By now we all know the horror by which OpenStack's networking creates a
> >> separate bridge device, and connects the guest's tap device to that
> >> bridge so that (in addition to other reasons) there is a place for
> >> libvirt's nwfilter rules to attach (what they *really* want to connect
> >> to is an Open vSwitch, but ipfilter rules aren't in the data path when a
> >> tap device is connected to OVS). This atrocity could be avoided if
> >> nwfilter supported OVN (OVS's ipfilter analog) directly. In order to
> >> support it though, nwfilter would need to know more details about the
> >> network device that it's adding rules for. (because some guests may be
> >> connected to OVS and others may be connected to a standard host bridge
> >> (or nothing at all) we can't just have a system-wide config to decide).
> >>
> >> I can't recall if there is a reasonable API to figure out what a tap
> >> device is connected to, but I think such a thing may not exist and, if
> >> so, we'll likely need to send some sort of indicator in the
> >> NWFilterBinding XML. It *might* be simpler / more futureproof if we
> >> included the <virtualport> element that is already in the domain XML
> >> <interface> information - that's currently what we use to decide how to
> >> connect the tap device; hopefully in the future it will continue to
> >> conain everything that's needed (if we think that's inadequate, we could
> >> just go for broke and include the entire <interface>, but that's
> >> probably overkill. (although..... - thinking about the potential case
> >> where some SRIOV network card supported some sort of filtering, if we
> >> sent the entire <interface>, we would know that it was a hostdev...)
> >>
> >> I started typing this wondering if the C API might need any change, but
> >> now that I've typed this, I realize it would only require additions to
> >> the XML, which can always be done later, so
> > 
> > Yes, that's exactly why I ended up using XML for this - originally I
> > had just used virTypedParameters but felt XML would give us better
> > future proofing.  Esstentially any part of the domain XML <interface>
> > that is related to the /backend/ of the device is fair game for us
> > to include in the filterbinding XML. I just started with the minimal
> > set of data, rather than try to wire up everything, so it is simpler.
> > 
> 
> So in the future , will the <filterbinding> possibly have a "type"
> attribute?  Should we go there now or just leave it for the future?

I didn't want to add 'type' to this, as we don't neccessarily care
about the device type. eg we might add a "type" that specifies the
firewall engine type instead.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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