Current implementation uses a single command to flush the old rules and
create new ones. This is not optimal because if flush fails for some
non-critical reasons (e.g. because the anchor didn't previously exist),
it will block rules creation and network start.

Split this command into two: one for flush, and one for rules creation.
Also, don't fail if the flush command fails.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
---
 src/network/network_pf.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/network/network_pf.c b/src/network/network_pf.c
index ce4461c999..a39eef5fa2 100644
--- a/src/network/network_pf.c
+++ b/src/network/network_pf.c
@@ -168,6 +168,7 @@ pfAddNatFirewallRules(virNetworkDef *def,
     g_autofree const char *forwardIf = g_strdup(virNetworkDefForwardIf(def, 0));
     g_auto(virBuffer) pf_rules_buf = VIR_BUFFER_INITIALIZER;
     g_autoptr(virCommand) cmd = virCommandNew(PFCTL);
+    g_autoptr(virCommand) flush_cmd = virCommandNew(PFCTL);
     virPortRange *portRange = &def->forward.port;
     g_autofree char *portRangeStr = NULL;
 
@@ -240,13 +241,25 @@ pfAddNatFirewallRules(virNetworkDef *def,
                       "block on %s\n",
                       def->bridge);
 
-    /*  pfctl -a libvirt/default -F all -f - */
+    /* pfctl -a libvirt/default -f - */
     virCommandAddArg(cmd, "-a");
     virCommandAddArgFormat(cmd, "libvirt/%s", def->name);
-    virCommandAddArgList(cmd, "-F", "all", "-f", "-", NULL);
+    virCommandAddArgList(cmd, "-f", "-", NULL);
 
     virCommandSetInputBuffer(cmd, virBufferContentAndReset(&pf_rules_buf));
 
+    /* pfctl -a libvirt/default -F all */
+    /* Flush rules as a separate command, so when it fails, e.g. because the
+     * anchor didn't exist, we still proceed with rules creation */
+    virCommandAddArg(flush_cmd, "-a");
+    virCommandAddArgFormat(flush_cmd, "libvirt/%s", def->name);
+    virCommandAddArgList(flush_cmd, "-F", "all", NULL);
+
+    if (virCommandRun(flush_cmd, NULL) < 0) {
+        VIR_WARN("Failed to flush firewall rules for network %s",
+                 def->name);
+    }
+
     if (virCommandRun(cmd, NULL) < 0) {
         VIR_WARN("Failed to create firewall rules for network %s",
                  def->name);
-- 
2.51.0

On 10/5/25 2:58 AM, Roman Bogorodskiy wrote:
> Current implementation uses a single command to flush the old rules and
> create new ones. This is not optimal because if flush fails for some
> non-critical reasons (e.g. because the anchor didn't previously exist),
> it will block rules creation and network start.
> 
> Split this command into two: one for flush, and one for rules creation.
> Also, don't fail if the flush command fails.
> 
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/network/network_pf.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network/network_pf.c b/src/network/network_pf.c
> index ce4461c999..a39eef5fa2 100644
> --- a/src/network/network_pf.c
> +++ b/src/network/network_pf.c
> @@ -168,6 +168,7 @@ pfAddNatFirewallRules(virNetworkDef *def,
>       g_autofree const char *forwardIf = g_strdup(virNetworkDefForwardIf(def, 0));
>       g_auto(virBuffer) pf_rules_buf = VIR_BUFFER_INITIALIZER;
>       g_autoptr(virCommand) cmd = virCommandNew(PFCTL);
> +    g_autoptr(virCommand) flush_cmd = virCommandNew(PFCTL);
>       virPortRange *portRange = &def->forward.port;
>       g_autofree char *portRangeStr = NULL;
>   
> @@ -240,13 +241,25 @@ pfAddNatFirewallRules(virNetworkDef *def,
>                         "block on %s\n",
>                         def->bridge);
>   
> -    /*  pfctl -a libvirt/default -F all -f - */
> +    /* pfctl -a libvirt/default -f - */
>       virCommandAddArg(cmd, "-a");
>       virCommandAddArgFormat(cmd, "libvirt/%s", def->name);
> -    virCommandAddArgList(cmd, "-F", "all", "-f", "-", NULL);
> +    virCommandAddArgList(cmd, "-f", "-", NULL);
>   
>       virCommandSetInputBuffer(cmd, virBufferContentAndReset(&pf_rules_buf));
>   
> +    /* pfctl -a libvirt/default -F all */
> +    /* Flush rules as a separate command, so when it fails, e.g. because the
> +     * anchor didn't exist, we still proceed with rules creation */
> +    virCommandAddArg(flush_cmd, "-a");
> +    virCommandAddArgFormat(flush_cmd, "libvirt/%s", def->name);
> +    virCommandAddArgList(flush_cmd, "-F", "all", NULL);
> +
> +    if (virCommandRun(flush_cmd, NULL) < 0) {
> +        VIR_WARN("Failed to flush firewall rules for network %s",
> +                 def->name);
> +    }
> +
>       if (virCommandRun(cmd, NULL) < 0) {
>           VIR_WARN("Failed to create firewall rules for network %s",
>                    def->name);