current/SOURCES/0003-network-improve-error-report-when-firewall-chain-cre.patch

From b990740b12117eaaf2797141a53a30b41f07c791 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
Date: Mon, 18 Mar 2019 17:31:21 +0000
Subject: [PATCH 3/5] network: improve error report when firewall chain
 creation fails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
(cherry picked from commit 9f4e35dc73ec9e940aa61bc7c140c2b800218ef3)
---
 src/network/bridge_driver.c          |  3 +--
 src/network/bridge_driver_linux.c    | 31 +++++++++++++++++++++-------
 src/network/bridge_driver_nop.c      |  3 +--
 src/network/bridge_driver_platform.h |  2 +-
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b3ca5b8a15..1da60f0a21 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2108,8 +2108,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 b10d0a6c4d..c899f4b6d0 100644
--- 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 a0e57012f9..ea9db338cb 100644
--- 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 baeb22bc3e..95fd64bdc7 100644
--- 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);
-- 
2.20.1

1	From b990740b12117eaaf2797141a53a30b41f07c791 Mon Sep 17 00:00:00 2001
2	From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
3	Date: Mon, 18 Mar 2019 17:31:21 +0000
4	Subject: [PATCH 3/5] network: improve error report when firewall chain
5	creation fails
6	MIME-Version: 1.0
7	Content-Type: text/plain; charset=UTF-8
8	Content-Transfer-Encoding: 8bit
9
10	During startup we create some top level chains in which all
11	virtual network firewall rules will be placed. The upfront
12	creation is done to avoid slowing down creation of individual
13	virtual networks by checking for chain existance every time.
14
15	There are some factors which can cause this upfront creation
16	to fail and while a message will get into the libvirtd log
17	this won't be seen by users who later try to start a virtual
18	network. Instead they'll just get a message saying that the
19	libvirt top level chain does not exist. This message is
20	accurate, but unhelpful for solving the root cause.
21
22	This patch thus saves any error during daemon startup and
23	reports it when trying to create a virtual network later.
24
25	Reviewed-by: Andrea Bolognani <abologna@redhat.com>
26	Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
27	(cherry picked from commit 9f4e35dc73ec9e940aa61bc7c140c2b800218ef3)
28	---
29	src/network/bridge_driver.c \| 3 +--
30	src/network/bridge_driver_linux.c \| 31 +++++++++++++++++++++-------
31	src/network/bridge_driver_nop.c \| 3 +--
32	src/network/bridge_driver_platform.h \| 2 +-
33	4 files changed, 27 insertions(+), 12 deletions(-)
34
35	diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
36	index b3ca5b8a15..1da60f0a21 100644
37	--- a/src/network/bridge_driver.c
38	+++ b/src/network/bridge_driver.c
39	@@ -2108,8 +2108,7 @@ static void
40	networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
41	{
42	VIR_INFO("Reloading iptables rules");
43	- if (networkPreReloadFirewallRules(startup) < 0)
44	- return;
45	+ networkPreReloadFirewallRules(startup);
46	virNetworkObjListForEach(driver->networks,
47	networkReloadFirewallRulesHelper,
48	NULL);
49	diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
50	index b10d0a6c4d..c899f4b6d0 100644
51	--- a/src/network/bridge_driver_linux.c
52	+++ b/src/network/bridge_driver_linux.c
53	@@ -35,11 +35,25 @@ VIR_LOG_INIT("network.bridge_driver_linux");
54
55	#define PROC_NET_ROUTE "/proc/net/route"
56
57	-int networkPreReloadFirewallRules(bool startup)
58	+static virErrorPtr errInit;
59	+
60	+void networkPreReloadFirewallRules(bool startup)
61	{
62	- int ret = iptablesSetupPrivateChains();
63	- if (ret < 0)
64	- return -1;
65	+ int rc;
66	+
67	+ /* We create global rules upfront as we don't want
68	+ * the perf hit of conditionally figuring out whether
69	+ * to create them each time a network is started.
70	+ *
71	+ * Any errors here are saved to be reported at time
72	+ * of starting the network though as that makes them
73	+ * more likely to be seen by a human
74	+ */
75	+ rc = iptablesSetupPrivateChains();
76	+ if (rc < 0) {
77	+ errInit = virSaveLastError();
78	+ virResetLastError();
79	+ }
80
81	/*
82	* If this is initial startup, and we just created the
83	@@ -54,10 +68,8 @@ int networkPreReloadFirewallRules(bool startup)
84	* rules will be present. Thus we can safely just tell it
85	* to always delete from the builin chain
86	*/
87	- if (startup && ret == 1)
88	+ if (startup && rc == 1)
89	iptablesSetDeletePrivate(false);
90	-
91	- return 0;
92	}
93
94
95	@@ -671,6 +683,11 @@ int networkAddFirewallRules(virNetworkDefPtr def)
96	virFirewallPtr fw = NULL;
97	int ret = -1;
98
99	+ if (errInit) {
100	+ virSetError(errInit);
101	+ return -1;
102	+ }
103	+
104	if (def->bridgeZone) {
105
106	/* if a firewalld zone has been specified, fail/log an error
107	diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
108	index a0e57012f9..ea9db338cb 100644
109	--- a/src/network/bridge_driver_nop.c
110	+++ b/src/network/bridge_driver_nop.c
111	@@ -19,9 +19,8 @@
112
113	#include <config.h>
114
115	-int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
116	+void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
117	{
118	- return 0;
119	}
120
121
122	diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
123	index baeb22bc3e..95fd64bdc7 100644
124	--- a/src/network/bridge_driver_platform.h
125	+++ b/src/network/bridge_driver_platform.h
126	@@ -58,7 +58,7 @@ struct _virNetworkDriverState {
127	typedef struct _virNetworkDriverState virNetworkDriverState;
128	typedef virNetworkDriverState *virNetworkDriverStatePtr;
129
130	-int networkPreReloadFirewallRules(bool startup);
131	+void networkPreReloadFirewallRules(bool startup);
132	void networkPostReloadFirewallRules(bool startup);
133
134	int networkCheckRouteCollision(virNetworkDefPtr def);
135	--
136	2.20.1
137