[libvirt] [PATCH] network: only reload firewall after firewalld is finished restarting

[Daniel P. Berrangé]

On Fri, Apr 12, 2019 at 11:35:13AM -0400, Laine Stump wrote:
> The network driver used to reload the firewall rules whenever a dbus
> NameOwnerChanged message for org.fedoraproject.FirewallD1 was
> received. Presumably at some point in the past this was successful at
> reloading our rules after a firewalld restart. Recently though I
> noticed that once firewalld was restarted, libvirt's logs would get this
> message:
> 
>   The name org.fedoraproject.FirewallD1 was not provided by any .service files
> 
> After this point, no networks could be started until libvirtd itself
> was restarted.
> 
> The problem is that the NameOwnerChanged message is sent twice during
> a firewalld restart - once when the old firewalld is stopped, and
> again when the new firewalld is started. If we try to reload at the
> point the old firewalld is stopped, none of the firewalld dbus calls
> will succeed.
> 
> The solution is to check the new_owner field of the message - we
> should reload our firewall rules only if new_owner is non-empty (it is
> set to "" when firewalld is stopped, and some sort of epoch number
> when it is again started).
> 
> Signed-off-by: Laine Stump <laine at laine.org>
> ---
>  src/network/bridge_driver.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 4d4ab0f375..167c142ae2 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -549,8 +549,23 @@ firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED,
>          dbus_message_is_signal(message, "org.fedoraproject.FirewallD1",
>                                 "Reloaded"))

This code path can be run for 2 different signals. You must only do the
Decode step for the NamedOwnerChanged signal, not the Reloaded signal.

>      {
> -        VIR_DEBUG("Reload in bridge_driver because of firewalld.");
> -        networkReloadFirewallRules(driver, false);
> +        VIR_AUTOFREE(char *) name = NULL;
> +        VIR_AUTOFREE(char *) old_owner = NULL;
> +        VIR_AUTOFREE(char *) new_owner = NULL;
> +
> +        if (virDBusMessageDecode(message, "sss", &name, &old_owner, &new_owner) < 0) {
> +            VIR_WARN("Failed to decode DBus NameOwnerChanged message");
> +            return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +        }
> +
> +        /*
> +         * if new_owner is empty, firewalld is shutting down. If it is
> +         * non-empty, then it is starting
> +         */
> +        if (new_owner && *new_owner) {
> +            VIR_DEBUG("Reload in bridge_driver because of firewalld.");
> +            networkReloadFirewallRules(driver, false);
> +        }
>      }
>  
>      return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;

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 :|