[libvirt] [PATCH] network: allow <pf> together with <interface>/<address> in network status

[Laine Stump]

The function that parses the <forward> subelement of a network used to
fail/log an error if the network definition contained both a <pf>
element as well as at least one <interface> or <address> element. That
check was present because the configuration of a network should have
either one <pf>, one or more <interface>, or one or more <address>,
but never combinations of multiple kinds.

This caused a problem when libvirtd was restarted with a network
already active - when a network with a <pf> element is started, the
referenced PF (Physical Function of an SRIOV-capable network card) is
checked for VFs (Virtual Functions), and the <forward> is filled in
with a list of all VFs for that PF either in the form of their PCI
addresses (a list of <address>) or their netdev names (a list of
<interface>); the <pf> element is not removed though. When libvirtd is
restarted, it parses the network status and finds both the original
<pf> from the config, as well as the list of either <address> or
<interface>, fails the parse, and the network is not added to the
active list. This failure is often obscured because the network is
marked as autostart so libvirt immediately restarts it.

It seems odd to me that <interface> and <address> are stored in the
same array rather than keeping two separate arrays, and having
separate arrays would have made the check much simpler. However,
changing to use two separate arrays would have required changes in
more places, potentially creating more conflicts and (more
importantly) more possible regressions in the event of a backport, so
I chose to keep the existing data structure in order to localize the
change.

It appears that this problem has been in the code ever since support
for <pf> was added (0.9.10), but until commit
34cc3b2f106e296df5e64309620c79d16fd76c85 (first in libvirt 1.2.4)
networks with interface pools were not properly marked as active on
restart anyway, so there is no point in backporting this patch any
further than that.
---
 src/conf/network_conf.c     | 10 +---------
 src/network/bridge_driver.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index f947d89..dce3360 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1,7 +1,7 @@
 /*
  * network_conf.c: network XML handling
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -1638,14 +1638,6 @@ virNetworkForwardDefParseXML(const char *networkName,
             goto cleanup;
     }
 
-    if (((nForwardIfs > 0) + (nForwardAddrs > 0) + (nForwardPfs > 0)) > 1) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       _("<address>, <interface>, and <pf> elements in <forward> "
-                         "of network %s are mutually exclusive"),
-                       networkName);
-        goto cleanup;
-    }
-
     forwardDev = virXPathString("string(./@dev)", ctxt);
     if (forwardDev && (nForwardAddrs > 0 || nForwardPfs > 0)) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 2798010..af16c25 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2734,6 +2734,7 @@ networkValidate(virNetworkDefPtr def,
     virNetworkIpDefPtr ipdef;
     bool ipv4def = false, ipv6def = false;
     bool bandwidthAllowed = true;
+    bool usesInterface = false, usesAddress = false;
 
     /* check for duplicate networks */
     if (virNetworkObjIsDuplicate(&driver->networks, def, check_active) < 0)
@@ -2798,6 +2799,40 @@ networkValidate(virNetworkDefPtr def,
         bandwidthAllowed = false;
     }
 
+    /* we support configs with a single PF defined:
+     *   <pf dev='eth0'/>
+     * or with a list of netdev names:
+     *   <interface dev='eth9'/>
+     * OR a list of PCI addresses
+     *   <address type='pci' domain='0' bus='4' slot='0' function='1'/>
+     * but not any combination of those.
+     *
+     * Since <interface> and <address> are for some strange reason
+     * stored in the same array, we need to cycle through it and check
+     * the type of each.
+     */
+    for (i = 0; i < def->forward.nifs; i++) {
+        switch ((virNetworkForwardHostdevDeviceType)
+                def->forward.ifs[i].type) {
+        case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV:
+            usesInterface = true;
+            break;
+        case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI:
+            usesAddress = true;
+            break;
+        case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NONE:
+        case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_LAST:
+            break;
+        }
+    }
+    if ((def->forward.npfs > 0) + usesInterface + usesAddress > 1) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("<address>, <interface>, and <pf> elements of "
+                         "<forward> in network %s are mutually exclusive"),
+                       def->name);
+        return -1;
+    }
+
     /* We only support dhcp on one IPv4 address and
      * on one IPv6 address per defined network
      */
-- 
2.1.0