[libvirt] [PATCH v3 05/36] network: use 'bridge' as actual type instead of 'network'

[Daniel P. Berrangé]

On Thu, Mar 21, 2019 at 09:07:36PM -0400, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > Ports allocated on virtual networks with type=nat|route|open all get
> > given an actual type of 'network'.
> > 
> > Only ports in networks with type=bridge use an actual type of 'bridge'.
> > 
> > This distinction makes little sense since the virtualization drivers
> > will treat both actual types in exactly the same way, as they're all
> > just bridge devices a VM needs to be connected to.
> > 
> > This doesn't affect user visible XML since the "actual" device XML
> > is internal only, but we need code to convert the data upgrades.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  src/network/bridge_driver.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> 
> Conceptually this makes sense. I wonder what the original motivation for
> the differentiation was. This also makes me wish there was a separate
> enum for the internal actual->type field, it would make ambiguous code
> much more readable
> 
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 4770dd1e4e..40122612c8 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -4454,11 +4454,7 @@ networkAllocateActualDevice(virNetworkPtr net,
> >      case VIR_NETWORK_FORWARD_NAT:
> >      case VIR_NETWORK_FORWARD_ROUTE:
> >      case VIR_NETWORK_FORWARD_OPEN:
> > -        /* for these forward types, the actual net type really *is*
> > -         * NETWORK; we just keep the info from the portgroup in
> > -         * iface->data.network.actual
> > -         */
> > -        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
> > +        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> >  
> >          /* we also store the bridge device and macTableManager settings
> >           * in iface->data.network.actual->data.bridge for later use
> > @@ -4832,6 +4828,15 @@ networkNotifyActualDevice(virNetworkPtr net,
> >                      netdef->bridge) < 0))
> >              goto error;
> >  
> > +    /* Older libvirtd uses actualType==network, but we now
> > +     * just use actualType==bridge, as nothing needs to
> > +     * distinguish the two cases, and this simplifies virt
> > +     * drive code */
> > +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > +        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> > +        actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
> > +    }
> > +
> 
> Why do this here and not at the status XML parse time? This code path is
> only called on reconnected devices correct? So that should always be
> hitting the status XML parser too AFAICT. That would make it easier to
> audit any TYPE_NETWORK paths in the status parser as well

This has to come after the code that is a few lines higher outside
the diffstat. So the XML parser is too early. That said we might
delete the earlier code entirely.

> qemu_driver.c processNicRxFilterChangedEvent also has a
> 
> if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
> 
> which looks sketchy after this. And it's involving bandwidth stuff so it
> may affect the previous patch too.

Yeah, that looks like something i missed.

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