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

On 3/21/19 9:07 PM, 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(a)redhat.com&gt; >> --- >>   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. I remember there was *some* reason for it, but it's very possible it's not valid. So lacking any solid memory from that distant time, one thing to do is audit all occurrences of VIR_DOMAIN_NET_TYPE_NETWORK where the actualType is being compared (rather than the NetDef's type), and behavior is different from VIR_DOMAIN_NET_TYPE_BRIDGE (which is apparently what Cole did, since he's already found most of the "interesting" places I found :-)). He mentions one that he found below, and he also pointed out the class_id thing during parsing in the previous patch, which will now be broken even if we don't support bandwidth on unmanaged bridge networks (because class_id is only parsed when actualType == NETWORK). I'll see if I find any others with a quick search...

> 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. I *think* that's correct, but it's been too long since I looked at the code. >   That would make it easier to > audit any TYPE_NETWORK paths in the status parser as well > > 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. virDomainNetResolveActualType() also needs to be adjusted. Also libxl handles the two types differently in libxlMakeNic, although I don't know enough about Xen networking to know which is way is better.