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

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 10:16:25PM -0500, Laine Stump wrote:
> 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 at redhat.com>
> > ---
> >   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.

In terms of this patch the change is a no-op as nothing is taking any
action based on the 'startup' bool value.

The last patch in this series does, however, look at the startup value.

If startup is true, then it will try to delete the legacy libvirt rules
from the built-in chains. This only ever needs to be done at startup,
because you'll only have the legacy rules present if you've just live
upgraded from old libvirt.  If the callback is triggered from a firewalld
restart, then you know there won't be any legacy rules, as libvirtd will
have already deleted them when it first started.

IOW, we are safe wrt firewalld restarts, even if it does delete all
rules during restart.

> So, since your method seems to work, and this code is all very
> straightforward:
> 
> 
> Reviewed-by: Laine Stump <laine at laine.org>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|