Re: [libvirt] [PATCH v3 01/36] network: restrict usage of port management APIs
On Thu, Mar 21, 2019 at 08:27:20PM -0400, Cole Robinson wrote:
Okay so I needed to do some studying to understand what's going
on in
the first part of this series. Just gonna type some notes here:
virDomainActualNetDef tracks all the data we need to convert a
virNetworkPtr content to a virDomainNetDef <source>. It's only ever
filled in for <interface type='network'> when a domain is running.
hypervisor drivers fill the data in at VM startup by calling
virDomainNetAllocateActualDevice, which hands the DomainNetDefPtr off to
bridge_drive.c callbacks, which serialize the net config into
virDomainActualNetDef.
Yes, close enough.
One desired end goal of this series is making sure that bridge_driver
is
not messing with any Domain*Def stuff. The first 10-15 patches here are
unwinding some pieces of that before it gets into the real new stuff.
Two real goals:
- the virt drivers must assume the virtual network driver does
not exist. As such they must not rely on using it, except
when having an type=network interface
- the network driver must never touch the domain configuration
if they need to accept, store or output data about the
guest NIC's connection to network, they must have their
own schema for that. This is the new virNetworkPortPtr object
and schema.
Essentially the virNetworkPortPtr should obsolete 100% of everything
stored in the virDomainActualNetDef struct.
Ideally we would in fact stop persisting virDomainActualNetDef on
disk in the status XML for a domain entirely. Instead we should
fetch the live information from the virNetworkPortPtr object when
libvirtd starts up fresh, as that is the canonical source of truth.
I've not gone that far in this series. IOW, we're just trusting that
it is always in sync which is OK for now.
On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> The port allocation APIs are currently called unconditionally for all
> types of NIC, but (mostly) only do anything for NICs with type=network.
>
> The exception is the port allocate API which does some validation even
> for NICs with type!=network. Relying on this validation is flawed,
> however, since the network driver may not even be installed. IOW virt
> drivers must not delegate validation to the network driver for NICs
> with type != network.
>
> This change allows us to report errors when the virtual network driver
> is not registered.
>
For this and the following patch I don't really follow why we can't put
the iface->type == TYPE_NETWORK and the netconn = virGetConnectNetwork()
directly into the domain_conf.c bridge_driver callback wrappers like
virDomainNetAllocateActualDevice. After the final patch in this series,
the open coded netconn lookup and TYPE_NETWORK checks are still there,
and the function signature hasn't changed. Am I missing something?
Pushing the virGetConnectNetwork call too far down leads to pretty
inefficient code when a guest has multiple NICs IMHO. If anything
I could push the calls further up the stack in places.
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 :|