[libvirt] [PATCH v4 04/29] network: use 'bridge' as actual type instead of 'network'

[Laine Stump]

On 4/17/19 1:19 PM, 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.


Yeah, I don't remember if there was an operational difference between 
the two in the past (other than bandwidth floor) or if I just made the 
distinction because I was concerned that there *might be*, but 
definitely it's something that needs to go! (Heck, maybe there *still* 
is a reason for differentiating, but even if there is we should still be 
able to look at the config of the network itself to learn the necessary 
details).

>
> This doesn't affect user visible XML since the "actual" device XML
> is internal only, but we need code to convert the data upgrades.


This does mean that downgrades will create "strange situations". Nothing 
horribly bad, but worth remembering for future reference.


>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>   src/conf/domain_conf.c      | 28 +++++++++++++++++++++++-----
>   src/network/bridge_driver.c | 11 +++--------
>   src/qemu/qemu_driver.c      |  2 +-


You also need to update the result of several xml2xml tests:

qemustatusxml2xmldata/migration-out-nbd-out.xml

qemustatusxml2xmldata/migration-in-params-out.xml

qemustatusxml2xmldata/migration-out-params-out.xml

qemustatusxml2xmldata/migration-out-nbd-tls-out.xml

qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml


(thanks again to crobinson for VIR_TEST_REGENERATE_OUTPUT :-)


>   3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0df3c2ed49..1dde45a6fd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5081,6 +5081,19 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
>           return -1;
>       }
>   
> +    /* 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 (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +        net->data.network.actual != NULL  &&
> +        net->data.network.actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        char mac[VIR_MAC_STRING_BUFLEN];
> +        virMacAddrFormat(&net->mac, mac);
> +        VIR_DEBUG("Updating NIC %s actual type to bridge", mac);
> +        net->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +    }
> +


It looks like git was confused here during a rebase, and it merged the 
code into the wrong function - this patch puts it into 
virDomainDiskDefPostParse(), but I'm guessing it should go in 
virDomainNetDefPostParse() (moving it there certainly gets it to compile 
properly at least :-)


(NB: once several years ago git merged a chunk into the wrong function 
and I learned that (at that time, hopefully it's changed since) git 
ignored the name of the function listed just after the line number info 
at the top of each chunk).

>       return 0;
>   }
>   
> @@ -11267,11 +11280,16 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>       }
>   
>       bandwidth_node = virXPathNode("./bandwidth", ctxt);
> -    if (bandwidth_node &&
> -        virNetDevBandwidthParse(&actual->bandwidth,
> -                                bandwidth_node,
> -                                def->type == VIR_DOMAIN_NET_TYPE_NETWORK) < 0)
> -        goto error;
> +    if (bandwidth_node) {
> +        bool allowFloor =
> +            (actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) ||
> +            (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
> +             actual->data.bridge.brname != NULL);
> +        if (virNetDevBandwidthParse(&actual->bandwidth,
> +                                    bandwidth_node,
> +                                    allowFloor) < 0)
> +            goto error;
> +    }
>   
>       vlanNode = virXPathNode("./vlan", ctxt);
>       if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &actual->vlan) < 0)
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6ed0bf1e8e..4055939ade 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4491,11 +4491,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
> @@ -5454,9 +5450,8 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface,
>       virNetDevBandwidthPtr ifaceBand;
>       unsigned long long old_floor, new_floor;
>   
> -    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
> -        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> -         iface->data.network.actual->data.bridge.brname == NULL)) {
> +    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> +        iface->data.network.actual->data.bridge.brname == NULL) {
>           /* This is not an interface that's plugged into a network.
>            * We don't care. Thus from our POV bandwidth change is allowed. */
>           return false;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c443c881d5..31d8647eee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4567,7 +4567,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
>           syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
>       }
>   
> -    if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +    if (virDomainNetGetActualType(def) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>           const char *brname = virDomainNetGetActualBridgeName(def);
>   
>           /* For libivrt network connections, set the following TUN/TAP network

This at first seemed really short since there are many more places that 
check for actualtype=network, but you add an error to all those places  
in the next patch, which makes sense.


Assuming that you fix the xml2xml test cases and move the chunk that got 
merged into the wrong place:


Reviewed-by: Laine Stump <laine at laine.org>