On 3/18/19 6:47 PM, Daniel P. Berrangé wrote:
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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver.c | 3 +-- src/network/bridge_driver_linux.c | 29 ++++++++++++++++++++++------ src/network/bridge_driver_nop.c | 3 +-- src/network/bridge_driver_platform.h | 2 +- 4 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6789dafd15..27d7d072ce 100644 --- 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 b10d0a6c4d..04b9c079ff 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 ret;
Nitpick, this should be 'rc' or 'rv' or something like that. I view 'ret' as a variable that holds the retval of a function and this is not such case.
+ + /* 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 + */ + ret = iptablesSetupPrivateChains(); + if (ret < 0) { + errInit = virSaveLastError(); + virResetLastError(); + }
ACK Michal