9:32 p.m.
While testing a recent patch that unsets the zone of bridge interfaces
when a virtual network is stopped, a side effect was noticed: when
firewalld reloaded its rules, this would result in an error log from
libvirt complaining about attempting to unset the zone of an interface
that wasn't in any zone. The two patches here fix that from different
angles:
* The first modifies the call to unsetZone so that it puts any error
message returned from firewalld to libvirt into a virError object
rather than logging it; this virError object is then silently
discarded.
* The second avoids even calling firewalld to unset the zone if it's
just going to immediately be set again. This avoids an error message
that would be logged directly by firewalld even if libvirt didn't
log the message it received from firewalld.
The combination of these two patches eliminate all misleading log
messages about failed attempts to unset a zone.
Laine Stump (2):
network: ignore/don't log errors when unsetting firewalld zone
network: don't unset the firewalld zone if it's going to be
immediately re-set
src/network/bridge_driver.c | 8 +++----
src/network/bridge_driver_linux.c | 10 +++++----
src/network/bridge_driver_nop.c | 4 +++-
src/network/bridge_driver_platform.h | 3 ++-
src/util/virfirewalld.c | 33 ++++++++++++++++++----------
src/util/virfirewalld.h | 2 +-
6 files changed, 38 insertions(+), 22 deletions(-)
--
2.47.0
9:32 p.m.
New subject: [PATCH 1/2] network: ignore/don't log errors when unsetting firewalld zone
The most common "error" when trying to unset the firewalld zone of an
interface is for firewalld to tell us that the interface already isn't
in any zone. Since this is what we want, no need to alarm the user by
logging it as an error.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virfirewalld.c | 33 ++++++++++++++++++++++-----------
src/util/virfirewalld.h | 2 +-
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
index ca61ed5ac0..0a886780ad 100644
--- a/src/util/virfirewalld.c
+++ b/src/util/virfirewalld.c
@@ -449,26 +449,37 @@ virFirewallDInterfaceSetZone(const char *iface,
}
-int
+void
virFirewallDInterfaceUnsetZone(const char *iface)
{
GDBusConnection *sysbus = virGDBusGetSystemBus();
g_autoptr(GVariant) message = NULL;
+ g_autoptr(virError) error = NULL;
if (!sysbus)
- return -1;
+ return;
+
+ /* we are sending virGDBusCallMethod an error object so that it
+ * will put the error message there rather than logging it,
+ * because we want to ignore any error as it doesn't matter - the
+ * most common "error" is to inform us that the interface is
+ * already not in any zone, and that is of course just fine, since
+ * that's what we're trying to do anyway. If there is an error,
+ * we'll just throw it away without logging it anywhere.
+ */
+ error = g_new0(virError, 1);
message = g_variant_new("(ss)", "", iface);
- return virGDBusCallMethod(sysbus,
- NULL,
- NULL,
- NULL,
- VIR_FIREWALL_FIREWALLD_SERVICE,
- "/org/fedoraproject/FirewallD1",
- "org.fedoraproject.FirewallD1.zone",
- "removeInterface",
- message);
+ virGDBusCallMethod(sysbus,
+ NULL,
+ NULL,
+ error,
+ VIR_FIREWALL_FIREWALLD_SERVICE,
+ "/org/fedoraproject/FirewallD1",
+ "org.fedoraproject.FirewallD1.zone",
+ "removeInterface",
+ message);
}
diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h
index 0dbe66d435..43803ee89a 100644
--- a/src/util/virfirewalld.h
+++ b/src/util/virfirewalld.h
@@ -46,6 +46,6 @@ int virFirewallDApplyRule(virFirewallLayer layer,
int virFirewallDInterfaceSetZone(const char *iface,
const char *zone);
-int virFirewallDInterfaceUnsetZone(const char *iface);
+void virFirewallDInterfaceUnsetZone(const char *iface);
void virFirewallDSynchronize(void);
--
2.47.0
9:32 p.m.
New subject: [PATCH 2/2] network: don't unset the firewalld zone if it's going to be immediately re-set
Any time the firewalld zone for an interface is set, by definition
that removes it from any previous zone that it was in, so there is
really no point in unsetting the zone if it's just going to be
immediately set again.
(incoming "weave" - it meanders a bit, but then ties together into a
point. Bigly.)
This is useful because when firewalld reloads its rules, 3 things happen:
1) firewalld flushes *all* firewall rules (including those added by libvirt)
2) firewalld unsets the zones for all interfaces (including those set
by libvirt)
3) firewalld re-adds its own rules, and sets the zone for all the
interfaces it manages
4) firewalld sends a dbus message that libvirt is watching for, and
when libvirt receives that message, it reloads all of the
libvirt-generated rules, and also re-sets the firewalld zone for
the bridge interfaces managed by libvirt.
libvirt accomplishes step 4 by a) calling
networkRemoveFirewallRules(), and then b) calling
networkAddFirewallRules(). But (because it is useful in other
contexts) networkRemoveFirewallRules() will attempt to *unset* the
zone for each bridge interface, and when firewalld receives this
request, it will that the bridge interface *has no zone* (because it
was unset by firewalld in step (2) above), and thus logs an error
message.
There is no way for libvirt to suppress an error message that is
logged by firewalld when a request to firewalld fails. But what
libvirt *can* do is realize that in these cases, the firewalld zone is
about to be set again anyway, and so we don't need to call firewalld
to unset the zone in the first place.
This patch handles that by adding a bool unsetZone to the arguments of
networkRemoveFirewallRules(); most calls to networkRemoveFirewallRules()
have unsetZone=true, but in two cases where the zone is about to be
reset, networkRemoveFirewallRules() is called with unsetZone=false,
which prevents the call to virFirewallDInterfaceUnsetZone() and thus
avoids the unnecessary (and confusing to users!) error message that
would have been logged by firewalld.
(see - that weave ended up sewed together, right?)
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 8 ++++----
src/network/bridge_driver_linux.c | 10 ++++++----
src/network/bridge_driver_nop.c | 4 +++-
src/network/bridge_driver_platform.h | 3 ++-
4 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 550759881a..d408f17de7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1752,7 +1752,7 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
* and this functionality is also handled by
* networkAdd/RemoveFirewallRules()
*/
- networkRemoveFirewallRules(obj);
+ networkRemoveFirewallRules(obj, false);
ignore_value(networkAddFirewallRules(def, cfg->firewallBackend,
&fwRemoval));
virNetworkObjSetFwRemoval(obj, fwRemoval);
saveStatus = true;
@@ -2129,7 +2129,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
ignore_value(virNetDevSetOnline(def->bridge, false));
if (firewalRulesAdded)
- networkRemoveFirewallRules(obj);
+ networkRemoveFirewallRules(obj, true);
virNetworkObjUnrefMacMap(obj);
@@ -2166,7 +2166,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj)
ignore_value(virNetDevSetOnline(def->bridge, false));
- networkRemoveFirewallRules(obj);
+ networkRemoveFirewallRules(obj, true);
ignore_value(virNetDevBridgeDelete(def->bridge));
@@ -3332,7 +3332,7 @@ networkUpdate(virNetworkPtr net,
* old rules (and remember to load new ones after the
* update).
*/
- networkRemoveFirewallRules(obj);
+ networkRemoveFirewallRules(obj, false);
needFirewallRefresh = true;
break;
default:
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 6c3ec403a4..86f6a5915f 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -447,7 +447,8 @@ networkAddFirewallRules(virNetworkDef *def,
void
-networkRemoveFirewallRules(virNetworkObj *obj)
+networkRemoveFirewallRules(virNetworkObj *obj,
+ bool unsetZone)
{
virNetworkDef *def = virNetworkObjGetDef(obj);
virFirewall *fw;
@@ -484,9 +485,10 @@ networkRemoveFirewallRules(virNetworkObj *obj)
* same interface name wants *no* zone set. To avoid this, we must
* "unset" the zone if we set it when the network was started.
*/
- if (virFirewallDIsRegistered() == 0 &&
- (def->forward.type != VIR_NETWORK_FORWARD_OPEN ||
- def->bridgeZone)) {
+ if (unsetZone
+ && virFirewallDIsRegistered() == 0
+ && (def->forward.type != VIR_NETWORK_FORWARD_OPEN
+ || def->bridgeZone)) {
VIR_DEBUG("unsetting zone for '%s' (current zone is
'%s')",
def->bridge, def->bridgeZone);
diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 8bf3367bff..59fc0e3c96 100644
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -56,6 +56,8 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
return 0;
}
-void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED)
+void
+networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED,
+ bool unsetZone G_GNUC_UNUSED)
{
}
diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
index cd2e3fa7b5..6a393c9733 100644
--- a/src/network/bridge_driver_platform.h
+++ b/src/network/bridge_driver_platform.h
@@ -36,4 +36,5 @@ int networkAddFirewallRules(virNetworkDef *def,
virFirewallBackend firewallBackend,
virFirewall **fwRemoval);
-void networkRemoveFirewallRules(virNetworkObj *obj);
+void networkRemoveFirewallRules(virNetworkObj *obj,
+ bool unsetZone);
--
2.47.0
8:30 a.m.
New subject: [PATCH 0/2] network: avoid logging unnecessary and misleading
errors when failing to unset a zone
On a Monday in 2024, Laine Stump wrote:
While testing a recent patch that unsets the zone of bridge
interfaces
when a virtual network is stopped, a side effect was noticed: when
firewalld reloaded its rules, this would result in an error log from
libvirt complaining about attempting to unset the zone of an interface
that wasn't in any zone. The two patches here fix that from different
angles:
* The first modifies the call to unsetZone so that it puts any error
message returned from firewalld to libvirt into a virError object
rather than logging it; this virError object is then silently
discarded.
* The second avoids even calling firewalld to unset the zone if it's
just going to immediately be set again. This avoids an error message
that would be logged directly by firewalld even if libvirt didn't
log the message it received from firewalld.
The combination of these two patches eliminate all misleading log
messages about failed attempts to unset a zone.
Laine Stump (2):
network: ignore/don't log errors when unsetting firewalld zone
network: don't unset the firewalld zone if it's going to be
immediately re-set
src/network/bridge_driver.c | 8 +++----
src/network/bridge_driver_linux.c | 10 +++++----
src/network/bridge_driver_nop.c | 4 +++-
src/network/bridge_driver_platform.h | 3 ++-
src/util/virfirewalld.c | 33 ++++++++++++++++++----------
src/util/virfirewalld.h | 2 +-
6 files changed, 38 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
32
days inactive
34
days old
3 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump