network: improve error report when firewall chain creation fails

During startup we create some top level chains in which all
virtual network firewall rules will be placed. The upfront
creation is done to avoid slowing down creation of individual
virtual networks by checking for chain existance every time.

There are some factors which can cause this upfront creation
to fail and while a message will get into the libvirtd log
this won't be seen by users who later try to start a virtual
network. Instead they'll just get a message saying that the
libvirt top level chain does not exist. This message is
accurate, but unhelpful for solving the root cause.

This patch thus saves any error during daemon startup and
reports it when trying to create a virtual network later.

Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6789dafd157f6da501bc69fe5555602fa7211ea9..27d7d072ce344cac74b1385ff799bece7870b3e1 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2095,8 +2095,7 @@ static void
 networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
 {
     VIR_INFO("Reloading iptables rules");
-    if (networkPreReloadFirewallRules(startup) < 0)
-        return;
+    networkPreReloadFirewallRules(startup);
     virNetworkObjListForEach(driver->networks,
                              networkReloadFirewallRulesHelper,
                              NULL);

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index b10d0a6c4d70e04aebdc224e6b6cbb05707fec3b..c899f4b6d0ec45e6c49ab07669e1e9970c5f5c8c 100644 (file)
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -35,11 +35,25 @@ VIR_LOG_INIT("network.bridge_driver_linux");
 
 #define PROC_NET_ROUTE "/proc/net/route"
 
-int networkPreReloadFirewallRules(bool startup)
+static virErrorPtr errInit;
+
+void networkPreReloadFirewallRules(bool startup)
 {
-    int ret = iptablesSetupPrivateChains();
-    if (ret < 0)
-        return -1;
+    int rc;
+
+    /* We create global rules upfront as we don't want
+     * the perf hit of conditionally figuring out whether
+     * to create them each time a network is started.
+     *
+     * Any errors here are saved to be reported at time
+     * of starting the network though as that makes them
+     * more likely to be seen by a human
+     */
+    rc = iptablesSetupPrivateChains();
+    if (rc < 0) {
+        errInit = virSaveLastError();
+        virResetLastError();
+    }
 
     /*
      * If this is initial startup, and we just created the
@@ -54,10 +68,8 @@ int networkPreReloadFirewallRules(bool startup)
      * rules will be present. Thus we can safely just tell it
      * to always delete from the builin chain
      */
-    if (startup && ret == 1)
+    if (startup && rc == 1)
         iptablesSetDeletePrivate(false);
-
-    return 0;
 }
 
 
@@ -671,6 +683,11 @@ int networkAddFirewallRules(virNetworkDefPtr def)
     virFirewallPtr fw = NULL;
     int ret = -1;
 
+    if (errInit) {
+        virSetError(errInit);
+        return -1;
+    }
+
     if (def->bridgeZone) {
 
         /* if a firewalld zone has been specified, fail/log an error

diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index a0e57012f957bc059b7f4e1b793263c77eb9b8f0..ea9db338cb3c51fcdb7a17c19458967c66aa06ed 100644 (file)
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -19,9 +19,8 @@
 
 #include <config.h>
 
-int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
+void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
 {
-    return 0;
 }
 
 

diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
index baeb22bc3ed8fa2c944cdd4c1d5d9a650a47dc71..95fd64bdc7c50b97202a593b01f28d64195f40cb 100644 (file)
--- a/src/network/bridge_driver_platform.h
+++ b/src/network/bridge_driver_platform.h
@@ -58,7 +58,7 @@ struct _virNetworkDriverState {
 typedef struct _virNetworkDriverState virNetworkDriverState;
 typedef virNetworkDriverState *virNetworkDriverStatePtr;
 
-int networkPreReloadFirewallRules(bool startup);
+void networkPreReloadFirewallRules(bool startup);
 void networkPostReloadFirewallRules(bool startup);
 
 int networkCheckRouteCollision(virNetworkDefPtr def);