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