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>
> ---
> 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.
Those are the only additional places I could find.