[libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Daniel P. Berrangé posted 24 patches 14 weeks ago

[libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Daniel P. Berrangé 14 weeks ago
Introduce a virNetworkPortDefPtr struct to represent the data associated
with a virtual network port. Add APIs for parsing/formatting XML docs
with the data.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/docs.html.in                             |   1 +
 docs/formatnetworkport.html.in                | 212 ++++++++
 docs/schemas/networkport.rng                  | 165 ++++++
 src/conf/Makefile.inc.am                      |   2 +
 src/conf/virnetworkportdef.c                  | 509 ++++++++++++++++++
 src/conf/virnetworkportdef.h                  | 113 ++++
 src/libvirt_private.syms                      |  10 +
 tests/Makefile.am                             |   7 +
 .../plug-bridge-mactbl.xml                    |   9 +
 .../virnetworkportxml2xmldata/plug-bridge.xml |  15 +
 .../virnetworkportxml2xmldata/plug-direct.xml |  12 +
 .../plug-hostdev-pci.xml                      |  12 +
 .../plug-network.xml                          |  16 +
 tests/virnetworkportxml2xmldata/plug-none.xml |   8 +
 tests/virnetworkportxml2xmltest.c             | 104 ++++
 tests/virschematest.c                         |   1 +
 16 files changed, 1196 insertions(+)
 create mode 100644 docs/formatnetworkport.html.in
 create mode 100644 docs/schemas/networkport.rng
 create mode 100644 src/conf/virnetworkportdef.c
 create mode 100644 src/conf/virnetworkportdef.h
 create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
 create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge.xml
 create mode 100644 tests/virnetworkportxml2xmldata/plug-direct.xml
 create mode 100644 tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
 create mode 100644 tests/virnetworkportxml2xmldata/plug-network.xml
 create mode 100644 tests/virnetworkportxml2xmldata/plug-none.xml
 create mode 100644 tests/virnetworkportxml2xmltest.c

diff --git a/docs/docs.html.in b/docs/docs.html.in
index d0ff844d0c..c8674e1457 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -72,6 +72,7 @@
         <dd>Description of the XML schemas for
           <a href="formatdomain.html">domains</a>,
           <a href="formatnetwork.html">networks</a>,
+          <a href="formatnetworkport.html">network ports</a>,
           <a href="formatnwfilter.html">network filtering</a>,
           <a href="formatstorage.html">storage</a>,
           <a href="formatstorageencryption.html">storage encryption</a>,
diff --git a/docs/formatnetworkport.html.in b/docs/formatnetworkport.html.in
new file mode 100644
index 0000000000..0211518a6a
--- /dev/null
+++ b/docs/formatnetworkport.html.in
@@ -0,0 +1,212 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html>
+<html xmlns="http://www.w3.org/1999/xhtml">
+  <body>
+    <h1>Network XML format</h1>
+
+    <ul id="toc">
+    </ul>
+
+    <p>
+      This page provides an introduction to the network port XML format.
+      This stores information about the connection between an virtual
+      interface on a virtual domain's, and the virtual network it is
+      attached to.
+    </p>
+
+    <h2><a id="elements">Element and attribute overview</a></h2>
+
+    <p>
+      The root element required for all virtual network ports is
+      named <code>networkport</code> and has no configurable attributes
+      The network port XML format is available <span class="since">since
+      5.4.0</span>
+    </p>
+
+    <h3><a id="elementsMetadata">General metadata</a></h3>
+
+    <p>
+      The first elements provide basic metadata about the virtual
+      network port.
+    </p>
+
+    <pre>
+&lt;networkport
+  &lt;uuid&gt;7ae63b5f-fe96-4af0-a7c3-da04ba1b3f54&lt;/uuid&gt;
+  &lt;owner&gt;
+    &lt;uuid&gt;06578fc1-c686-46fa-bc2c-220893b466a6&lt;/uuid&gt;
+    &lt;name&gt;myguest&lt;name&gt;
+  &lt;/owner&gt;
+  &lt;group&gt;webfront&lt;group&gt;
+  &lt;mac address='52:54:0:7b:35:93'/&gt;
+  ...</pre>
+
+    <dl>
+      <dt><code>uuid</code></dt>
+      <dd>The content of the <code>uuid</code> element provides
+        a globally unique identifier for the virtual network port.
+        The format must be RFC 4122 compliant, eg <code>3e3fce45-4f53-4fa7-bb32-11f34168b82b</code>.
+        If omitted when defining/creating a new network port, a random
+        UUID is generated.</dd>
+      <dd>The <code>owner</code> node records the domain object that
+        is the owner of the network port. It contains two child nodes:
+        <dl>
+          <dt><code>uuid</code></dt>
+          <dd>The content of the <code>uuid</code> element provides
+            a globally unique identifier for the virtual domain.</dd>
+          <dt><code>name</code></dt>
+          <dd>The unique name of the virtual domain</dd>
+        </dl>
+      </dd>
+      <dt><code>group</code></dt>
+      <dd>The port group in the virtual network to which the
+        port belongs. Can be omitted if no port groups are
+        defined on the network.</dd>
+      <dt><code>mac</code></dt>
+      <dd>The <code>address</code> attribute provides the MAC
+        address of the virtual port that will be see by the
+        guest. The MAC address must not start with 0xFE as this
+        byte is reserved for use on the host side of the port.
+      </dd>
+    </dl>
+
+    <h3><a id="elementsCommon">Common elements</a></h3>
+
+    <p>
+      The following elements are common to one of more of the plug
+      types listed later
+    </p>
+
+    <pre>
+  ...
+  &lt;bandwidth&gt;
+    &lt;inbound average='1000' peak='5000' floor='200' burst='1024'/&gt;
+    &lt;outbound average='128' peak='256' burst='256'/&gt;
+  &lt;/bandwidth&gt;
+  &lt;rxfilters trustGuest='yes'/&gt;
+  &lt;virtualport type='802.1Qbg'&gt;
+    &lt;parameters managerid='11' typeid='1193047' typeidversion='2'/&gt;
+  &lt;/virtualport&gt;
+  ...</pre>
+
+    <dl>
+      <dt><code>bandwidth</code></dt>
+      <dd>This part of the network port XML provides setting quality of service.
+        Incoming and outgoing traffic can be shaped independently.
+        The <code>bandwidth</code> element and its child elements are described
+        in the <a href="formatnetwork.html#elementQoS">QoS</a> section of
+        the Network XML. In addition the <code>classID</code> attribute may
+        exist provide the ID of the traffic shaping class that is active.
+      </dd>
+      <dt><code>rxfilters</code></dt>
+      <dd>The <code>rxfilters</code> element property
+        <code>trustGuestRxFilters</code> provides the
+        capability for the host to detect and trust reports from the
+        guest regarding changes to the interface mac address and receive
+        filters by setting the attribute to <code>yes</code>. The default
+        setting for the attribute is <code>no</code> for security
+        reasons and support depends on the guest network device model as
+        well as the type of connection on the host - currently it is
+        only supported for the virtio device model and for macvtap
+        connections on the host.
+      </dd>
+      <dt><code>virtualport</code></dt>
+      <dd>The <code>virtualport</code> element describes metadata that
+        needs to be provided to the underlying network subsystem. It
+        is described in the domain XML
+        <a href="formatdomain.html#elementsNICS">interface documentation</a>.
+      </dd>
+    </dl>
+
+
+    <h3><a id="elementsPlug">Plugs</a></h3>
+
+    <p>
+      The <code>plug</code> element has varying content depending
+      on the value of the <code>type</code> attribute.
+    </p>
+
+    <h4><a id="elementsPlugNetwork">Network</a></h4>
+
+    <p>
+      The <code>network</code> plug type refers to a managed virtual
+      network plug that is based on a traditional software bridge
+      device privately managed by libvirt.
+    </p>
+
+    <pre>
+  ...
+  &lt;plug type='network' bridge='virbr0'&gt;
+  ...</pre>
+
+    <p>
+      The <code>bridge</code> attribute provides the name of the
+      privately managed bridge device associated with the virtual
+      network.
+    </p>
+
+    <h4><a id="elementsPlugNetwork">Bridge</a></h4>
+
+    <p>
+      The <code>bridge</code> plug type refers to an externally
+      managed traditional software bridge.
+    </p>
+
+    <pre>
+  ...
+  &lt;plug type='bridge' bridge='br2'&gt;
+  ...</pre>
+
+    <p>
+      The <code>bridge</code> attribute provides the name of the
+      externally managed bridge device associated with the virtual
+      network.
+    </p>
+
+    <h4><a id="elementsPlugNetwork">Direct</a></h4>
+
+    <p>
+      The <code>direct</code> plug type refers to an connection
+      directly to a physical network interface.
+    </p>
+
+    <pre>
+  ...
+  &lt;plug type='direct' dev='ens3' mode='vepa'/&gt;
+  ...</pre>
+
+    <p>
+      The <code>dev</code> attribute provides the name of the
+      physical network interface to which the port will be
+      connected. The <code>mode</code> attribute describes
+      how the connection will be setup and takes the same
+      values described in the
+      <a href="formatdomain.html#elementsNICSDirect">domain XML</a>.
+    </p>
+
+    <h4><a id="elementsPlugNetwork">Host PCI</a></h4>
+
+    <p>
+      The <code>hostdev-pci</code> plug type refers to the
+      passthrough of a physical PCI device rather than emulation.
+    </p>
+
+    <pre>
+  ...
+  &lt;plug type='hostdev-pci' managed='yes'&gt;
+    &lt;driver name='vfio'/&gt;
+    &lt;address domain='0x0001' bus='0x02' slot='0x03' function='0x4'/&gt;
+  &lt;/plug&gt;
+  ...</pre>
+
+    <p>
+      The <code>managed</code> attribute indicates who is responsible for
+      managing the PCI device in the host. When set to the value <code>yes</code>
+      libvirt is responsible for automatically detaching the device from host
+      drivers and resetting it if needed. If the value is <code>no</code>,
+      some other party must ensure the device is not attached to any
+      host drivers.
+    </p>
+
+  </body>
+</html>
diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng
new file mode 100644
index 0000000000..8192f6efc4
--- /dev/null
+++ b/docs/schemas/networkport.rng
@@ -0,0 +1,165 @@
+<?xml version="1.0"?>
+<!-- A Relax NG schema for the libvirt network port XML format -->
+<grammar xmlns="http://relaxng.org/ns/structure/1.0"
+         datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes">
+  <include href='basictypes.rng'/>
+  <include href='networkcommon.rng'/>
+
+  <start>
+    <ref name="networkport"/>
+  </start>
+
+  <define name="networkport">
+    <element name="networkport">
+      <interleave>
+        <element name="uuid">
+          <ref name="UUID"/>
+        </element>
+        <ref name="owner"/>
+        <ref name="mac"/>
+        <optional>
+          <ref name="group"/>
+        </optional>
+        <optional>
+          <ref name="class"/>
+        </optional>
+        <optional>
+          <ref name="rxfilters"/>
+        </optional>
+        <optional>
+          <ref name="virtualPortProfile"/>
+        </optional>
+        <optional>
+          <ref name="bandwidth"/>
+        </optional>
+        <optional>
+          <ref name="plug"/>
+        </optional>
+      </interleave>
+    </element>
+  </define>
+
+  <define name="owner">
+    <element name="owner">
+      <element name="name">
+        <text/>
+      </element>
+      <element name="uuid">
+        <ref name="UUID"/>
+      </element>
+    </element>
+  </define>
+
+  <define name="mac">
+    <element name="mac">
+      <attribute name="address">
+        <ref name="uniMacAddr"/>
+      </attribute>
+      <empty/>
+    </element>
+  </define>
+
+  <define name="group">
+    <element name="group">
+      <ref name="deviceName"/>
+    </element>
+  </define>
+
+  <define name="class">
+    <element name="class">
+      <attribute name="id">
+        <ref name="positiveInteger"/>
+      </attribute>
+    </element>
+  </define>
+
+  <define name="rxfilters">
+    <element name="rxfilters">
+      <attribute name="trustGuest">
+        <ref name="virYesNo"/>
+      </attribute>
+    </element>
+  </define>
+
+  <define name="plug">
+    <element name="plug">
+      <choice>
+        <ref name="plugnetwork"/>
+        <ref name="plugbridge"/>
+        <ref name="plugdirect"/>
+        <ref name="plughostdevpci"/>
+      </choice>
+    </element>
+  </define>
+
+  <define name="plugnetwork">
+    <attribute name="type">
+      <value>network</value>
+    </attribute>
+    <attribute name="bridge">
+      <ref name="deviceName"/>
+    </attribute>
+    <optional>
+      <attribute name="macTableManager">
+        <ref name="macTableManager"/>
+      </attribute>
+    </optional>
+  </define>
+
+  <define name="plugbridge">
+    <attribute name="type">
+      <value>bridge</value>
+    </attribute>
+    <attribute name="bridge">
+      <ref name="deviceName"/>
+    </attribute>
+    <optional>
+      <attribute name="macTableManager">
+        <ref name="macTableManager"/>
+      </attribute>
+    </optional>
+  </define>
+
+  <define name="plugdirect">
+    <attribute name="type">
+      <value>direct</value>
+    </attribute>
+    <attribute name="dev">
+      <ref name="deviceName"/>
+    </attribute>
+    <attribute name="mode">
+      <choice>
+        <value>bridge</value>
+        <value>passthrough</value>
+        <value>private</value>
+        <value>vepa</value>
+      </choice>
+    </attribute>
+  </define>
+
+  <define name="plughostdevpci">
+    <attribute name="type">
+      <value>hostdev-pci</value>
+    </attribute>
+    <optional>
+      <attribute name="managed">
+        <ref name="virYesNo"/>
+      </attribute>
+    </optional>
+    <optional>
+      <element name="driver">
+        <attribute name="name">
+          <choice>
+            <value>kvm</value>
+            <value>vfio</value>
+          </choice>
+        </attribute>
+        <empty/>
+      </element>
+    </optional>
+    <element name='address'>
+      <ref name="pciaddress"/>
+    </element>
+  </define>
+
+</grammar>
diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am
index 3e9fdd1aea..6b52ba674b 100644
--- a/src/conf/Makefile.inc.am
+++ b/src/conf/Makefile.inc.am
@@ -7,6 +7,8 @@ NETDEV_CONF_SOURCES = \
 	conf/netdev_vport_profile_conf.c \
 	conf/netdev_vlan_conf.h \
 	conf/netdev_vlan_conf.c \
+	conf/virnetworkportdef.h \
+	conf/virnetworkportdef.c \
 	$(NULL)
 
 DOMAIN_CONF_SOURCES = \
diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c
new file mode 100644
index 0000000000..6c896968ce
--- /dev/null
+++ b/src/conf/virnetworkportdef.c
@@ -0,0 +1,509 @@
+/*
+ * virnetworkportdef.c: network port XML processing
+ *
+ * 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 "viralloc.h"
+#include "virerror.h"
+#include "virstring.h"
+#include "virfile.h"
+#include "virnetworkportdef.h"
+#include "network_conf.h"
+
+#define VIR_FROM_THIS VIR_FROM_NETWORK
+
+VIR_ENUM_IMPL(virNetworkPortPlug,
+              VIR_NETWORK_PORT_PLUG_TYPE_LAST,
+              "none", "network", "bridge", "direct", "hostdev-pci");
+
+void
+virNetworkPortDefFree(virNetworkPortDefPtr def)
+{
+    if (!def)
+        return;
+
+    VIR_FREE(def->ownername);
+    VIR_FREE(def->group);
+
+    virNetDevBandwidthFree(def->bandwidth);
+    virNetDevVlanClear(&def->vlan);
+    VIR_FREE(def->virtPortProfile);
+
+    switch ((virNetworkPortPlugType)def->plugtype) {
+    case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
+    case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
+        VIR_FREE(def->plug.bridge.brname);
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
+        VIR_FREE(def->plug.direct.linkdev);
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
+    default:
+        break;
+    }
+
+    VIR_FREE(def);
+}
+
+
+
+static virNetworkPortDefPtr
+virNetworkPortDefParseXML(xmlXPathContextPtr ctxt)
+{
+    virNetworkPortDefPtr def;
+    char *uuid = NULL;
+    xmlNodePtr virtPortNode;
+    xmlNodePtr vlanNode;
+    xmlNodePtr bandwidthNode;
+    xmlNodePtr addressNode;
+    char *trustGuestRxFilters = NULL;
+    char *mac = NULL;
+    char *macmgr = NULL;
+    char *mode = NULL;
+    char *plugtype = NULL;
+    char *managed = NULL;
+    char *driver = NULL;
+    char *class_id = NULL;
+
+    if (VIR_ALLOC(def) < 0)
+        return NULL;
+
+    uuid = virXPathString("string(./uuid)", ctxt);
+    if (!uuid) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("network port has no uuid"));
+        goto error;
+    }
+    if (virUUIDParse(uuid, def->uuid) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to parse UUID '%s'"), uuid);
+        goto error;
+    }
+
+    def->ownername = virXPathString("string(./owner/name)", ctxt);
+    if (!def->ownername) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("network port has no owner name"));
+        goto error;
+    }
+
+    VIR_FREE(uuid);
+    uuid = virXPathString("string(./owner/uuid)", ctxt);
+    if (!uuid) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("network port has no owner UUID"));
+        goto error;
+    }
+
+    if (virUUIDParse(uuid, def->owneruuid) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to parse UUID '%s'"), uuid);
+        goto error;
+    }
+
+    def->group = virXPathString("string(./group)", ctxt);
+
+    virtPortNode = virXPathNode("./virtualport", ctxt);
+    if (virtPortNode &&
+        (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) {
+        goto error;
+    }
+
+    mac = virXPathString("string(./mac/@address)", ctxt);
+    if (!mac) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("network port has no mac"));
+        goto error;
+    }
+    if (virMacAddrParse(mac, &def->mac) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unable to parse MAC '%s'"), mac);
+        goto error;
+    }
+
+    bandwidthNode = virXPathNode("./bandwidth", ctxt);
+    /*
+     * We don't know if the port will allow the "floor" param or
+     * not at this stage, so we must just tell virNetDevBandwidthParse
+     * to allow it regardless. Any bad config must be reported at
+     * time of use instead.
+     */
+    if (bandwidthNode &&
+        virNetDevBandwidthParse(&def->bandwidth, &def->class_id,
+                                bandwidthNode, true) < 0)
+        goto error;
+
+    vlanNode = virXPathNode("./vlan", ctxt);
+    if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0)
+        goto error;
+
+
+    trustGuestRxFilters
+        = virXPathString("string(./rxfilters/@trustGuest)", ctxt);
+    if (trustGuestRxFilters) {
+        if ((def->trustGuestRxFilters
+             = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Invalid guest rx filters trust setting '%s' "),
+                           trustGuestRxFilters);
+            goto error;
+        }
+    }
+
+    plugtype = virXPathString("string(./plug/@type)", ctxt);
+
+    if (plugtype &&
+        (def->plugtype = virNetworkPortPlugTypeFromString(plugtype)) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid network prt plug type '%s'"), plugtype);
+    }
+
+    switch (def->plugtype) {
+    case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
+    case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
+        if (!(def->plug.bridge.brname = virXPathString("string(./plug/@bridge)", ctxt))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Missing network port bridge name"));
+            goto error;
+        }
+        macmgr = virXPathString("string(./plug/@macTableManager)", ctxt);
+        if (macmgr &&
+            (def->plug.bridge.macTableManager =
+             virNetworkBridgeMACTableManagerTypeFromString(macmgr)) <= 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Invalid macTableManager setting '%s' "
+                             "in network port"), macmgr);
+            goto error;
+        }
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
+        if (!(def->plug.direct.linkdev = virXPathString("string(./plug/@dev)", ctxt))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Missing network port link device name"));
+            goto error;
+        }
+        mode = virXPathString("string(./plug/@mode)", ctxt);
+        if (mode &&
+            (def->plug.direct.mode =
+             virNetDevMacVLanModeTypeFromString(mode)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Invalid mode setting '%s' in network port"), mode);
+            goto error;
+        }
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
+        managed = virXPathString("string(./plug/@managed)", ctxt);
+        if (managed &&
+            (def->plug.hostdevpci.managed =
+             virTristateBoolTypeFromString(managed)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Invalid managed setting '%s' in network port"), mode);
+            goto error;
+        }
+        driver = virXPathString("string(./plug/driver/@name)", ctxt);
+        if (driver &&
+            (def->plug.hostdevpci.driver =
+             virNetworkForwardDriverNameTypeFromString(driver)) <= 0) {
+              virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Missing network port driver name"));
+            goto error;
+        }
+        if (!(addressNode = virXPathNode("./plug/address", ctxt))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("Missing network port PCI address"));
+            goto error;
+        }
+
+        if (virPCIDeviceAddressParseXML(addressNode, &def->plug.hostdevpci.addr) < 0)
+            goto error;
+        break;
+
+    case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
+    default:
+        virReportEnumRangeError(virNetworkPortPlugType, def->plugtype);
+        goto error;
+    }
+
+ cleanup:
+    VIR_FREE(class_id);
+    VIR_FREE(uuid);
+    VIR_FREE(plugtype);
+    VIR_FREE(mac);
+    VIR_FREE(mode);
+    VIR_FREE(macmgr);
+    VIR_FREE(driver);
+    VIR_FREE(managed);
+    return def;
+
+ error:
+    virNetworkPortDefFree(def);
+    def = NULL;
+    goto cleanup;
+}
+
+
+virNetworkPortDefPtr
+virNetworkPortDefParseNode(xmlDocPtr xml,
+                           xmlNodePtr root)
+{
+    xmlXPathContextPtr ctxt = NULL;
+    virNetworkPortDefPtr def = NULL;
+
+    if (STRNEQ((const char *)root->name, "networkport")) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s",
+                       _("unknown root element for network port"));
+        goto cleanup;
+    }
+
+    ctxt = xmlXPathNewContext(xml);
+    if (ctxt == NULL) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    ctxt->node = root;
+    def = virNetworkPortDefParseXML(ctxt);
+
+ cleanup:
+    xmlXPathFreeContext(ctxt);
+    return def;
+}
+
+
+static virNetworkPortDefPtr
+virNetworkPortDefParse(const char *xmlStr,
+                       const char *filename)
+{
+    virNetworkPortDefPtr def = NULL;
+    xmlDocPtr xml;
+
+    if ((xml = virXMLParse(filename, xmlStr, _("(networkport_definition)")))) {
+        def = virNetworkPortDefParseNode(xml, xmlDocGetRootElement(xml));
+        xmlFreeDoc(xml);
+    }
+
+    return def;
+}
+
+
+virNetworkPortDefPtr
+virNetworkPortDefParseString(const char *xmlStr)
+{
+    return virNetworkPortDefParse(xmlStr, NULL);
+}
+
+
+virNetworkPortDefPtr
+virNetworkPortDefParseFile(const char *filename)
+{
+    return virNetworkPortDefParse(NULL, filename);
+}
+
+
+char *
+virNetworkPortDefFormat(const virNetworkPortDef *def)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+    if (virNetworkPortDefFormatBuf(&buf, def) < 0) {
+        virBufferFreeAndReset(&buf);
+        return NULL;
+    }
+
+    if (virBufferCheckError(&buf) < 0)
+        return NULL;
+
+    return virBufferContentAndReset(&buf);
+}
+
+
+int
+virNetworkPortDefFormatBuf(virBufferPtr buf,
+                           const virNetworkPortDef *def)
+{
+    char uuid[VIR_UUID_STRING_BUFLEN];
+    char macaddr[VIR_MAC_STRING_BUFLEN];
+
+    virBufferAddLit(buf, "<networkport>\n");
+
+    virBufferAdjustIndent(buf, 2);
+
+    virUUIDFormat(def->uuid, uuid);
+    virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid);
+
+    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");
+
+    if (def->group)
+        virBufferEscapeString(buf, "<group>%s</group>\n", def->group);
+
+    virMacAddrFormat(&def->mac, macaddr);
+    virBufferAsprintf(buf, "<mac address='%s'/>\n", macaddr);
+
+    if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
+        return -1;
+    if (def->bandwidth)
+        virNetDevBandwidthFormat(def->bandwidth, def->class_id, buf);
+    if (virNetDevVlanFormat(&def->vlan, buf) < 0)
+        return -1;
+    if (def->trustGuestRxFilters)
+        virBufferAsprintf(buf, "<rxfilters trustGuest='%s'/>\n",
+                          virTristateBoolTypeToString(def->trustGuestRxFilters));
+
+    if (def->plugtype != VIR_NETWORK_PORT_PLUG_TYPE_NONE) {
+        virBufferAsprintf(buf, "<plug type='%s'",
+                          virNetworkPortPlugTypeToString(def->plugtype));
+
+        switch (def->plugtype) {
+        case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
+            break;
+
+        case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
+        case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
+            virBufferEscapeString(buf, " bridge='%s'", def->plug.bridge.brname);
+            if (def->plug.bridge.macTableManager)
+                virBufferAsprintf(buf, " macTableManager='%s'",
+                                  virNetworkBridgeMACTableManagerTypeToString(
+                                      def->plug.bridge.macTableManager));
+            virBufferAddLit(buf, "/>\n");
+            break;
+
+        case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
+            virBufferEscapeString(buf, " dev='%s'", def->plug.direct.linkdev);
+            virBufferAsprintf(buf, " mode='%s'",
+                              virNetDevMacVLanModeTypeToString(
+                                  def->plug.direct.mode));
+            virBufferAddLit(buf, "/>\n");
+            break;
+
+        case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
+            virBufferAsprintf(buf, " managed='%s'>\n",
+                              def->plug.hostdevpci.managed ? "yes" : "no");
+            virBufferAdjustIndent(buf, 2);
+            if (def->plug.hostdevpci.driver)
+                virBufferEscapeString(buf, "<driver name='%s'/>\n",
+                                      virNetworkForwardDriverNameTypeToString(
+                                          def->plug.hostdevpci.driver));
+
+            virPCIDeviceAddressFormat(buf, def->plug.hostdevpci.addr, false);
+            virBufferAdjustIndent(buf, -2);
+            virBufferAddLit(buf, "</plug>\n");
+            break;
+
+        case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
+        default:
+            virReportEnumRangeError(virNetworkPortPlugType, def->plugtype);
+            return -1;
+        }
+    }
+
+
+    virBufferAdjustIndent(buf, -2);
+    virBufferAddLit(buf, "</networkport>\n");
+
+    return 0;
+}
+
+
+static char *
+virNetworkPortDefConfigFile(const char *dir,
+                            const char *name)
+{
+    char *ret = NULL;
+
+    ignore_value(virAsprintf(&ret, "%s/%s.xml", dir, name));
+    return ret;
+}
+
+
+int
+virNetworkPortDefSaveStatus(virNetworkPortDef *def,
+                            const char *dir)
+{
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+    char *path;
+    char *xml = NULL;
+    int ret = -1;
+
+    virUUIDFormat(def->uuid, uuidstr);
+
+    if (virFileMakePath(dir) < 0)
+        goto cleanup;
+
+    if (!(path = virNetworkPortDefConfigFile(dir, uuidstr)))
+        goto cleanup;
+
+    if (!(xml = virNetworkPortDefFormat(def)))
+        goto cleanup;
+
+    if (virXMLSaveFile(path, uuidstr, "net-port-create", xml) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(xml);
+    VIR_FREE(path);
+    return ret;
+}
+
+
+int
+virNetworkPortDefDeleteStatus(virNetworkPortDef *def,
+                              const char *dir)
+{
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+    char *path;
+    int ret = -1;
+
+    virUUIDFormat(def->uuid, uuidstr);
+
+    if (!(path = virNetworkPortDefConfigFile(dir, uuidstr)))
+        goto cleanup;
+
+    if (unlink(path) < 0 && errno != ENOENT) {
+        virReportSystemError(errno,
+                             _("Unable to delete %s"), path);
+        goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(path);
+    return ret;
+}
diff --git a/src/conf/virnetworkportdef.h b/src/conf/virnetworkportdef.h
new file mode 100644
index 0000000000..94b2fabf6b
--- /dev/null
+++ b/src/conf/virnetworkportdef.h
@@ -0,0 +1,113 @@
+/*
+ * virnetworkportdef.h: network port XML processing
+ *
+ * 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/>.
+ *
+ */
+
+#ifndef LIBVIRT_VIRNETWORKPORTDEF_H
+# define LIBVIRT_VIRNETWORKPORTDEF_H
+
+# include "internal.h"
+# include "viruuid.h"
+# include "virnetdevvlan.h"
+# include "virnetdevvportprofile.h"
+# include "virnetdevbandwidth.h"
+# include "virpci.h"
+# include "virxml.h"
+# include "netdev_vport_profile_conf.h"
+# include "netdev_bandwidth_conf.h"
+# include "netdev_vlan_conf.h"
+
+typedef struct _virNetworkPortDef virNetworkPortDef;
+typedef virNetworkPortDef *virNetworkPortDefPtr;
+
+typedef enum {
+    VIR_NETWORK_PORT_PLUG_TYPE_NONE,
+    VIR_NETWORK_PORT_PLUG_TYPE_NETWORK,
+    VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE,
+    VIR_NETWORK_PORT_PLUG_TYPE_DIRECT,
+    VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI,
+
+    VIR_NETWORK_PORT_PLUG_TYPE_LAST,
+} virNetworkPortPlugType;
+
+VIR_ENUM_DECL(virNetworkPortPlug);
+
+struct _virNetworkPortDef {
+    unsigned char uuid[VIR_UUID_BUFLEN];
+    char *ownername;
+    unsigned char owneruuid[VIR_UUID_BUFLEN];
+
+    char *group;
+    virMacAddr mac;
+
+    virNetDevVPortProfilePtr virtPortProfile;
+    virNetDevBandwidthPtr bandwidth;
+    unsigned int class_id; /* class ID for bandwidth 'floor' */
+    virNetDevVlan vlan;
+    int trustGuestRxFilters; /* enum virTristateBool */
+
+    int plugtype; /* virNetworkPortPlugType */
+    union {
+        struct {
+            char *brname;
+            int macTableManager; /* enum virNetworkBridgeMACTableManagerType */
+        } bridge; /* For TYPE_NETWORK & TYPE_BRIDGE */
+        struct {
+            char *linkdev;
+            int mode; /* enum virNetDevMacVLanMode from util/virnetdevmacvlan.h */
+        } direct;
+        struct {
+            virPCIDeviceAddress addr; /* PCI Address of device */
+            int driver; /* virNetworkForwardDriverNameType */
+            int managed;
+        } hostdevpci;
+    } plug;
+};
+
+
+void
+virNetworkPortDefFree(virNetworkPortDefPtr port);
+
+virNetworkPortDefPtr
+virNetworkPortDefParseNode(xmlDocPtr xml,
+                           xmlNodePtr root);
+
+virNetworkPortDefPtr
+virNetworkPortDefParseString(const char *xml);
+
+virNetworkPortDefPtr
+virNetworkPortDefParseFile(const char *filename);
+
+char *
+virNetworkPortDefFormat(const virNetworkPortDef *def);
+
+int
+virNetworkPortDefFormatBuf(virBufferPtr buf,
+                           const virNetworkPortDef *def);
+
+int
+virNetworkPortDefSaveStatus(virNetworkPortDef *def,
+                            const char *dir);
+
+int
+virNetworkPortDefDeleteStatus(virNetworkPortDef *def,
+                              const char *dir);
+
+
+#endif /* LIBVIRT_VIRNETWORKPORTDEF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 909975750c..62a07c094e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1075,6 +1075,16 @@ virNetworkObjUpdate;
 virNetworkObjUpdateAssignDef;
 
 
+# conf/virnetworkportdef.h
+virNetworkPortDefFormat;
+virNetworkPortDefFormatBuf;
+virNetworkPortDefFree;
+virNetworkPortDefParseFile;
+virNetworkPortDefParseNode;
+virNetworkPortDefParseString;
+virNetworkPortDefSaveStatus;
+
+
 # conf/virnodedeviceobj.h
 virNodeDeviceObjEndAPI;
 virNodeDeviceObjGetDef;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 46d94d2236..6865ee946e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -149,6 +149,7 @@ EXTRA_DIST = \
 	virmockstathelpers.c \
 	virnetdaemondata \
 	virnetdevtestdata \
+	virnetworkportxml2xmldata \
 	virnwfilterbindingxml2xmldata \
 	virpcitestdata \
 	virscsidata \
@@ -335,6 +336,7 @@ endif WITH_YAJL
 test_programs += \
 		networkxml2xmltest \
 		networkxml2xmlupdatetest \
+		virnetworkportxml2xmltest \
 		$(NULL)
 
 if WITH_NETWORK
@@ -832,6 +834,11 @@ networkxml2xmlupdatetest_SOURCES = \
 	testutils.c testutils.h
 networkxml2xmlupdatetest_LDADD = $(LDADDS)
 
+virnetworkportxml2xmltest_SOURCES = \
+	virnetworkportxml2xmltest.c \
+	testutils.c testutils.h
+virnetworkportxml2xmltest_LDADD = $(LDADDS)
+
 if WITH_NETWORK
 networkxml2conftest_SOURCES = \
 	networkxml2conftest.c \
diff --git a/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml b/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
new file mode 100644
index 0000000000..8036bc2e1c
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
@@ -0,0 +1,9 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <mac address='52:54:00:7b:35:93'/>
+  <plug type='bridge' bridge='virbr0' macTableManager='libvirt'/>
+</networkport>
diff --git a/tests/virnetworkportxml2xmldata/plug-bridge.xml b/tests/virnetworkportxml2xmldata/plug-bridge.xml
new file mode 100644
index 0000000000..e09fc45a9d
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-bridge.xml
@@ -0,0 +1,15 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <group>web1</group>
+  <mac address='52:54:00:7b:35:93'/>
+  <bandwidth classID='1729'>
+    <inbound average='1000' peak='4000' floor='2000' burst='1024'/>
+    <outbound average='128' peak='256' burst='32768'/>
+  </bandwidth>
+  <rxfilters trustGuest='yes'/>
+  <plug type='bridge' bridge='virbr0'/>
+</networkport>
diff --git a/tests/virnetworkportxml2xmldata/plug-direct.xml b/tests/virnetworkportxml2xmldata/plug-direct.xml
new file mode 100644
index 0000000000..81554b4579
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-direct.xml
@@ -0,0 +1,12 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <mac address='52:54:00:7b:35:93'/>
+  <virtualport type='802.1Qbg'>
+    <parameters managerid='11' typeid='1193047' typeidversion='2'/>
+  </virtualport>
+  <plug type='direct' dev='ens3' mode='vepa'/>
+</networkport>
diff --git a/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml b/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
new file mode 100644
index 0000000000..cc4419f3fd
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
@@ -0,0 +1,12 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <mac address='52:54:00:7b:35:93'/>
+  <plug type='hostdev-pci' managed='yes'>
+    <driver name='vfio'/>
+    <address domain='0x0001' bus='0x02' slot='0x03' function='0x4'/>
+  </plug>
+</networkport>
diff --git a/tests/virnetworkportxml2xmldata/plug-network.xml b/tests/virnetworkportxml2xmldata/plug-network.xml
new file mode 100644
index 0000000000..7b08ca295a
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-network.xml
@@ -0,0 +1,16 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <group>web1</group>
+  <mac address='52:54:00:7b:35:93'/>
+  <bandwidth>
+    <inbound average='1000' peak='4000' floor='2000' burst='1024'/>
+    <outbound average='128' peak='256' burst='32768'/>
+  </bandwidth>
+  <class id='1729'/>
+  <rxfilters trustGuest='yes'/>
+  <plug type='network' bridge='virbr0'/>
+</networkport>
diff --git a/tests/virnetworkportxml2xmldata/plug-none.xml b/tests/virnetworkportxml2xmldata/plug-none.xml
new file mode 100644
index 0000000000..ed7199ec8c
--- /dev/null
+++ b/tests/virnetworkportxml2xmldata/plug-none.xml
@@ -0,0 +1,8 @@
+<networkport>
+  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
+  <owner>
+    <name>memtest</name>
+    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
+  </owner>
+  <mac address='52:54:00:7b:35:93'/>
+</networkport>
diff --git a/tests/virnetworkportxml2xmltest.c b/tests/virnetworkportxml2xmltest.c
new file mode 100644
index 0000000000..bb0ae8a8d5
--- /dev/null
+++ b/tests/virnetworkportxml2xmltest.c
@@ -0,0 +1,104 @@
+/*
+ * virnetworkportxml2xmltest.c: network port XML processing test suite
+ *
+ * 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 <unistd.h>
+
+#include <sys/types.h>
+#include <fcntl.h>
+
+#include "internal.h"
+#include "testutils.h"
+#include "virnetworkportdef.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+
+static int
+testCompareXMLToXMLFiles(const char *expected)
+{
+    char *actual = NULL;
+    int ret = -1;
+    virNetworkPortDefPtr dev = NULL;
+
+    if (!(dev = virNetworkPortDefParseFile(expected)))
+        goto cleanup;
+
+    if (!(actual = virNetworkPortDefFormat(dev)))
+        goto cleanup;
+
+    if (virTestCompareToFile(actual, expected) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(actual);
+    virNetworkPortDefFree(dev);
+    return ret;
+}
+
+struct testInfo {
+    const char *name;
+};
+
+static int
+testCompareXMLToXMLHelper(const void *data)
+{
+    const struct testInfo *info = data;
+    int ret = -1;
+    char *xml = NULL;
+
+    if (virAsprintf(&xml, "%s/virnetworkportxml2xmldata/%s.xml",
+                    abs_srcdir, info->name) < 0)
+        goto cleanup;
+
+    ret = testCompareXMLToXMLFiles(xml);
+
+ cleanup:
+    VIR_FREE(xml);
+
+    return ret;
+}
+
+static int
+mymain(void)
+{
+    int ret = 0;
+
+#define DO_TEST(name) \
+    do { \
+        const struct testInfo info = {name}; \
+        if (virTestRun("virnetworkportdeftest " name, \
+                       testCompareXMLToXMLHelper, &info) < 0) \
+            ret = -1; \
+    } while (0)
+
+    DO_TEST("plug-none");
+    DO_TEST("plug-bridge");
+    DO_TEST("plug-bridge-mactbl");
+    DO_TEST("plug-direct");
+    DO_TEST("plug-hostdev-pci");
+
+    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIR_TEST_MAIN(mymain)
diff --git a/tests/virschematest.c b/tests/virschematest.c
index 56bdcb2f88..13c30acc30 100644
--- a/tests/virschematest.c
+++ b/tests/virschematest.c
@@ -227,6 +227,7 @@ mymain(void)
     DO_TEST_DIR("interface.rng", "interfaceschemadata");
     DO_TEST_DIR("network.rng", "../src/network", "networkxml2xmlin",
                 "networkxml2xmlout", "networkxml2confdata");
+    DO_TEST_DIR("networkport.rng", "virnetworkportxml2xmldata");
     DO_TEST_DIR("nodedev.rng", "nodedevschemadata");
     DO_TEST_DIR("nwfilter.rng", "nwfilterxml2xmlout", "../examples/xml/nwfilter");
     DO_TEST_DIR("nwfilterbinding.rng", "virnwfilterbindingxml2xmldata");
-- 
2.21.0

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

Re: [libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Laine Stump 13 weeks ago
On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
> Introduce a virNetworkPortDefPtr struct to represent the data associated
> with a virtual network port. Add APIs for parsing/formatting XML docs
> with the data.
>
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   docs/docs.html.in                             |   1 +
>   docs/formatnetworkport.html.in                | 212 ++++++++
>   docs/schemas/networkport.rng                  | 165 ++++++
>   src/conf/Makefile.inc.am                      |   2 +
>   src/conf/virnetworkportdef.c                  | 509 ++++++++++++++++++
>   src/conf/virnetworkportdef.h                  | 113 ++++
>   src/libvirt_private.syms                      |  10 +
>   tests/Makefile.am                             |   7 +
>   .../plug-bridge-mactbl.xml                    |   9 +
>   .../virnetworkportxml2xmldata/plug-bridge.xml |  15 +
>   .../virnetworkportxml2xmldata/plug-direct.xml |  12 +
>   .../plug-hostdev-pci.xml                      |  12 +
>   .../plug-network.xml                          |  16 +
>   tests/virnetworkportxml2xmldata/plug-none.xml |   8 +
>   tests/virnetworkportxml2xmltest.c             | 104 ++++
>   tests/virschematest.c                         |   1 +
>   16 files changed, 1196 insertions(+)
>   create mode 100644 docs/formatnetworkport.html.in
>   create mode 100644 docs/schemas/networkport.rng
>   create mode 100644 src/conf/virnetworkportdef.c
>   create mode 100644 src/conf/virnetworkportdef.h
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge.xml
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-direct.xml
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-network.xml
>   create mode 100644 tests/virnetworkportxml2xmldata/plug-none.xml
>   create mode 100644 tests/virnetworkportxml2xmltest.c
>
> diff --git a/docs/docs.html.in b/docs/docs.html.in
> index d0ff844d0c..c8674e1457 100644
> --- a/docs/docs.html.in
> +++ b/docs/docs.html.in
> @@ -72,6 +72,7 @@
>           <dd>Description of the XML schemas for
>             <a href="formatdomain.html">domains</a>,
>             <a href="formatnetwork.html">networks</a>,
> +          <a href="formatnetworkport.html">network ports</a>,
>             <a href="formatnwfilter.html">network filtering</a>,
>             <a href="formatstorage.html">storage</a>,
>             <a href="formatstorageencryption.html">storage encryption</a>,
> diff --git a/docs/formatnetworkport.html.in b/docs/formatnetworkport.html.in
> new file mode 100644
> index 0000000000..0211518a6a
> --- /dev/null
> +++ b/docs/formatnetworkport.html.in
> @@ -0,0 +1,212 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE html>
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +  <body>
> +    <h1>Network XML format</h1>
> +
> +    <ul id="toc">
> +    </ul>
> +
> +    <p>
> +      This page provides an introduction to the network port XML format.
> +      This stores information about the connection between an virtual
> +      interface on a virtual domain's, and the virtual network it is
> +      attached to.
> +    </p>
> +
> +    <h2><a id="elements">Element and attribute overview</a></h2>
> +
> +    <p>
> +      The root element required for all virtual network ports is
> +      named <code>networkport</code> and has no configurable attributes
> +      The network port XML format is available <span class="since">since
> +      5.4.0</span>
> +    </p>
> +
> +    <h3><a id="elementsMetadata">General metadata</a></h3>
> +
> +    <p>
> +      The first elements provide basic metadata about the virtual
> +      network port.
> +    </p>
> +
> +    <pre>
> +&lt;networkport
> +  &lt;uuid&gt;7ae63b5f-fe96-4af0-a7c3-da04ba1b3f54&lt;/uuid&gt;
> +  &lt;owner&gt;
> +    &lt;uuid&gt;06578fc1-c686-46fa-bc2c-220893b466a6&lt;/uuid&gt;
> +    &lt;name&gt;myguest&lt;name&gt;
> +  &lt;/owner&gt;
> +  &lt;group&gt;webfront&lt;group&gt;
> +  &lt;mac address='52:54:0:7b:35:93'/&gt;
> +  ...</pre>
> +
> +    <dl>
> +      <dt><code>uuid</code></dt>
> +      <dd>The content of the <code>uuid</code> element provides
> +        a globally unique identifier for the virtual network port.
> +        The format must be RFC 4122 compliant, eg <code>3e3fce45-4f53-4fa7-bb32-11f34168b82b</code>.
> +        If omitted when defining/creating a new network port, a random
> +        UUID is generated.</dd>
> +      <dd>The <code>owner</code> node records the domain object that
> +        is the owner of the network port. It contains two child nodes:
> +        <dl>
> +          <dt><code>uuid</code></dt>
> +          <dd>The content of the <code>uuid</code> element provides
> +            a globally unique identifier for the virtual domain.</dd>
> +          <dt><code>name</code></dt>
> +          <dd>The unique name of the virtual domain</dd>
> +        </dl>
> +      </dd>
> +      <dt><code>group</code></dt>
> +      <dd>The port group in the virtual network to which the
> +        port belongs. Can be omitted if no port groups are
> +        defined on the network.</dd>
> +      <dt><code>mac</code></dt>
> +      <dd>The <code>address</code> attribute provides the MAC
> +        address of the virtual port that will be see by the
> +        guest. The MAC address must not start with 0xFE as this
> +        byte is reserved for use on the host side of the port.
> +      </dd>
> +    </dl>
> +
> +    <h3><a id="elementsCommon">Common elements</a></h3>
> +
> +    <p>
> +      The following elements are common to one of more of the plug
> +      types listed later
> +    </p>
> +
> +    <pre>
> +  ...
> +  &lt;bandwidth&gt;
> +    &lt;inbound average='1000' peak='5000' floor='200' burst='1024'/&gt;
> +    &lt;outbound average='128' peak='256' burst='256'/&gt;
> +  &lt;/bandwidth&gt;
> +  &lt;rxfilters trustGuest='yes'/&gt;
> +  &lt;virtualport type='802.1Qbg'&gt;
> +    &lt;parameters managerid='11' typeid='1193047' typeidversion='2'/&gt;
> +  &lt;/virtualport&gt;
> +  ...</pre>
> +
> +    <dl>
> +      <dt><code>bandwidth</code></dt>
> +      <dd>This part of the network port XML provides setting quality of service.
> +        Incoming and outgoing traffic can be shaped independently.
> +        The <code>bandwidth</code> element and its child elements are described
> +        in the <a href="formatnetwork.html#elementQoS">QoS</a> section of
> +        the Network XML. In addition the <code>classID</code> attribute may
> +        exist provide the ID of the traffic shaping class that is active.
> +      </dd>
> +      <dt><code>rxfilters</code></dt>
> +      <dd>The <code>rxfilters</code> element property
> +        <code>trustGuestRxFilters</code> provides the


Earlier you referred to this as "trustGuest" (also looks like that's 
what the parser/formatter use)


> +        capability for the host to detect and trust reports from the
> +        guest regarding changes to the interface mac address and receive
> +        filters by setting the attribute to <code>yes</code>. The default
> +        setting for the attribute is <code>no</code> for security
> +        reasons and support depends on the guest network device model as
> +        well as the type of connection on the host - currently it is
> +        only supported for the virtio device model and for macvtap
> +        connections on the host.
> +      </dd>
> +      <dt><code>virtualport</code></dt>
> +      <dd>The <code>virtualport</code> element describes metadata that
> +        needs to be provided to the underlying network subsystem. It
> +        is described in the domain XML
> +        <a href="formatdomain.html#elementsNICS">interface documentation</a>.
> +      </dd>
> +    </dl>
> +
> +
> +    <h3><a id="elementsPlug">Plugs</a></h3>
> +
> +    <p>
> +      The <code>plug</code> element has varying content depending
> +      on the value of the <code>type</code> attribute.
> +    </p>
> +
> +    <h4><a id="elementsPlugNetwork">Network</a></h4>
> +
> +    <p>
> +      The <code>network</code> plug type refers to a managed virtual
> +      network plug that is based on a traditional software bridge
> +      device privately managed by libvirt.
> +    </p>


Your patches essentially take the same items that are in the 
virActualNetDef and put them into virNetworkPortDef. I think this is the 
right way to approach the task of changing the infrastructure - makes it 
more straightforward to get all the same functionality working with the 
new intra-driver communication method. The one thing that bothers me, 
though, is that this ends up replicating design mistakes that we ("I" 
:-P - most of it was good, I just added in some things that were kind of 
screwed up...) made with the original. I'm hoping that there will also 
be a path to correcting those mistakes after the fact so that we don't 
have to live with them forever.


In particular, we've been using "type='network' to imply that the 
network supports a bandwidth floor setting (and who knows what else). 
Although we made this mistake with <interface> status, I would really 
prefer if we don't (permanently at least) propagate the mistake to 
networkport. What "type='network' really means is a tap device connected 
to a bridge that probably has a routed connection and supports bandwidth 
floor setting). But what if network port was instead something like:


    <plug type='tap' forward='route' bridge='virbr0'/>

    <plug type='tap' forward='bridge' bridge='br0'/>

    <plug type='direct' dev='ens3' mode='vepa'/>


(maybe if we use "type='tap'", we should also use "type='macvtap'"?. 
Hmm, and of course if we're going to spell out "tap", then for 
container-ish connections that use a veth pair, we would have to say 
"type='veth'"...)

(in the future if networkport was used for what we call 
"type='ethernet'" in <interface>, it would maybe look like <plug 
type='tap' forward='unknown'/> (with no bridge attribute). Or something 
like that.)


The idea really is to deconstruct all of the attributes implicit in 
"type='network'" (and "type='bridge'", etc) into several independent 
attributes that do a better job of correctly describing the connection).


Also, I'm thinking to a time in the future when the network driver might 
create and connect the device on behalf of an unprivileged hypervisor. 
The above could be extended to:


     <plug type='tap' tap='vnet0' forward='route' bridge='virbr0'/>


etc.


Anyway, I haven't thought this all through to the logical end, and 
there's definitely holes in it, but I'm concerned of repeating the 
mistake of trying to use a single attribute to convey multiple pieces of 
information when we have the chance to rethink it and get it right.


Anyway, like I said, I think it *is* better to change the infrastructure 
without changing the attributes right now, but will we be able to change 
it in the future?


+
> +    <pre>
> +  ...
> +  &lt;plug type='network' bridge='virbr0'&gt;
> +  ...</pre>
> +
> +    <p>
> +      The <code>bridge</code> attribute provides the name of the
> +      privately managed bridge device associated with the virtual
> +      network.
> +    </p>
> +
> +    <h4><a id="elementsPlugNetwork">Bridge</a></h4>
> +
> +    <p>
> +      The <code>bridge</code> plug type refers to an externally
> +      managed traditional software bridge.
> +    </p>
> +
> +    <pre>
> +  ...
> +  &lt;plug type='bridge' bridge='br2'&gt;
> +  ...</pre>
> +
> +    <p>
> +      The <code>bridge</code> attribute provides the name of the
> +      externally managed bridge device associated with the virtual
> +      network.
> +    </p>
> +
> +    <h4><a id="elementsPlugNetwork">Direct</a></h4>
> +
> +    <p>
> +      The <code>direct</code> plug type refers to an connection


"a connection"


> +      directly to a physical network interface.
> +    </p>
> +
> +    <pre>
> +  ...
> +  &lt;plug type='direct' dev='ens3' mode='vepa'/&gt;
> +  ...</pre>
> +
> +    <p>
> +      The <code>dev</code> attribute provides the name of the
> +      physical network interface to which the port will be
> +      connected. The <code>mode</code> attribute describes
> +      how the connection will be setup and takes the same
> +      values described in the
> +      <a href="formatdomain.html#elementsNICSDirect">domain XML</a>.
> +    </p>
> +
> +    <h4><a id="elementsPlugNetwork">Host PCI</a></h4>
> +
> +    <p>
> +      The <code>hostdev-pci</code> plug type refers to the
> +      passthrough of a physical PCI device rather than emulation.
> +    </p>
> +
> +    <pre>
> +  ...
> +  &lt;plug type='hostdev-pci' managed='yes'&gt;
> +    &lt;driver name='vfio'/&gt;
> +    &lt;address domain='0x0001' bus='0x02' slot='0x03' function='0x4'/&gt;
> +  &lt;/plug&gt;
> +  ...</pre>
> +
> +    <p>
> +      The <code>managed</code> attribute indicates who is responsible for
> +      managing the PCI device in the host. When set to the value <code>yes</code>
> +      libvirt is responsible for automatically detaching the device from host
> +      drivers and resetting it if needed. If the value is <code>no</code>,
> +      some other party must ensure the device is not attached to any
> +      host drivers.


Actually it specifically must be bound to the vfio-pci driver (legacy 
KVM assignment, using the pcistub driver) has been deprecated since 
kernel 3.1.x days, and completely removed in RHEL kernels (at least, 
maybe also upstream) since RHEL7.0. But I'm not going to be persnickety 
about details in the docs :-)


> +    </p>
> +
> +  </body>
> +</html>
> diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng
> new file mode 100644
> index 0000000000..8192f6efc4
> --- /dev/null
> +++ b/docs/schemas/networkport.rng
> @@ -0,0 +1,165 @@
> +<?xml version="1.0"?>
> +<!-- A Relax NG schema for the libvirt network port XML format -->
> +<grammar xmlns="http://relaxng.org/ns/structure/1.0"
> +         datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes">
> +  <include href='basictypes.rng'/>
> +  <include href='networkcommon.rng'/>
> +
> +  <start>
> +    <ref name="networkport"/>
> +  </start>
> +
> +  <define name="networkport">
> +    <element name="networkport">
> +      <interleave>
> +        <element name="uuid">
> +          <ref name="UUID"/>
> +        </element>
> +        <ref name="owner"/>
> +        <ref name="mac"/>
> +        <optional>
> +          <ref name="group"/>
> +        </optional>
> +        <optional>
> +          <ref name="class"/>
> +        </optional>



I thought in the previous patch you had integrated <class id='blah'/> 
into the bandwidth element as a classID attribute. Did you just forget 
to remove it from the rng?



> +        <optional>
> +          <ref name="rxfilters"/>
> +        </optional>
> +        <optional>
> +          <ref name="virtualPortProfile"/>
> +        </optional>
> +        <optional>
> +          <ref name="bandwidth"/>
> +        </optional>
> +        <optional>
> +          <ref name="plug"/>
> +        </optional>
> +      </interleave>
> +    </element>
> +  </define>
> +
> +  <define name="owner">
> +    <element name="owner">
> +      <element name="name">
> +        <text/>
> +      </element>
> +      <element name="uuid">
> +        <ref name="UUID"/>
> +      </element>
> +    </element>
> +  </define>
> +
> +  <define name="mac">
> +    <element name="mac">
> +      <attribute name="address">
> +        <ref name="uniMacAddr"/>
> +      </attribute>
> +      <empty/>
> +    </element>
> +  </define>
> +
> +  <define name="group">
> +    <element name="group">
> +      <ref name="deviceName"/>
> +    </element>
> +  </define>
> +
> +  <define name="class">
> +    <element name="class">
> +      <attribute name="id">
> +        <ref name="positiveInteger"/>
> +      </attribute>
> +    </element>
> +  </define>

> +
> +  <define name="rxfilters">
> +    <element name="rxfilters">
> +      <attribute name="trustGuest">
> +        <ref name="virYesNo"/>
> +      </attribute>
> +    </element>
> +  </define>
> +
> +  <define name="plug">
> +    <element name="plug">
> +      <choice>
> +        <ref name="plugnetwork"/>
> +        <ref name="plugbridge"/>
> +        <ref name="plugdirect"/>
> +        <ref name="plughostdevpci"/>
> +      </choice>
> +    </element>
> +  </define>
> +
> +  <define name="plugnetwork">
> +    <attribute name="type">
> +      <value>network</value>
> +    </attribute>
> +    <attribute name="bridge">
> +      <ref name="deviceName"/>
> +    </attribute>
> +    <optional>
> +      <attribute name="macTableManager">
> +        <ref name="macTableManager"/>
> +      </attribute>
> +    </optional>
> +  </define>
> +
> +  <define name="plugbridge">
> +    <attribute name="type">
> +      <value>bridge</value>
> +    </attribute>
> +    <attribute name="bridge">
> +      <ref name="deviceName"/>
> +    </attribute>
> +    <optional>
> +      <attribute name="macTableManager">
> +        <ref name="macTableManager"/>
> +      </attribute>
> +    </optional>
> +  </define>
> +
> +  <define name="plugdirect">
> +    <attribute name="type">
> +      <value>direct</value>
> +    </attribute>
> +    <attribute name="dev">
> +      <ref name="deviceName"/>
> +    </attribute>
> +    <attribute name="mode">
> +      <choice>
> +        <value>bridge</value>
> +        <value>passthrough</value>
> +        <value>private</value>
> +        <value>vepa</value>
> +      </choice>
> +    </attribute>
> +  </define>
> +
> +  <define name="plughostdevpci">
> +    <attribute name="type">
> +      <value>hostdev-pci</value>
> +    </attribute>
> +    <optional>
> +      <attribute name="managed">
> +        <ref name="virYesNo"/>
> +      </attribute>
> +    </optional>
> +    <optional>
> +      <element name="driver">
> +        <attribute name="name">
> +          <choice>
> +            <value>kvm</value>


We *really* need to get rid of <driver name='kvm'/> for hostdev - it's 
been deprecated for 5 years, and removing it would eliminate ugly code.


(I suppose since this is distinct from the <driver name='blah'/> in 
<hostdev> or <interface type='hostdev'> you could safely leave it out. 
For that matter, since the only driver supported is vfio, the entire 
<driver> element is unneeded - maybe we don't need it in networkport at 
all?)


(I remember when we added it, we didn't really want to (specifically 
because it could mean supporting it essentially forever), but there were 
some people concerned that we might have to fallback to legacy KVM 
assignment if VFIO was found inadequate in some way. Turns out their 
fears were unfounded (although it could easily have gone the other way :-))

> +            <value>vfio</value>
> +          </choice>
> +        </attribute>
> +        <empty/>
> +      </element>
> +    </optional>
> +    <element name='address'>
> +      <ref name="pciaddress"/>
> +    </element>
> +  </define>
> +
> +</grammar>
> diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am
> index 3e9fdd1aea..6b52ba674b 100644
> --- a/src/conf/Makefile.inc.am
> +++ b/src/conf/Makefile.inc.am
> @@ -7,6 +7,8 @@ NETDEV_CONF_SOURCES = \
>   	conf/netdev_vport_profile_conf.c \
>   	conf/netdev_vlan_conf.h \
>   	conf/netdev_vlan_conf.c \
> +	conf/virnetworkportdef.h \
> +	conf/virnetworkportdef.c \
>   	$(NULL)
>   
>   DOMAIN_CONF_SOURCES = \
> diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c
> new file mode 100644
> index 0000000000..6c896968ce
> --- /dev/null
> +++ b/src/conf/virnetworkportdef.c
> @@ -0,0 +1,509 @@
> +/*
> + * virnetworkportdef.c: network port XML processing
> + *
> + * 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 "viralloc.h"
> +#include "virerror.h"
> +#include "virstring.h"
> +#include "virfile.h"
> +#include "virnetworkportdef.h"
> +#include "network_conf.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NETWORK
> +
> +VIR_ENUM_IMPL(virNetworkPortPlug,
> +              VIR_NETWORK_PORT_PLUG_TYPE_LAST,
> +              "none", "network", "bridge", "direct", "hostdev-pci");
> +
> +void
> +virNetworkPortDefFree(virNetworkPortDefPtr def)
> +{
> +    if (!def)
> +        return;
> +
> +    VIR_FREE(def->ownername);
> +    VIR_FREE(def->group);
> +
> +    virNetDevBandwidthFree(def->bandwidth);
> +    virNetDevVlanClear(&def->vlan);
> +    VIR_FREE(def->virtPortProfile);
> +
> +    switch ((virNetworkPortPlugType)def->plugtype) {
> +    case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
> +    case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
> +        VIR_FREE(def->plug.bridge.brname);
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
> +        VIR_FREE(def->plug.direct.linkdev);
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
> +    default:
> +        break;
> +    }
> +
> +    VIR_FREE(def);
> +}
> +
> +
> +
> +static virNetworkPortDefPtr
> +virNetworkPortDefParseXML(xmlXPathContextPtr ctxt)
> +{
> +    virNetworkPortDefPtr def;
> +    char *uuid = NULL;
> +    xmlNodePtr virtPortNode;
> +    xmlNodePtr vlanNode;
> +    xmlNodePtr bandwidthNode;
> +    xmlNodePtr addressNode;
> +    char *trustGuestRxFilters = NULL;
> +    char *mac = NULL;
> +    char *macmgr = NULL;
> +    char *mode = NULL;
> +    char *plugtype = NULL;
> +    char *managed = NULL;
> +    char *driver = NULL;
> +    char *class_id = NULL;
> +
> +    if (VIR_ALLOC(def) < 0)
> +        return NULL;
> +
> +    uuid = virXPathString("string(./uuid)", ctxt);
> +    if (!uuid) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("network port has no uuid"));
> +        goto error;
> +    }
> +    if (virUUIDParse(uuid, def->uuid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse UUID '%s'"), uuid);
> +        goto error;
> +    }
> +
> +    def->ownername = virXPathString("string(./owner/name)", ctxt);
> +    if (!def->ownername) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("network port has no owner name"));
> +        goto error;
> +    }


I'm curious why you made name and uuid subelements of owner, rather than 
attributes. Just for consistency with the name element we use in other 
places? Or are you expecting it might need qualifying attributes? Or 
just because the <owner> line would be too long if they were attributes?



> +
> +    VIR_FREE(uuid);
> +    uuid = virXPathString("string(./owner/uuid)", ctxt);
> +    if (!uuid) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("network port has no owner UUID"));
> +        goto error;
> +    }
> +
> +    if (virUUIDParse(uuid, def->owneruuid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse UUID '%s'"), uuid);
> +        goto error;
> +    }
> +
> +    def->group = virXPathString("string(./group)", ctxt);
> +
> +    virtPortNode = virXPathNode("./virtualport", ctxt);
> +    if (virtPortNode &&
> +        (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) {
> +        goto error;
> +    }
> +
> +    mac = virXPathString("string(./mac/@address)", ctxt);
> +    if (!mac) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("network port has no mac"));
> +        goto error;
> +    }
> +    if (virMacAddrParse(mac, &def->mac) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to parse MAC '%s'"), mac);
> +        goto error;
> +    }
> +
> +    bandwidthNode = virXPathNode("./bandwidth", ctxt);
> +    /*
> +     * We don't know if the port will allow the "floor" param or
> +     * not at this stage, so we must just tell virNetDevBandwidthParse
> +     * to allow it regardless. Any bad config must be reported at
> +     * time of use instead.
> +     */


(BTW, in case it got lost in other stuff, I agree with you that we 
should just always allow specifying floor in the XML, and only give an 
error if it turns out it's not supported at runtime.)


> +    if (bandwidthNode &&
> +        virNetDevBandwidthParse(&def->bandwidth, &def->class_id,
> +                                bandwidthNode, true) < 0)
> +        goto error;
> +
> +    vlanNode = virXPathNode("./vlan", ctxt);
> +    if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0)
> +        goto error;
> +
> +
> +    trustGuestRxFilters
> +        = virXPathString("string(./rxfilters/@trustGuest)", ctxt);
> +    if (trustGuestRxFilters) {
> +        if ((def->trustGuestRxFilters
> +             = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid guest rx filters trust setting '%s' "),
> +                           trustGuestRxFilters);
> +            goto error;
> +        }
> +    }
> +
> +    plugtype = virXPathString("string(./plug/@type)", ctxt);
> +
> +    if (plugtype &&
> +        (def->plugtype = virNetworkPortPlugTypeFromString(plugtype)) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Invalid network prt plug type '%s'"), plugtype);
> +    }
> +
> +    switch (def->plugtype) {
> +    case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
> +    case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
> +        if (!(def->plug.bridge.brname = virXPathString("string(./plug/@bridge)", ctxt))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Missing network port bridge name"));
> +            goto error;
> +        }
> +        macmgr = virXPathString("string(./plug/@macTableManager)", ctxt);
> +        if (macmgr &&
> +            (def->plug.bridge.macTableManager =
> +             virNetworkBridgeMACTableManagerTypeFromString(macmgr)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid macTableManager setting '%s' "
> +                             "in network port"), macmgr);
> +            goto error;
> +        }
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
> +        if (!(def->plug.direct.linkdev = virXPathString("string(./plug/@dev)", ctxt))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Missing network port link device name"));
> +            goto error;
> +        }
> +        mode = virXPathString("string(./plug/@mode)", ctxt);
> +        if (mode &&
> +            (def->plug.direct.mode =
> +             virNetDevMacVLanModeTypeFromString(mode)) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid mode setting '%s' in network port"), mode);
> +            goto error;
> +        }
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
> +        managed = virXPathString("string(./plug/@managed)", ctxt);
> +        if (managed &&
> +            (def->plug.hostdevpci.managed =
> +             virTristateBoolTypeFromString(managed)) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid managed setting '%s' in network port"), mode);
> +            goto error;
> +        }
> +        driver = virXPathString("string(./plug/driver/@name)", ctxt);
> +        if (driver &&
> +            (def->plug.hostdevpci.driver =
> +             virNetworkForwardDriverNameTypeFromString(driver)) <= 0) {
> +              virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Missing network port driver name"));
> +            goto error;
> +        }


Yeah, the more I think about this, the more I think it's unnecessary 
(and that we should also consider removing it from other places it's 
used, since non-VFIO device assignment hasn't worked in Linux since 
RHEL6 days).


> +        if (!(addressNode = virXPathNode("./plug/address", ctxt))) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Missing network port PCI address"));
> +            goto error;
> +        }
> +
> +        if (virPCIDeviceAddressParseXML(addressNode, &def->plug.hostdevpci.addr) < 0)
> +            goto error;
> +        break;
> +
> +    case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
> +    default:
> +        virReportEnumRangeError(virNetworkPortPlugType, def->plugtype);
> +        goto error;
> +    }
> +
> + cleanup:
> +    VIR_FREE(class_id);
> +    VIR_FREE(uuid);
> +    VIR_FREE(plugtype);
> +    VIR_FREE(mac);
> +    VIR_FREE(mode);
> +    VIR_FREE(macmgr);
> +    VIR_FREE(driver);
> +    VIR_FREE(managed);
> +    return def;
> +
> + error:
> +    virNetworkPortDefFree(def);
> +    def = NULL;
> +    goto cleanup;
> +}
> +
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseNode(xmlDocPtr xml,
> +                           xmlNodePtr root)
> +{
> +    xmlXPathContextPtr ctxt = NULL;
> +    virNetworkPortDefPtr def = NULL;
> +
> +    if (STRNEQ((const char *)root->name, "networkport")) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       "%s",
> +                       _("unknown root element for network port"));
> +        goto cleanup;
> +    }
> +
> +    ctxt = xmlXPathNewContext(xml);
> +    if (ctxt == NULL) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    ctxt->node = root;
> +    def = virNetworkPortDefParseXML(ctxt);
> +
> + cleanup:
> +    xmlXPathFreeContext(ctxt);
> +    return def;
> +}
> +
> +
> +static virNetworkPortDefPtr
> +virNetworkPortDefParse(const char *xmlStr,
> +                       const char *filename)
> +{
> +    virNetworkPortDefPtr def = NULL;
> +    xmlDocPtr xml;
> +
> +    if ((xml = virXMLParse(filename, xmlStr, _("(networkport_definition)")))) {
> +        def = virNetworkPortDefParseNode(xml, xmlDocGetRootElement(xml));
> +        xmlFreeDoc(xml);
> +    }
> +
> +    return def;
> +}
> +
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseString(const char *xmlStr)
> +{
> +    return virNetworkPortDefParse(xmlStr, NULL);
> +}
> +
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseFile(const char *filename)
> +{
> +    return virNetworkPortDefParse(NULL, filename);
> +}
> +
> +
> +char *
> +virNetworkPortDefFormat(const virNetworkPortDef *def)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (virNetworkPortDefFormatBuf(&buf, def) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        return NULL;
> +    }
> +
> +    if (virBufferCheckError(&buf) < 0)
> +        return NULL;
> +
> +    return virBufferContentAndReset(&buf);
> +}
> +


So much boilerplate... :-P


> +
> +int
> +virNetworkPortDefFormatBuf(virBufferPtr buf,
> +                           const virNetworkPortDef *def)
> +{
> +    char uuid[VIR_UUID_STRING_BUFLEN];
> +    char macaddr[VIR_MAC_STRING_BUFLEN];
> +
> +    virBufferAddLit(buf, "<networkport>\n");
> +
> +    virBufferAdjustIndent(buf, 2);
> +
> +    virUUIDFormat(def->uuid, uuid);
> +    virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid);
> +
> +    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");
> +
> +    if (def->group)
> +        virBufferEscapeString(buf, "<group>%s</group>\n", def->group);


Because virBufferEscapeString() outputs nothing if the 2nd arg is NULL, 
it's not necessary to have the "if (def->group)". (I've never been 
totally comfortable with that behavior, but it's being used in a lot of 
places in the code, so if it ever changed then lots of things would also 
break, so I think it's safe to rely on it :-))


> +
> +    virMacAddrFormat(&def->mac, macaddr);
> +    virBufferAsprintf(buf, "<mac address='%s'/>\n", macaddr);
> +
> +    if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
> +        return -1;
> +    if (def->bandwidth)
> +        virNetDevBandwidthFormat(def->bandwidth, def->class_id, buf);
> +    if (virNetDevVlanFormat(&def->vlan, buf) < 0)
> +        return -1;
> +    if (def->trustGuestRxFilters)
> +        virBufferAsprintf(buf, "<rxfilters trustGuest='%s'/>\n",
> +                          virTristateBoolTypeToString(def->trustGuestRxFilters));
> +
> +    if (def->plugtype != VIR_NETWORK_PORT_PLUG_TYPE_NONE) {
> +        virBufferAsprintf(buf, "<plug type='%s'",
> +                          virNetworkPortPlugTypeToString(def->plugtype));
> +
> +        switch (def->plugtype) {
> +        case VIR_NETWORK_PORT_PLUG_TYPE_NONE:
> +            break;
> +
> +        case VIR_NETWORK_PORT_PLUG_TYPE_NETWORK:
> +        case VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE:
> +            virBufferEscapeString(buf, " bridge='%s'", def->plug.bridge.brname);
> +            if (def->plug.bridge.macTableManager)
> +                virBufferAsprintf(buf, " macTableManager='%s'",
> +                                  virNetworkBridgeMACTableManagerTypeToString(
> +                                      def->plug.bridge.macTableManager));
> +            virBufferAddLit(buf, "/>\n");
> +            break;
> +
> +        case VIR_NETWORK_PORT_PLUG_TYPE_DIRECT:
> +            virBufferEscapeString(buf, " dev='%s'", def->plug.direct.linkdev);
> +            virBufferAsprintf(buf, " mode='%s'",
> +                              virNetDevMacVLanModeTypeToString(
> +                                  def->plug.direct.mode));
> +            virBufferAddLit(buf, "/>\n");
> +            break;
> +
> +        case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
> +            virBufferAsprintf(buf, " managed='%s'>\n",
> +                              def->plug.hostdevpci.managed ? "yes" : "no");
> +            virBufferAdjustIndent(buf, 2);
> +            if (def->plug.hostdevpci.driver)
> +                virBufferEscapeString(buf, "<driver name='%s'/>\n",
> +                                      virNetworkForwardDriverNameTypeToString(
> +                                          def->plug.hostdevpci.driver));
> +
> +            virPCIDeviceAddressFormat(buf, def->plug.hostdevpci.addr, false);
> +            virBufferAdjustIndent(buf, -2);
> +            virBufferAddLit(buf, "</plug>\n");
> +            break;
> +
> +        case VIR_NETWORK_PORT_PLUG_TYPE_LAST:
> +        default:
> +            virReportEnumRangeError(virNetworkPortPlugType, def->plugtype);
> +            return -1;
> +        }
> +    }
> +
> +
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</networkport>\n");
> +
> +    return 0;
> +}
> +
> +
> +static char *
> +virNetworkPortDefConfigFile(const char *dir,
> +                            const char *name)
> +{
> +    char *ret = NULL;
> +
> +    ignore_value(virAsprintf(&ret, "%s/%s.xml", dir, name));
> +    return ret;
> +}
> +
> +
> +int
> +virNetworkPortDefSaveStatus(virNetworkPortDef *def,
> +                            const char *dir)
> +{
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    char *path;
> +    char *xml = NULL;
> +    int ret = -1;
> +
> +    virUUIDFormat(def->uuid, uuidstr);
> +
> +    if (virFileMakePath(dir) < 0)
> +        goto cleanup;
> +
> +    if (!(path = virNetworkPortDefConfigFile(dir, uuidstr)))
> +        goto cleanup;
> +
> +    if (!(xml = virNetworkPortDefFormat(def)))
> +        goto cleanup;
> +
> +    if (virXMLSaveFile(path, uuidstr, "net-port-create", xml) < 0)
> +        goto cleanup;


For other objects, when they are saved to a "status" file, the xml is 
enclosed in a "<blahstatus>" element, e.g. <domstatus>, <networkstatus>, 
etc. I guess this is done in case there are other things about the state 
of the object that aren't contained directly in virBlahDef. Did you not 
do that here on purpose (maybe because the entire point of this object 
is to keep track of the state of a particular connection, so of course 
there is nothing extra?), or was it an oversite?


> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(xml);
> +    VIR_FREE(path);
> +    return ret;
> +}
> +
> +
> +int
> +virNetworkPortDefDeleteStatus(virNetworkPortDef *def,
> +                              const char *dir)
> +{
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    char *path;
> +    int ret = -1;
> +
> +    virUUIDFormat(def->uuid, uuidstr);
> +
> +    if (!(path = virNetworkPortDefConfigFile(dir, uuidstr)))
> +        goto cleanup;
> +
> +    if (unlink(path) < 0 && errno != ENOENT) {
> +        virReportSystemError(errno,
> +                             _("Unable to delete %s"), path);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(path);
> +    return ret;
> +}
> diff --git a/src/conf/virnetworkportdef.h b/src/conf/virnetworkportdef.h
> new file mode 100644
> index 0000000000..94b2fabf6b
> --- /dev/null
> +++ b/src/conf/virnetworkportdef.h
> @@ -0,0 +1,113 @@
> +/*
> + * virnetworkportdef.h: network port XML processing
> + *
> + * 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/>.
> + *
> + */
> +
> +#ifndef LIBVIRT_VIRNETWORKPORTDEF_H
> +# define LIBVIRT_VIRNETWORKPORTDEF_H
> +
> +# include "internal.h"
> +# include "viruuid.h"
> +# include "virnetdevvlan.h"
> +# include "virnetdevvportprofile.h"
> +# include "virnetdevbandwidth.h"
> +# include "virpci.h"
> +# include "virxml.h"
> +# include "netdev_vport_profile_conf.h"
> +# include "netdev_bandwidth_conf.h"
> +# include "netdev_vlan_conf.h"
> +
> +typedef struct _virNetworkPortDef virNetworkPortDef;
> +typedef virNetworkPortDef *virNetworkPortDefPtr;
> +
> +typedef enum {
> +    VIR_NETWORK_PORT_PLUG_TYPE_NONE,
> +    VIR_NETWORK_PORT_PLUG_TYPE_NETWORK,
> +    VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE,
> +    VIR_NETWORK_PORT_PLUG_TYPE_DIRECT,
> +    VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI,
> +
> +    VIR_NETWORK_PORT_PLUG_TYPE_LAST,
> +} virNetworkPortPlugType;
> +
> +VIR_ENUM_DECL(virNetworkPortPlug);
> +
> +struct _virNetworkPortDef {
> +    unsigned char uuid[VIR_UUID_BUFLEN];
> +    char *ownername;
> +    unsigned char owneruuid[VIR_UUID_BUFLEN];
> +
> +    char *group;
> +    virMacAddr mac;
> +
> +    virNetDevVPortProfilePtr virtPortProfile;
> +    virNetDevBandwidthPtr bandwidth;
> +    unsigned int class_id; /* class ID for bandwidth 'floor' */
> +    virNetDevVlan vlan;
> +    int trustGuestRxFilters; /* enum virTristateBool */
> +
> +    int plugtype; /* virNetworkPortPlugType */
> +    union {
> +        struct {
> +            char *brname;
> +            int macTableManager; /* enum virNetworkBridgeMACTableManagerType */
> +        } bridge; /* For TYPE_NETWORK & TYPE_BRIDGE */
> +        struct {
> +            char *linkdev;
> +            int mode; /* enum virNetDevMacVLanMode from util/virnetdevmacvlan.h */
> +        } direct;
> +        struct {
> +            virPCIDeviceAddress addr; /* PCI Address of device */
> +            int driver; /* virNetworkForwardDriverNameType */
> +            int managed;
> +        } hostdevpci;
> +    } plug;
> +};
> +
> +
> +void
> +virNetworkPortDefFree(virNetworkPortDefPtr port);
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseNode(xmlDocPtr xml,
> +                           xmlNodePtr root);
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseString(const char *xml);
> +
> +virNetworkPortDefPtr
> +virNetworkPortDefParseFile(const char *filename);
> +
> +char *
> +virNetworkPortDefFormat(const virNetworkPortDef *def);
> +
> +int
> +virNetworkPortDefFormatBuf(virBufferPtr buf,
> +                           const virNetworkPortDef *def);
> +
> +int
> +virNetworkPortDefSaveStatus(virNetworkPortDef *def,
> +                            const char *dir);
> +
> +int
> +virNetworkPortDefDeleteStatus(virNetworkPortDef *def,
> +                              const char *dir);
> +
> +
> +#endif /* LIBVIRT_VIRNETWORKPORTDEF_H */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 909975750c..62a07c094e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1075,6 +1075,16 @@ virNetworkObjUpdate;
>   virNetworkObjUpdateAssignDef;
>   
>   
> +# conf/virnetworkportdef.h
> +virNetworkPortDefFormat;
> +virNetworkPortDefFormatBuf;
> +virNetworkPortDefFree;
> +virNetworkPortDefParseFile;
> +virNetworkPortDefParseNode;
> +virNetworkPortDefParseString;
> +virNetworkPortDefSaveStatus;
> +
> +
>   # conf/virnodedeviceobj.h
>   virNodeDeviceObjEndAPI;
>   virNodeDeviceObjGetDef;
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 46d94d2236..6865ee946e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -149,6 +149,7 @@ EXTRA_DIST = \
>   	virmockstathelpers.c \
>   	virnetdaemondata \
>   	virnetdevtestdata \
> +	virnetworkportxml2xmldata \
>   	virnwfilterbindingxml2xmldata \
>   	virpcitestdata \
>   	virscsidata \
> @@ -335,6 +336,7 @@ endif WITH_YAJL
>   test_programs += \
>   		networkxml2xmltest \
>   		networkxml2xmlupdatetest \
> +		virnetworkportxml2xmltest \
>   		$(NULL)
>   
>   if WITH_NETWORK
> @@ -832,6 +834,11 @@ networkxml2xmlupdatetest_SOURCES = \
>   	testutils.c testutils.h
>   networkxml2xmlupdatetest_LDADD = $(LDADDS)
>   
> +virnetworkportxml2xmltest_SOURCES = \
> +	virnetworkportxml2xmltest.c \
> +	testutils.c testutils.h
> +virnetworkportxml2xmltest_LDADD = $(LDADDS)
> +
>   if WITH_NETWORK
>   networkxml2conftest_SOURCES = \
>   	networkxml2conftest.c \
> diff --git a/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml b/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
> new file mode 100644
> index 0000000000..8036bc2e1c
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
> @@ -0,0 +1,9 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <plug type='bridge' bridge='virbr0' macTableManager='libvirt'/>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmldata/plug-bridge.xml b/tests/virnetworkportxml2xmldata/plug-bridge.xml
> new file mode 100644
> index 0000000000..e09fc45a9d
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-bridge.xml
> @@ -0,0 +1,15 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <group>web1</group>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <bandwidth classID='1729'>
> +    <inbound average='1000' peak='4000' floor='2000' burst='1024'/>
> +    <outbound average='128' peak='256' burst='32768'/>
> +  </bandwidth>
> +  <rxfilters trustGuest='yes'/>
> +  <plug type='bridge' bridge='virbr0'/>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmldata/plug-direct.xml b/tests/virnetworkportxml2xmldata/plug-direct.xml
> new file mode 100644
> index 0000000000..81554b4579
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-direct.xml
> @@ -0,0 +1,12 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <virtualport type='802.1Qbg'>
> +    <parameters managerid='11' typeid='1193047' typeidversion='2'/>
> +  </virtualport>
> +  <plug type='direct' dev='ens3' mode='vepa'/>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml b/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
> new file mode 100644
> index 0000000000..cc4419f3fd
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
> @@ -0,0 +1,12 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <plug type='hostdev-pci' managed='yes'>
> +    <driver name='vfio'/>
> +    <address domain='0x0001' bus='0x02' slot='0x03' function='0x4'/>
> +  </plug>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmldata/plug-network.xml b/tests/virnetworkportxml2xmldata/plug-network.xml
> new file mode 100644
> index 0000000000..7b08ca295a
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-network.xml
> @@ -0,0 +1,16 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <group>web1</group>
> +  <mac address='52:54:00:7b:35:93'/>
> +  <bandwidth>
> +    <inbound average='1000' peak='4000' floor='2000' burst='1024'/>
> +    <outbound average='128' peak='256' burst='32768'/>
> +  </bandwidth>
> +  <class id='1729'/>
> +  <rxfilters trustGuest='yes'/>
> +  <plug type='network' bridge='virbr0'/>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmldata/plug-none.xml b/tests/virnetworkportxml2xmldata/plug-none.xml
> new file mode 100644
> index 0000000000..ed7199ec8c
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmldata/plug-none.xml
> @@ -0,0 +1,8 @@
> +<networkport>
> +  <uuid>5d744f21-ba4a-4d6e-bdb2-30a35ff3207d</uuid>
> +  <owner>
> +    <name>memtest</name>
> +    <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid>
> +  </owner>
> +  <mac address='52:54:00:7b:35:93'/>
> +</networkport>
> diff --git a/tests/virnetworkportxml2xmltest.c b/tests/virnetworkportxml2xmltest.c
> new file mode 100644
> index 0000000000..bb0ae8a8d5
> --- /dev/null
> +++ b/tests/virnetworkportxml2xmltest.c
> @@ -0,0 +1,104 @@
> +/*
> + * virnetworkportxml2xmltest.c: network port XML processing test suite
> + *
> + * 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 <unistd.h>
> +
> +#include <sys/types.h>
> +#include <fcntl.h>
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "virnetworkportdef.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +
> +static int
> +testCompareXMLToXMLFiles(const char *expected)
> +{
> +    char *actual = NULL;
> +    int ret = -1;
> +    virNetworkPortDefPtr dev = NULL;
> +
> +    if (!(dev = virNetworkPortDefParseFile(expected)))
> +        goto cleanup;
> +
> +    if (!(actual = virNetworkPortDefFormat(dev)))
> +        goto cleanup;
> +
> +    if (virTestCompareToFile(actual, expected) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(actual);
> +    virNetworkPortDefFree(dev);
> +    return ret;
> +}
> +
> +struct testInfo {
> +    const char *name;
> +};
> +
> +static int
> +testCompareXMLToXMLHelper(const void *data)
> +{
> +    const struct testInfo *info = data;
> +    int ret = -1;
> +    char *xml = NULL;
> +
> +    if (virAsprintf(&xml, "%s/virnetworkportxml2xmldata/%s.xml",
> +                    abs_srcdir, info->name) < 0)
> +        goto cleanup;
> +
> +    ret = testCompareXMLToXMLFiles(xml);
> +
> + cleanup:
> +    VIR_FREE(xml);
> +
> +    return ret;
> +}
> +
> +static int
> +mymain(void)
> +{
> +    int ret = 0;
> +
> +#define DO_TEST(name) \
> +    do { \
> +        const struct testInfo info = {name}; \
> +        if (virTestRun("virnetworkportdeftest " name, \
> +                       testCompareXMLToXMLHelper, &info) < 0) \
> +            ret = -1; \
> +    } while (0)
> +
> +    DO_TEST("plug-none");
> +    DO_TEST("plug-bridge");
> +    DO_TEST("plug-bridge-mactbl");
> +    DO_TEST("plug-direct");
> +    DO_TEST("plug-hostdev-pci");
> +
> +    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +VIR_TEST_MAIN(mymain)
> diff --git a/tests/virschematest.c b/tests/virschematest.c
> index 56bdcb2f88..13c30acc30 100644
> --- a/tests/virschematest.c
> +++ b/tests/virschematest.c
> @@ -227,6 +227,7 @@ mymain(void)
>       DO_TEST_DIR("interface.rng", "interfaceschemadata");
>       DO_TEST_DIR("network.rng", "../src/network", "networkxml2xmlin",
>                   "networkxml2xmlout", "networkxml2confdata");
> +    DO_TEST_DIR("networkport.rng", "virnetworkportxml2xmldata");
>       DO_TEST_DIR("nodedev.rng", "nodedevschemadata");
>       DO_TEST_DIR("nwfilter.rng", "nwfilterxml2xmlout", "../examples/xml/nwfilter");
>       DO_TEST_DIR("nwfilterbinding.rng", "virnwfilterbindingxml2xmldata");


I'm inclined to ACK this (after a few questions are resolved) even 
though I would like to replace the type='network|bridge' with something 
more deconstructed - we have to take a dive and do it with *something* 
(as long as we're able to fine tune it later). Let me look through the 
rest of the patches though.

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

Re: [libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Daniel P. Berrangé 13 weeks ago
On Mon, May 20, 2019 at 09:44:04PM -0400, Laine Stump wrote:
> On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
> > Introduce a virNetworkPortDefPtr struct to represent the data associated
> > with a virtual network port. Add APIs for parsing/formatting XML docs
> > with the data.
> > 
> > Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> > ---
> >   docs/docs.html.in                             |   1 +
> >   docs/formatnetworkport.html.in                | 212 ++++++++
> >   docs/schemas/networkport.rng                  | 165 ++++++
> >   src/conf/Makefile.inc.am                      |   2 +
> >   src/conf/virnetworkportdef.c                  | 509 ++++++++++++++++++
> >   src/conf/virnetworkportdef.h                  | 113 ++++
> >   src/libvirt_private.syms                      |  10 +
> >   tests/Makefile.am                             |   7 +
> >   .../plug-bridge-mactbl.xml                    |   9 +
> >   .../virnetworkportxml2xmldata/plug-bridge.xml |  15 +
> >   .../virnetworkportxml2xmldata/plug-direct.xml |  12 +
> >   .../plug-hostdev-pci.xml                      |  12 +
> >   .../plug-network.xml                          |  16 +
> >   tests/virnetworkportxml2xmldata/plug-none.xml |   8 +
> >   tests/virnetworkportxml2xmltest.c             | 104 ++++
> >   tests/virschematest.c                         |   1 +
> >   16 files changed, 1196 insertions(+)
> >   create mode 100644 docs/formatnetworkport.html.in
> >   create mode 100644 docs/schemas/networkport.rng
> >   create mode 100644 src/conf/virnetworkportdef.c
> >   create mode 100644 src/conf/virnetworkportdef.h
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge.xml
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-direct.xml
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-network.xml
> >   create mode 100644 tests/virnetworkportxml2xmldata/plug-none.xml
> >   create mode 100644 tests/virnetworkportxml2xmltest.c
> > 

> > diff --git a/docs/formatnetworkport.html.in b/docs/formatnetworkport.html.in
> > new file mode 100644
> > index 0000000000..0211518a6a
> > --- /dev/null
> > +++ b/docs/formatnetworkport.html.in


> > +    <dl>
> > +      <dt><code>bandwidth</code></dt>
> > +      <dd>This part of the network port XML provides setting quality of service.
> > +        Incoming and outgoing traffic can be shaped independently.
> > +        The <code>bandwidth</code> element and its child elements are described
> > +        in the <a href="formatnetwork.html#elementQoS">QoS</a> section of
> > +        the Network XML. In addition the <code>classID</code> attribute may
> > +        exist provide the ID of the traffic shaping class that is active.
> > +      </dd>
> > +      <dt><code>rxfilters</code></dt>
> > +      <dd>The <code>rxfilters</code> element property
> > +        <code>trustGuestRxFilters</code> provides the
> 
> 
> Earlier you referred to this as "trustGuest" (also looks like that's what
> the parser/formatter use)

Yes, copy & paste mistake

> 
> 
> > +        capability for the host to detect and trust reports from the
> > +        guest regarding changes to the interface mac address and receive
> > +        filters by setting the attribute to <code>yes</code>. The default
> > +        setting for the attribute is <code>no</code> for security
> > +        reasons and support depends on the guest network device model as
> > +        well as the type of connection on the host - currently it is
> > +        only supported for the virtio device model and for macvtap
> > +        connections on the host.
> > +      </dd>
> > +      <dt><code>virtualport</code></dt>
> > +      <dd>The <code>virtualport</code> element describes metadata that
> > +        needs to be provided to the underlying network subsystem. It
> > +        is described in the domain XML
> > +        <a href="formatdomain.html#elementsNICS">interface documentation</a>.
> > +      </dd>
> > +    </dl>
> > +
> > +
> > +    <h3><a id="elementsPlug">Plugs</a></h3>
> > +
> > +    <p>
> > +      The <code>plug</code> element has varying content depending
> > +      on the value of the <code>type</code> attribute.
> > +    </p>
> > +
> > +    <h4><a id="elementsPlugNetwork">Network</a></h4>
> > +
> > +    <p>
> > +      The <code>network</code> plug type refers to a managed virtual
> > +      network plug that is based on a traditional software bridge
> > +      device privately managed by libvirt.
> > +    </p>
> 
> 
> Your patches essentially take the same items that are in the virActualNetDef
> and put them into virNetworkPortDef. I think this is the right way to
> approach the task of changing the infrastructure - makes it more
> straightforward to get all the same functionality working with the new
> intra-driver communication method. The one thing that bothers me, though, is
> that this ends up replicating design mistakes that we ("I" :-P - most of it
> was good, I just added in some things that were kind of screwed up...) made
> with the original. I'm hoping that there will also be a path to correcting
> those mistakes after the fact so that we don't have to live with them
> forever.

I don't see any viable alternative. We have to be able to convert
between the virActualNetDef & virNetworkPortDef in both directions
without loosing information. This inherantly means replicating the
same concepts.

> In particular, we've been using "type='network' to imply that the network
> supports a bandwidth floor setting (and who knows what else). Although we
> made this mistake with <interface> status, I would really prefer if we don't
> (permanently at least) propagate the mistake to networkport. What
> "type='network' really means is a tap device connected to a bridge that
> probably has a routed connection and supports bandwidth floor setting). But
> what if network port was instead something like:
> 
> 
>    <plug type='tap' forward='route' bridge='virbr0'/>
> 
>    <plug type='tap' forward='bridge' bridge='br0'/>
> 
>    <plug type='direct' dev='ens3' mode='vepa'/>
>
> (maybe if we use "type='tap'", we should also use "type='macvtap'"?. Hmm,
> and of course if we're going to spell out "tap", then for container-ish
> connections that use a veth pair, we would have to say "type='veth'"...)

You raised this same point in an earlier review of this same series.
This is not desirable as it is refering to a specific implementation
choice of the QEMU backend. The LXC driver doesn't use tap devices
or macvtap devices, instead it uses veth devices and macvlan devices.


> Anyway, like I said, I think it *is* better to change the infrastructure
> without changing the attributes right now, but will we be able to change it
> in the future?

I don't believe we should change it, as the current concepts are
intentionally independant of a specific implementation choice.


> > diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng
> > new file mode 100644
> > index 0000000000..8192f6efc4
> > --- /dev/null
> > +++ b/docs/schemas/networkport.rng
> > @@ -0,0 +1,165 @@
> > +<?xml version="1.0"?>
> > +<!-- A Relax NG schema for the libvirt network port XML format -->
> > +<grammar xmlns="http://relaxng.org/ns/structure/1.0"
> > +         datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes">
> > +  <include href='basictypes.rng'/>
> > +  <include href='networkcommon.rng'/>
> > +
> > +  <start>
> > +    <ref name="networkport"/>
> > +  </start>
> > +
> > +  <define name="networkport">
> > +    <element name="networkport">
> > +      <interleave>
> > +        <element name="uuid">
> > +          <ref name="UUID"/>
> > +        </element>
> > +        <ref name="owner"/>
> > +        <ref name="mac"/>
> > +        <optional>
> > +          <ref name="group"/>
> > +        </optional>
> > +        <optional>
> > +          <ref name="class"/>
> > +        </optional>
> 
> 
> 
> I thought in the previous patch you had integrated <class id='blah'/> into
> the bandwidth element as a classID attribute. Did you just forget to remove
> it from the rng?

Yeah, I forgot to update the schema.



> > +  <define name="plughostdevpci">
> > +    <attribute name="type">
> > +      <value>hostdev-pci</value>
> > +    </attribute>
> > +    <optional>
> > +      <attribute name="managed">
> > +        <ref name="virYesNo"/>
> > +      </attribute>
> > +    </optional>
> > +    <optional>
> > +      <element name="driver">
> > +        <attribute name="name">
> > +          <choice>
> > +            <value>kvm</value>
> 
> 
> We *really* need to get rid of <driver name='kvm'/> for hostdev - it's been
> deprecated for 5 years, and removing it would eliminate ugly code.

Regardless of whether the code uses it or not, it is in the domain schema
and for purposes of doing a bi-directional conversion, it better to have
it in the network port XML too.  It isn't the network port XML parser's
job to decide if its used or not - that's upto the QEMU driver


> > +static virNetworkPortDefPtr
> > +virNetworkPortDefParseXML(xmlXPathContextPtr ctxt)
> > +{
> > +    virNetworkPortDefPtr def;
> > +    char *uuid = NULL;
> > +    xmlNodePtr virtPortNode;
> > +    xmlNodePtr vlanNode;
> > +    xmlNodePtr bandwidthNode;
> > +    xmlNodePtr addressNode;
> > +    char *trustGuestRxFilters = NULL;
> > +    char *mac = NULL;
> > +    char *macmgr = NULL;
> > +    char *mode = NULL;
> > +    char *plugtype = NULL;
> > +    char *managed = NULL;
> > +    char *driver = NULL;
> > +    char *class_id = NULL;
> > +
> > +    if (VIR_ALLOC(def) < 0)
> > +        return NULL;
> > +
> > +    uuid = virXPathString("string(./uuid)", ctxt);
> > +    if (!uuid) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       "%s", _("network port has no uuid"));
> > +        goto error;
> > +    }
> > +    if (virUUIDParse(uuid, def->uuid) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("Unable to parse UUID '%s'"), uuid);
> > +        goto error;
> > +    }
> > +
> > +    def->ownername = virXPathString("string(./owner/name)", ctxt);
> > +    if (!def->ownername) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       "%s", _("network port has no owner name"));
> > +        goto error;
> > +    }
> 
> 
> I'm curious why you made name and uuid subelements of owner, rather than
> attributes. Just for consistency with the name element we use in other
> places? Or are you expecting it might need qualifying attributes? Or just
> because the <owner> line would be too long if they were attributes?

This mirrors what is done with the nwfilter binding XML schema.



> > +    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
> > +        managed = virXPathString("string(./plug/@managed)", ctxt);
> > +        if (managed &&
> > +            (def->plug.hostdevpci.managed =
> > +             virTristateBoolTypeFromString(managed)) < 0) {
> > +            virReportError(VIR_ERR_XML_ERROR,
> > +                           _("Invalid managed setting '%s' in network port"), mode);
> > +            goto error;
> > +        }
> > +        driver = virXPathString("string(./plug/driver/@name)", ctxt);
> > +        if (driver &&
> > +            (def->plug.hostdevpci.driver =
> > +             virNetworkForwardDriverNameTypeFromString(driver)) <= 0) {
> > +              virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                           _("Missing network port driver name"));
> > +            goto error;
> > +        }
> 
> 
> Yeah, the more I think about this, the more I think it's unnecessary (and
> that we should also consider removing it from other places it's used, since
> non-VFIO device assignment hasn't worked in Linux since RHEL6 days).

Again, from XML parsing pov it is preferrable to have it present so that
conversion to/from the actual network def avoids any special cases. If
QEMU driver wants to reject it later that's fine

> > +int
> > +virNetworkPortDefSaveStatus(virNetworkPortDef *def,
> > +                            const char *dir)
> > +{
> > +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> > +    char *path;
> > +    char *xml = NULL;
> > +    int ret = -1;
> > +
> > +    virUUIDFormat(def->uuid, uuidstr);
> > +
> > +    if (virFileMakePath(dir) < 0)
> > +        goto cleanup;
> > +
> > +    if (!(path = virNetworkPortDefConfigFile(dir, uuidstr)))
> > +        goto cleanup;
> > +
> > +    if (!(xml = virNetworkPortDefFormat(def)))
> > +        goto cleanup;
> > +
> > +    if (virXMLSaveFile(path, uuidstr, "net-port-create", xml) < 0)
> > +        goto cleanup;
> 
> 
> For other objects, when they are saved to a "status" file, the xml is
> enclosed in a "<blahstatus>" element, e.g. <domstatus>, <networkstatus>,
> etc. I guess this is done in case there are other things about the state of
> the object that aren't contained directly in virBlahDef. Did you not do that
> here on purpose (maybe because the entire point of this object is to keep
> track of the state of a particular connection, so of course there is nothing
> extra?), or was it an oversite?

Essentially the network port XML is always a "status" file, since this
object only ever exists for a running VM. There shouldn't be any info
we record in the network port that isn't part of the public schema. Thus
there's no need to wrap it in a extra status XML schema. This is the same
as the nwfilter binding XML which is also a runtime only XML doc.


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

Re: [libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Laine Stump 13 weeks ago
On 5/22/19 6:29 AM, Daniel P. Berrangé wrote:
> On Mon, May 20, 2019 at 09:44:04PM -0400, Laine Stump wrote:
>> On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
>>> Introduce a virNetworkPortDefPtr struct to represent the data associated
>>> with a virtual network port. Add APIs for parsing/formatting XML docs
>>> with the data.
>>>
>>> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
>>> ---
>>>    docs/docs.html.in                             |   1 +
>>>    docs/formatnetworkport.html.in                | 212 ++++++++
>>>    docs/schemas/networkport.rng                  | 165 ++++++
>>>    src/conf/Makefile.inc.am                      |   2 +
>>>    src/conf/virnetworkportdef.c                  | 509 ++++++++++++++++++
>>>    src/conf/virnetworkportdef.h                  | 113 ++++
>>>    src/libvirt_private.syms                      |  10 +
>>>    tests/Makefile.am                             |   7 +
>>>    .../plug-bridge-mactbl.xml                    |   9 +
>>>    .../virnetworkportxml2xmldata/plug-bridge.xml |  15 +
>>>    .../virnetworkportxml2xmldata/plug-direct.xml |  12 +
>>>    .../plug-hostdev-pci.xml                      |  12 +
>>>    .../plug-network.xml                          |  16 +
>>>    tests/virnetworkportxml2xmldata/plug-none.xml |   8 +
>>>    tests/virnetworkportxml2xmltest.c             | 104 ++++
>>>    tests/virschematest.c                         |   1 +
>>>    16 files changed, 1196 insertions(+)
>>>    create mode 100644 docs/formatnetworkport.html.in
>>>    create mode 100644 docs/schemas/networkport.rng
>>>    create mode 100644 src/conf/virnetworkportdef.c
>>>    create mode 100644 src/conf/virnetworkportdef.h
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge-mactbl.xml
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-bridge.xml
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-direct.xml
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-hostdev-pci.xml
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-network.xml
>>>    create mode 100644 tests/virnetworkportxml2xmldata/plug-none.xml
>>>    create mode 100644 tests/virnetworkportxml2xmltest.c
>>>
> [...]
>>
>> Your patches essentially take the same items that are in the virActualNetDef
>> and put them into virNetworkPortDef. I think this is the right way to
>> approach the task of changing the infrastructure - makes it more
>> straightforward to get all the same functionality working with the new
>> intra-driver communication method. The one thing that bothers me, though, is
>> that this ends up replicating design mistakes that we ("I" :-P - most of it
>> was good, I just added in some things that were kind of screwed up...) made
>> with the original. I'm hoping that there will also be a path to correcting
>> those mistakes after the fact so that we don't have to live with them
>> forever.
> I don't see any viable alternative. We have to be able to convert
> between the virActualNetDef & virNetworkPortDef in both directions
> without loosing information. This inherantly means replicating the
> same concepts.


Or something that the existing concepts can be derived from (that's what 
I was getting at). But like I said, even if we agreed that was a good 
idea, it would create extra complexity that would make it more likely 
something would go wrong)


>
>> In particular, we've been using "type='network' to imply that the network
>> supports a bandwidth floor setting (and who knows what else). Although we
>> made this mistake with <interface> status, I would really prefer if we don't
>> (permanently at least) propagate the mistake to networkport. What
>> "type='network' really means is a tap device connected to a bridge that
>> probably has a routed connection and supports bandwidth floor setting). But
>> what if network port was instead something like:
>>
>>
>>     <plug type='tap' forward='route' bridge='virbr0'/>
>>
>>     <plug type='tap' forward='bridge' bridge='br0'/>
>>
>>     <plug type='direct' dev='ens3' mode='vepa'/>
>>
>> (maybe if we use "type='tap'", we should also use "type='macvtap'"?. Hmm,
>> and of course if we're going to spell out "tap", then for container-ish
>> connections that use a veth pair, we would have to say "type='veth'"...)
> You raised this same point in an earlier review of this same series.


Really? Ugh. I need to find a good memory medication or something... 
Anyway, sorry for the circular conversation :-/ (I may bring it up again 
sometime in the future (e.g. a few lines below this!), but I at least 
promise I won't present it as if we'd never talked about it before, and 
won't use it to block patches :-))


> This is not desirable as it is refering to a specific implementation
> choice of the QEMU backend. The LXC driver doesn't use tap devices
> or macvtap devices, instead it uses veth devices and macvlan devices.


For current uses, yes. But if we ever hope to make the network driver 
useful for a non-privileged hypervisor that can't create/attach its own 
network devices, we will necessarily have to descend to the 
implementation detail in the XML.


Anyway, since my main intent is to be sure that we're thinking about 
this, I'll agree to the XML you've defined and give my


Reviewed-by: Laine Stump <laine@laine.org>


(assuming the few typos and cut-paste errors are fixed, including the 
RNG problem that is going to cause virschematest failure once patch 03 
is applied).


>
>
>> Anyway, like I said, I think it *is* better to change the infrastructure
>> without changing the attributes right now, but will we be able to change it
>> in the future?
> I don't believe we should change it, as the current concepts are
> intentionally independant of a specific implementation choice.


I think we're overloading a single attribute with several pieces of 
information, and it would be cleaner to have a separate attribute for 
each piece of info (yeah, I understand that you think that those details 
should be unimportant to the network driver, and you may be right). But 
we can talk about that more later (or not, if it ends up ultimately 
being unimportant/irrelevant/blatantly incorrect).


>>> +    case VIR_NETWORK_PORT_PLUG_TYPE_HOSTDEV_PCI:
>>> +        managed = virXPathString("string(./plug/@managed)", ctxt);
>>> +        if (managed &&
>>> +            (def->plug.hostdevpci.managed =
>>> +             virTristateBoolTypeFromString(managed)) < 0) {
>>> +            virReportError(VIR_ERR_XML_ERROR,
>>> +                           _("Invalid managed setting '%s' in network port"), mode);
>>> +            goto error;
>>> +        }
>>> +        driver = virXPathString("string(./plug/driver/@name)", ctxt);
>>> +        if (driver &&
>>> +            (def->plug.hostdevpci.driver =
>>> +             virNetworkForwardDriverNameTypeFromString(driver)) <= 0) {
>>> +              virReportError(VIR_ERR_XML_ERROR, "%s",
>>> +                           _("Missing network port driver name"));
>>> +            goto error;
>>> +        }
>>
>> Yeah, the more I think about this, the more I think it's unnecessary (and
>> that we should also consider removing it from other places it's used, since
>> non-VFIO device assignment hasn't worked in Linux since RHEL6 days).
> Again, from XML parsing pov it is preferrable to have it present so that
> conversion to/from the actual network def avoids any special cases. If
> QEMU driver wants to reject it later that's fine


Sure. The code that bothers me the most is the device reset shenanigans 
that exist only for legacy KVM device assignment. I guess we could just 
remove that.


>> For other objects, when they are saved to a "status" file, the xml is
>> enclosed in a "<blahstatus>" element, e.g. <domstatus>, <networkstatus>,
>> etc. I guess this is done in case there are other things about the state of
>> the object that aren't contained directly in virBlahDef. Did you not do that
>> here on purpose (maybe because the entire point of this object is to keep
>> track of the state of a particular connection, so of course there is nothing
>> extra?), or was it an oversite?
> Essentially the network port XML is always a "status" file, since this
> object only ever exists for a running VM. There shouldn't be any info
> we record in the network port that isn't part of the public schema. Thus
> there's no need to wrap it in a extra status XML schema. This is the same
> as the nwfilter binding XML which is also a runtime only XML doc.
>

Okay, I hadn't looked at what was written for nwfilter status before, 
only network and qemu. Just wanted to make sure this was intentional.

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

Re: [libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Laine Stump 13 weeks ago
On 5/20/19 9:44 PM, Laine Stump wrote:
> On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
>> Introduce a virNetworkPortDefPtr struct to represent the data associated
>> with a virtual network port. Add APIs for parsing/formatting XML docs
>> with the data.


Oops. This fails virschematest #1974:


1974) Checking plug-bridge.xml against networkport.rng                  
... libvirt: XML Util error : XML document failed to validate against 
schema: Unable to validate doc against 
/home/laine/devel/libvirt/docs/schemas/networkport.rng
Extra element bandwidth in interleave
Element networkport failed to validate content
FAILED

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

Re: [libvirt] [PATCH v5 02/24] conf: introduce virNetworkPortDefPtr struct and XML support

Posted by Daniel P. Berrangé 13 weeks ago
On Tue, May 21, 2019 at 10:46:27AM -0400, Laine Stump wrote:
> On 5/20/19 9:44 PM, Laine Stump wrote:
> > On 5/14/19 11:48 AM, Daniel P. Berrangé wrote:
> > > Introduce a virNetworkPortDefPtr struct to represent the data associated
> > > with a virtual network port. Add APIs for parsing/formatting XML docs
> > > with the data.
> 
> 
> Oops. This fails virschematest #1974:
> 
> 
> 1974) Checking plug-bridge.xml against networkport.rng                  ...
> libvirt: XML Util error : XML document failed to validate against schema:
> Unable to validate doc against
> /home/laine/devel/libvirt/docs/schemas/networkport.rng
> Extra element bandwidth in interleave
> Element networkport failed to validate content
> FAILED

Yeah, due to mistake in the RNG, and forgetting to update one of the tests
too.

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