Re: [PATCH v5 12/30] network: support setting firewallBackend from network.conf

[Laine Stump <laine@xxxxxxxxxx>]

On 5/22/24 10:36 AM, Daniel P. Berrangé wrote:
> On Mon, May 20, 2024 at 12:14:26PM -0400, Laine Stump wrote:
[...]
> > That is taken care of by [see [*] below], but I'll add this suggestion
> > anyway to reduce the brain cells required for someone reading through the
> > code :-) (and also as a backup in case the code at [*] gets changed in the
> > future).
>
> Yeah, the current code works, but is a bit confusing to me.
> Meanwhile
>
>    Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>

Okay, I'll squash this in before I push (to summarize, I add a variable 
for nFwBackends, then set it to 1 if a backend is specified in the 
config file, and finally I move the " else if (fwBackendStr)" clause 
(the code that's executed if a backend was set in the conf file and it 
failed) down below the loop (with the rest of the log messages).

diff --git a/src/network/bridge_driver_conf.c 
b/src/network/bridge_driver_conf.c
index a083fa68e6..8f4956dace 100644
--- a/src/network/bridge_driver_conf.c
+++ b/src/network/bridge_driver_conf.c
@@ -69,6 +69,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
G_GNUC_UNUSED,
     size_t i;
     int fwBackends[] = { FIREWALL_BACKEND_DEFAULT_1, 
FIREWALL_BACKEND_DEFAULT_2 };
     G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == 
VIR_FIREWALL_BACKEND_LAST);
+    int nFwBackends = G_N_ELEMENTS(fwBackends);

     if (access(filename, R_OK) == 0) {

@@ -83,6 +84,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
G_GNUC_UNUSED,

         if (fwBackendStr) {
             fwBackends[0] = 
virFirewallBackendTypeFromString(fwBackendStr);
+            nFwBackends = 1;

             if (fwBackends[0] < 0) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -95,7 +97,7 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg 
G_GNUC_UNUSED,
         }
     }

-    for (i = 0; i < G_N_ELEMENTS(fwBackends) && !fwBackendSelected; i++) {
+    for (i = 0; i < nFwBackends && !fwBackendSelected; i++) {

         switch ((virFirewallBackend)fwBackends[i]) {
         case VIR_FIREWALL_BACKEND_IPTABLES: {
@@ -119,24 +121,23 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig 
*cfg G_GNUC_UNUSED,
             return -1;
         }

-        if (fwBackendSelected) {
-
+        if (fwBackendSelected)
             cfg->firewallBackend = fwBackends[i];
-
-        } else if (fwBackendStr) {
-
-            /* explicitly requested backend not found - this is a 
failure */
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("requested firewall_backend '%1$s' is not 
available"),
-                           virFirewallBackendTypeToString(fwBackends[i]));
-            return -1;
-        }
     }

     if (fwBackendSelected) {
         VIR_INFO("using firewall_backend: '%s'",
                  virFirewallBackendTypeToString(cfg->firewallBackend));
         return 0;
+
+    } else if (fwBackendStr) {
+
+        /* the explicitly requested backend wasn't found - this is a 
failure */
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("requested firewall_backend '%1$s' is not 
available"),
+                       fwBackendStr);
+        return -1;
+
     } else {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("could not find a usable firewall backend"));