Re: [PATCH v2 1/4] network: add platform driver callbacks around firewall reload

[Laine Stump <laine@xxxxxxxxx>]

On 12/7/18 11:21 AM, Daniel P. Berrangé wrote:
> Allow the platform driver impls to run logic before and after the
> firewall reload process.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
>   src/network/bridge_driver.c          | 13 ++++++++-----
>   src/network/bridge_driver_linux.c    | 11 +++++++++++
>   src/network/bridge_driver_nop.c      | 11 +++++++++++
>   src/network/bridge_driver_platform.h |  3 +++
>   4 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 4bbc4f5a6d..11095bf974 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -165,7 +165,7 @@ static int
>   networkShutdownNetworkExternal(virNetworkObjPtr obj);
>   
>   static void
> -networkReloadFirewallRules(virNetworkDriverStatePtr driver);
> +networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup);
>   
>   static void
>   networkRefreshDaemons(virNetworkDriverStatePtr driver);
> @@ -553,7 +553,7 @@ firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED,
>                                  "Reloaded"))
>       {
>           VIR_DEBUG("Reload in bridge_driver because of firewalld.");
> -        networkReloadFirewallRules(driver);
> +        networkReloadFirewallRules(driver, false);


Okay, I think I get what you're doing here - you wanted a place to put 
code that would only be run once when libvirtd starts, and not when the 
rules are reloaded due to firewalld restarting, right? But doesn't a 
restart of firewalld also remove all the private chains?


Or did I miss the point? I guess I must have, because when I built with 
these patches and tried restarting firewalld, I still saw the private 
chains after the restart.


So, since your method seems to work, and this code is all very 
straightforward:


Reviewed-by: Laine Stump <laine@xxxxxxxxx>


>       }
>   
>       return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> @@ -753,7 +753,7 @@ networkStateInitialize(bool privileged,
>       virNetworkObjListPrune(network_driver->networks,
>                              VIR_CONNECT_LIST_NETWORKS_INACTIVE |
>                              VIR_CONNECT_LIST_NETWORKS_TRANSIENT);
> -    networkReloadFirewallRules(network_driver);
> +    networkReloadFirewallRules(network_driver, true);
>       networkRefreshDaemons(network_driver);
>   
>       network_driver->networkEventState = virObjectEventStateNew();
> @@ -829,7 +829,7 @@ networkStateReload(void)
>       virNetworkObjLoadAllConfigs(network_driver->networks,
>                                   network_driver->networkConfigDir,
>                                   network_driver->networkAutostartDir);
> -    networkReloadFirewallRules(network_driver);
> +    networkReloadFirewallRules(network_driver, false);
>       networkRefreshDaemons(network_driver);
>       virNetworkObjListForEach(network_driver->networks,
>                                networkAutostartConfig,
> @@ -2181,12 +2181,15 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj,
>   
>   
>   static void
> -networkReloadFirewallRules(virNetworkDriverStatePtr driver)
> +networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
>   {
>       VIR_INFO("Reloading iptables rules");
> +    if (networkPreReloadFirewallRules(startup) < 0)
> +        return;
>       virNetworkObjListForEach(driver->networks,
>                                networkReloadFirewallRulesHelper,
>                                NULL);
> +    networkPostReloadFirewallRules(startup);
>   }
>   
>   
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index fb09954b8f..5650e1e061 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -36,6 +36,17 @@ VIR_LOG_INIT("network.bridge_driver_linux");
>   
>   #define PROC_NET_ROUTE "/proc/net/route"
>   
> +int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +{
> +    return 0;
> +}
> +
> +
> +void networkPostReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +{
> +}
> +
> +
>   /* XXX: This function can be a lot more exhaustive, there are certainly
>    *      other scenarios where we can ruin host network connectivity.
>    * XXX: Using a proper library is preferred over parsing /proc
> diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
> index 5e1acd07b4..64ff768b2f 100644
> --- a/src/network/bridge_driver_nop.c
> +++ b/src/network/bridge_driver_nop.c
> @@ -21,6 +21,17 @@
>   
>   #include <config.h>
>   
> +int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +{
> +    return 0;
> +}
> +
> +
> +void networkPostReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +{
> +}
> +
> +
>   int networkCheckRouteCollision(virNetworkDefPtr def ATTRIBUTE_UNUSED)
>   {
>       return 0;
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index 706000df4e..2589948933 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -60,6 +60,9 @@ struct _virNetworkDriverState {
>   typedef struct _virNetworkDriverState virNetworkDriverState;
>   typedef virNetworkDriverState *virNetworkDriverStatePtr;
>   
> +int networkPreReloadFirewallRules(bool startup);
> +void networkPostReloadFirewallRules(bool startup);
> +
>   int networkCheckRouteCollision(virNetworkDefPtr def);
>   
>   int networkAddFirewallRules(virNetworkDefPtr def);


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list