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

[Cole Robinson]

On 3/21/19 10:16 PM, Laine Stump wrote:
> 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 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.
> 
> 
> 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...
> 

It's difficult to audit: to know whether actualType can even be
TYPE_NETWORK after this patch requires knowing if the caller is acting
on a live/starting VM or not which is context dependent.

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

Patch 8 addresses that case, by removing it entirely :)

- Cole