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

Laine Stump posted 1 patch 4 years, 11 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190412153513.1782-1-laine@laine.org
There is a newer version of this series
src/network/bridge_driver.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
[libvirt] [PATCH] network: only reload firewall after firewalld is finished restarting
Posted by Laine Stump 4 years, 11 months ago
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@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"))
     {
-        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;
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: only reload firewall after firewalld is finished restarting
Posted by Daniel P. Berrangé 4 years, 11 months ago
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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: only reload firewall after firewalld is finished restarting
Posted by Laine Stump 4 years, 11 months ago
On 4/12/19 11:57 AM, Daniel P. Berrangé wrote:
> 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@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.


Ah, right. Okay, time for V2.


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