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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
