We know at the time a virFirewallRule is created (with
virFirewallAddRule*()) whether or not we will later want to ignore
errors encountered when attempting to apply that rule - if
ignoreErrors is set in the AddRule or if the group has already had
VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS set, then we ignore the errors.

Rather than setting the rule->ignoreErrors rule only according to the
arg sent to virFirewallAddRuleFull(), and then later (at
ApplyRule-time) combining that with the group transactionFlags setting
(and passing it all the way down the call chain), just combine the two
flags right away and store this final value in rule->ignoreErrors when
the rule is created (thus avoiding the need to look at anything other
than rule->ignoreErrors at the time the rule is applied). And since we
now have an API for retrieving the setting of ignoreErrors from a
rule, just grab that with the API down in vir*ApplyRule() rather than
cluttering up the argument list on the entire call chain.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/util/virfirewall.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 15c8db3702..e3ba8f7846 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -211,14 +211,20 @@ virFirewallAddRuleFullV(virFirewall *firewall,
     rule->layer = layer;
     rule->queryCB = cb;
     rule->queryOpaque = opaque;
-    rule->ignoreErrors = ignoreErrors;
 
     while ((str = va_arg(args, char *)) != NULL)
         ADD_ARG(rule, str);
 
     if (group->addingRollback) {
+        rule->ignoreErrors = true; /* always ignore errors when rolling back */
         VIR_APPEND_ELEMENT_COPY(group->rollback, group->nrollback, rule);
     } else {
+        /* when not rolling back, ignore errors if this group (transaction)
+         * was started with VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS *or*
+         * if this specific rule was created with ignoreErrors == true
+         */
+        rule->ignoreErrors = ignoreErrors
+            || (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
         VIR_APPEND_ELEMENT_COPY(group->action, group->naction, rule);
     }
 
@@ -496,7 +502,6 @@ virFirewallRuleToString(const char *cmd,
 
 static int
 virFirewallApplyRuleDirect(virFirewallRule *rule,
-                           bool ignoreErrors,
                            char **output)
 {
     size_t i;
@@ -541,7 +546,7 @@ virFirewallApplyRuleDirect(virFirewallRule *rule,
         return -1;
 
     if (status != 0) {
-        if (ignoreErrors) {
+        if (virFirewallRuleGetIgnoreErrors(rule)) {
             VIR_DEBUG("Ignoring error running command");
         } else {
             virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -558,16 +563,12 @@ virFirewallApplyRuleDirect(virFirewallRule *rule,
 
 static int
 virFirewallApplyRule(virFirewall *firewall,
-                     virFirewallRule *rule,
-                     bool ignoreErrors)
+                     virFirewallRule *rule)
 {
     g_autofree char *output = NULL;
     g_auto(GStrv) lines = NULL;
 
-    if (rule->ignoreErrors)
-        ignoreErrors = rule->ignoreErrors;
-
-    if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0)
+    if (virFirewallApplyRuleDirect(rule, &output) < 0)
         return -1;
 
     if (rule->queryCB && output) {
@@ -594,7 +595,7 @@ virFirewallApplyGroup(virFirewall *firewall,
                       size_t idx)
 {
     virFirewallGroup *group = firewall->groups[idx];
-    bool ignoreErrors = (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
+
     size_t i;
 
     VIR_INFO("Starting transaction for firewall=%p group=%p flags=0x%x",
@@ -602,9 +603,7 @@ virFirewallApplyGroup(virFirewall *firewall,
     firewall->currentGroup = idx;
     group->addingRollback = false;
     for (i = 0; i < group->naction; i++) {
-        if (virFirewallApplyRule(firewall,
-                                 group->action[i],
-                                 ignoreErrors) < 0)
+        if (virFirewallApplyRule(firewall, group->action[i]) < 0)
             return -1;
     }
     return 0;
@@ -621,11 +620,8 @@ virFirewallRollbackGroup(virFirewall *firewall,
     VIR_INFO("Starting rollback for group %p", group);
     firewall->currentGroup = idx;
     group->addingRollback = true;
-    for (i = 0; i < group->nrollback; i++) {
-        ignore_value(virFirewallApplyRule(firewall,
-                                          group->rollback[i],
-                                          true));
-    }
+    for (i = 0; i < group->nrollback; i++)
+        ignore_value(virFirewallApplyRule(firewall, group->rollback[i]));
 }
 
 
-- 
2.39.2

On Sun, Apr 30, 2023 at 11:19:18PM -0400, Laine Stump wrote:
> We know at the time a virFirewallRule is created (with
> virFirewallAddRule*()) whether or not we will later want to ignore
> errors encountered when attempting to apply that rule - if
> ignoreErrors is set in the AddRule or if the group has already had
> VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS set, then we ignore the errors.
> 
> Rather than setting the rule->ignoreErrors rule only according to the
> arg sent to virFirewallAddRuleFull(), and then later (at
> ApplyRule-time) combining that with the group transactionFlags setting
> (and passing it all the way down the call chain), just combine the two
> flags right away and store this final value in rule->ignoreErrors when
> the rule is created (thus avoiding the need to look at anything other
> than rule->ignoreErrors at the time the rule is applied). And since we
> now have an API for retrieving the setting of ignoreErrors from a
> rule, just grab that with the API down in vir*ApplyRule() rather than
> cluttering up the argument list on the entire call chain.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/util/virfirewall.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

On 5/1/23 05:19, Laine Stump wrote:
> We know at the time a virFirewallRule is created (with
> virFirewallAddRule*()) whether or not we will later want to ignore
> errors encountered when attempting to apply that rule - if
> ignoreErrors is set in the AddRule or if the group has already had
> VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS set, then we ignore the errors.
> 
> Rather than setting the rule->ignoreErrors rule only according to the
> arg sent to virFirewallAddRuleFull(), and then later (at
> ApplyRule-time) combining that with the group transactionFlags setting
> (and passing it all the way down the call chain), just combine the two
> flags right away and store this final value in rule->ignoreErrors when
> the rule is created (thus avoiding the need to look at anything other
> than rule->ignoreErrors at the time the rule is applied). And since we
> now have an API for retrieving the setting of ignoreErrors from a
> rule, just grab that with the API down in vir*ApplyRule() rather than
> cluttering up the argument list on the entire call chain.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/util/virfirewall.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index 15c8db3702..e3ba8f7846 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -211,14 +211,20 @@ virFirewallAddRuleFullV(virFirewall *firewall,
>      rule->layer = layer;
>      rule->queryCB = cb;
>      rule->queryOpaque = opaque;
> -    rule->ignoreErrors = ignoreErrors;
>  
>      while ((str = va_arg(args, char *)) != NULL)
>          ADD_ARG(rule, str);
>  
>      if (group->addingRollback) {
> +        rule->ignoreErrors = true; /* always ignore errors when rolling back */
>          VIR_APPEND_ELEMENT_COPY(group->rollback, group->nrollback, rule);
>      } else {
> +        /* when not rolling back, ignore errors if this group (transaction)
> +         * was started with VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS *or*
> +         * if this specific rule was created with ignoreErrors == true
> +         */
> +        rule->ignoreErrors = ignoreErrors
> +            || (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);

Nit pick - we usually put logical operands at the end of previous line.

>          VIR_APPEND_ELEMENT_COPY(group->action, group->naction, rule);
>      }
>  

Michal

On 5/2/23 11:15 AM, Michal Prívozník wrote:
> On 5/1/23 05:19, Laine Stump wrote:
>> +        rule->ignoreErrors = ignoreErrors
>> +            || (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
> 
> Nit pick - we usually put logical operands at the end of previous line.

A *very* long time ago (left that company in 1998) I worked with a group 
of people who put the operators at the beginning of the line in 
multiline expressions. Ever since then everyone I've worked with has 
been the opposite, but once or twice a year when I'm "programming while 
tired", my brain gets confused about which style was "that was then" and 
which style is "this is now".