[libvirt] [PATCH v3 01/36] network: restrict usage of port management APIs

[Daniel P. Berrangé]

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