9:46 a.m.
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(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
>>
[...]
>
> 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(a)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.