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@xxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
