[libvirt] [PATCH v2 0/5] Remove the last "no_memory" label
This series touches virfirewall.c, the last place where a no_memory label can be found. The series: - Gets rid of setting and checking for ENOMEM as a firewall's error; - Use g_auto / g_autofree in a few different places; - Adapt ADD_ARG() macro as VIR_RESIZE_N() just aborts in case of OOM; - Remove no_memory mention from the hacking guide; Changes since v1: https://www.redhat.com/archives/libvir-list/2020-January/msg00018.html - Daniel suggested to get rid of ADD_ARG(). Instead of doing so, I've adapted the macro (as it does more than just a realloc) and, by doing so, got rid of the no_memory labels; - Dropped: - util: Rename 'no_memory' label to 'cleanup' - util: Add ADD_ARG_RETURN_ON_ERROR - util: Add ADD_ARG_RETURN_VALUE_ON_ERROR - Added: - util: Adapt ADD_ARG() macro Fabiano Fidêncio (5): util: Don't set/check for ENOMEM as a firewall error util: Use g_auto/g_autofree in virFirewallAddRuleFullV() util: Use g_auto in virFirewallStartTransaction() util: Adapt ADD_ARG() macro docs: Remove mention to no_memory label docs/hacking.html.in | 1 - src/util/virfirewall.c | 65 +++++++++++------------------------------- 2 files changed, 16 insertions(+), 50 deletions(-) -- 2.24.1
As libvirt decided to take the path to not report OOM and simply abort when it happens, let's not set nor check for ENOMEM firewall's error and simplify the code whenever it's possible. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index ee72b579e4..6f7b5306e5 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -391,7 +391,6 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, return rule; no_memory: - firewall->err = ENOMEM; virFirewallRuleFree(rule); return NULL; } @@ -492,10 +491,8 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, ADD_ARG(rule, arg); - return; - no_memory: - firewall->err = ENOMEM; + return; } @@ -514,10 +511,8 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, ADD_ARG(rule, arg); - return; - no_memory: - firewall->err = ENOMEM; + return; } @@ -532,10 +527,8 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, args++; } - return; - no_memory: - firewall->err = ENOMEM; + return; } @@ -553,12 +546,7 @@ void virFirewallRuleAddArgList(virFirewallPtr firewall, while ((str = va_arg(list, char *)) != NULL) ADD_ARG(rule, str); - va_end(list); - - return; - no_memory: - firewall->err = ENOMEM; va_end(list); } @@ -591,15 +579,13 @@ void virFirewallStartTransaction(virFirewallPtr firewall, VIR_FIREWALL_RETURN_IF_ERROR(firewall); - if (!(group = virFirewallGroupNew())) { - firewall->err = ENOMEM; + if (!(group = virFirewallGroupNew())) return; - } + group->actionFlags = flags; if (VIR_EXPAND_N(firewall->groups, firewall->ngroups, 1) < 0) { - firewall->err = ENOMEM; virFirewallGroupFree(group); return; } @@ -747,10 +733,6 @@ virFirewallApplyRule(virFirewallPtr firewall, if (rule->queryCB(firewall, rule->layer, (const char *const *)lines, rule->queryOpaque) < 0) return -1; - if (firewall->err == ENOMEM) { - virReportOOMError(); - return -1; - } if (firewall->err) { virReportSystemError(firewall->err, "%s", _("Unable to create rule")); @@ -818,7 +800,7 @@ virFirewallApply(virFirewallPtr firewall) _("Failed to initialize a valid firewall backend")); goto cleanup; } - if (!firewall || firewall->err == ENOMEM) { + if (!firewall) { virReportOOMError(); goto cleanup; } -- 2.24.1
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 6f7b5306e5..2177617ecf 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -257,6 +257,7 @@ virFirewallRuleFree(virFirewallRulePtr rule) VIR_FREE(rule); } +G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virFirewallRulePtr, virFirewallRuleFree, NULL); static void virFirewallGroupFree(virFirewallGroupPtr group) @@ -335,8 +336,8 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, va_list args) { virFirewallGroupPtr group; - virFirewallRulePtr rule; - char *str; + g_auto(virFirewallRulePtr) rule = NULL; + g_autofree char *str = NULL; VIR_FIREWALL_RETURN_NULL_IF_ERROR(firewall); @@ -348,7 +349,7 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, if (VIR_ALLOC(rule) < 0) - goto no_memory; + return NULL; rule->layer = layer; rule->queryCB = cb; @@ -379,19 +380,18 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, if (VIR_APPEND_ELEMENT_COPY(group->rollback, group->nrollback, rule) < 0) - goto no_memory; + return NULL; } else { if (VIR_APPEND_ELEMENT_COPY(group->action, group->naction, rule) < 0) - goto no_memory; + return NULL; } - return rule; + return g_steal_pointer(&rule); no_memory: - virFirewallRuleFree(rule); return NULL; } -- 2.24.1
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 2177617ecf..564e2fe0be 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -278,6 +278,7 @@ virFirewallGroupFree(virFirewallGroupPtr group) VIR_FREE(group); } +G_DEFINE_AUTO_CLEANUP_FREE_FUNC(virFirewallGroupPtr, virFirewallGroupFree, NULL); /** * virFirewallFree: @@ -575,7 +576,7 @@ size_t virFirewallRuleGetArgCount(virFirewallRulePtr rule) void virFirewallStartTransaction(virFirewallPtr firewall, unsigned int flags) { - virFirewallGroupPtr group; + g_auto(virFirewallGroupPtr) group = NULL; VIR_FIREWALL_RETURN_IF_ERROR(firewall); @@ -586,10 +587,9 @@ void virFirewallStartTransaction(virFirewallPtr firewall, if (VIR_EXPAND_N(firewall->groups, firewall->ngroups, 1) < 0) { - virFirewallGroupFree(group); return; } - firewall->groups[firewall->ngroups - 1] = group; + firewall->groups[firewall->ngroups - 1] = g_steal_pointer(&group); firewall->currentGroup = firewall->ngroups - 1; } -- 2.24.1
As VIR_RESIZE_N() macro already aborts in case of OOM, there's no reason to check for its output in the ADD_ARG() macro. By doing this, we can simply get rid of a all no_memory labels spread in the virfirewall.c file. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 564e2fe0be..7c8040880c 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -320,11 +320,9 @@ void virFirewallFree(virFirewallPtr firewall) #define ADD_ARG(rule, str) \ do { \ - if (VIR_RESIZE_N(rule->args, \ - rule->argsAlloc, \ - rule->argsLen, 1) < 0) \ - goto no_memory; \ - \ + ignore_value(VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1)); \ rule->args[rule->argsLen++] = g_strdup(str); \ } while (0) @@ -391,9 +389,6 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, return g_steal_pointer(&rule); - - no_memory: - return NULL; } @@ -491,9 +486,6 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); ADD_ARG(rule, arg); - - no_memory: - return; } @@ -511,9 +503,6 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, va_end(list); ADD_ARG(rule, arg); - - no_memory: - return; } @@ -527,9 +516,6 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, ADD_ARG(rule, *args); args++; } - - no_memory: - return; } @@ -547,7 +533,6 @@ void virFirewallRuleAddArgList(virFirewallPtr firewall, while ((str = va_arg(list, char *)) != NULL) ADD_ARG(rule, str); - no_memory: va_end(list); } -- 2.24.1
no_memory labels have been entirely removed from libvirt code base. Knowing that, let's also remove any mention to the label from our hacking guide. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- docs/hacking.html.in | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 74aba5d46b..90bd0ddc81 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -1491,7 +1491,6 @@ BAD: <pre> error: A path only taken upon return with an error code cleanup: A path taken upon return with success code + optional error - no_memory: A path only taken upon return with an OOM error code retry: If needing to jump upwards (e.g., retry on EINTR) </pre> -- 2.24.1
participants (1)
-
Fabiano Fidêncio