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

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(a)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)

> + 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'"...)

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?

> 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?

> + <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.

> +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?

> + 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).

> +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?