[libvirt] [PATCH] network: fix networkValidate check for default portgroup and vlan

[Laine Stump]

This was found during testing of the fix for:

   https://bugzilla.redhat.com/show_bug.cgi?id=868483

networkValidate was supposed to check for the existence of multiple
portgroups and report an error if this was encountered. It did, but
there were two problems:

1) even though it logged an error, it still returned success, allowing
the operation to continue.

2) It could exit the portgroup checking loop early (or possibly not
even do it once) if a vlan tag was supplied in the base network config
or one of the portgroups.

This patch fixes networkValidate to return failure in addition to
logging the error, and also changes it to not exit the portgroup
checking loop early. The logic was a bit off in the checking for vlan
anyway, and it's intertwined with fixing the early loop exit, so I
fixed that as well. Now it correctly checks for combinations where a
<virtualport> is specified in the base network def and <vlan> is given
in a portgroup, as well as the opposite (<vlan> in base network def
and <virtualport> in portgroup), and ignores the case of a disallowed
vlan when using *no* portgroup if there is a default portgroup (since
in that case there is no way to not use any portgroup).
---
 src/network/bridge_driver.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 8837843..f814f6f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2617,8 +2617,8 @@ static int
 networkValidate(virNetworkDefPtr def)
 {
     int ii;
-    bool vlanUsed, vlanAllowed;
-    const char *defaultPortGroup = NULL;
+    bool vlanUsed, vlanAllowed, badVlanUse = false;
+    virPortGroupDefPtr defaultPortGroup = NULL;
 
     /* The only type of networks that currently support transparent
      * vlan configuration are those using hostdev sr-iov devices from
@@ -2630,14 +2630,23 @@ networkValidate(virNetworkDefPtr def)
                    def->virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH);
 
     vlanUsed = def->vlan.nTags > 0;
-    for (ii = 0; ii < def->nPortGroups && !(vlanUsed && vlanAllowed); ii++) {
-        if (def->portGroups[ii].vlan.nTags > 0)
-            vlanUsed = true;
-        if (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE &&
-            def->portGroups[ii].virtPortProfile &&
-            (def->portGroups[ii].virtPortProfile->virtPortType
-             == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)) {
-            vlanAllowed = true;
+    for (ii = 0; ii < def->nPortGroups; ii++) {
+        if (vlanUsed || def->portGroups[ii].vlan.nTags > 0) {
+            /* anyone using this portgroup will get a vlan tag. Verify
+             * that they will also be using an openvswitch connection,
+             * as that is the only type of network that currently
+             * supports a vlan tag.
+             */
+            if (def->portGroups[ii].virtPortProfile) {
+                if (def->forwardType != VIR_NETWORK_FORWARD_BRIDGE ||
+                    def->portGroups[ii].virtPortProfile->virtPortType
+                    != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
+                    badVlanUse = true;
+                }
+            } else if (!vlanAllowed) {
+                /* virtualport taken from base network definition */
+                badVlanUse = true;
+            }
         }
         if (def->portGroups[ii].isDefault) {
             if (defaultPortGroup) {
@@ -2645,13 +2654,20 @@ networkValidate(virNetworkDefPtr def)
                                _("network '%s' has multiple default "
                                  "<portgroup> elements (%s and %s), "
                                  "but only one default is allowed"),
-                               def->name, defaultPortGroup,
+                               def->name, defaultPortGroup->name,
                                def->portGroups[ii].name);
+                return -1;
             }
-            defaultPortGroup = def->portGroups[ii].name;
+            defaultPortGroup = &def->portGroups[ii];
         }
     }
-    if (vlanUsed && !vlanAllowed) {
+    if (badVlanUse ||
+        (vlanUsed && !vlanAllowed && !defaultPortGroup)) {
+        /* NB: if defaultPortGroup is set, we don't directly look at
+         * vlanUsed && !vlanAllowed, because the network will never be
+         * used without having a portgroup added in, so all necessary
+         * checks were done in the loop above.
+         */
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("<vlan> element specified for network %s, "
                          "whose type doesn't support vlan configuration"),
-- 
1.7.11.7