[libvirt] [PATCH 0/7] Remove the last "no_memory" label
This series touches virfirewall.c, the last place where a no_memory label can be found. The series: - Renames no_memory to cleanup; - Gets rid of ENOMEM error treatment; - Use g_auto / g_autofree in a few different places; - Add new macros as ADD_ARG heavily rely on a goto; - Remove no_memory mention from the hacking guide; I'm not totally sure whether the new macros addition are worth it. Fabiano Fidêncio (7): util: Rename 'no_memory' label to 'cleanup' 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: Add ADD_ARG_RETURN_ON_ERROR util: Add ADD_ARG_RETURN_VALUE_ON_ERROR docs: Remove mention to no_memory label docs/hacking.html.in | 1 - src/util/virfirewall.c | 94 +++++++++++++++++++----------------------- 2 files changed, 42 insertions(+), 53 deletions(-) -- 2.24.1
Let's rename the no_memory label to cleanup as the first step to remove the OOM handling in virfirewall.c file. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index ee72b579e4..c6a5de8842 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -321,7 +321,7 @@ void virFirewallFree(virFirewallPtr firewall) if (VIR_RESIZE_N(rule->args, \ rule->argsAlloc, \ rule->argsLen, 1) < 0) \ - goto no_memory; \ + goto cleanup; \ \ rule->args[rule->argsLen++] = g_strdup(str); \ } while (0) @@ -348,7 +348,7 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, if (VIR_ALLOC(rule) < 0) - goto no_memory; + goto cleanup; rule->layer = layer; rule->queryCB = cb; @@ -379,18 +379,18 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, if (VIR_APPEND_ELEMENT_COPY(group->rollback, group->nrollback, rule) < 0) - goto no_memory; + goto cleanup; } else { if (VIR_APPEND_ELEMENT_COPY(group->action, group->naction, rule) < 0) - goto no_memory; + goto cleanup; } return rule; - no_memory: + cleanup: firewall->err = ENOMEM; virFirewallRuleFree(rule); return NULL; @@ -494,7 +494,7 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, return; - no_memory: + cleanup: firewall->err = ENOMEM; } @@ -516,7 +516,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, return; - no_memory: + cleanup: firewall->err = ENOMEM; } @@ -534,7 +534,7 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, return; - no_memory: + cleanup: firewall->err = ENOMEM; } @@ -557,7 +557,7 @@ void virFirewallRuleAddArgList(virFirewallPtr firewall, return; - no_memory: + cleanup: firewall->err = ENOMEM; va_end(list); } -- 2.24.1
As libvirt decided to take the path to not report OOM and simply abort when it happens, let's get rid of OOM treatment in virfirewall.c 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 c6a5de8842..00b1bd0a97 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -391,7 +391,6 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, return rule; cleanup: - firewall->err = ENOMEM; virFirewallRuleFree(rule); return NULL; } @@ -492,10 +491,8 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, ADD_ARG(rule, arg); - return; - cleanup: - firewall->err = ENOMEM; + return; } @@ -514,10 +511,8 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, ADD_ARG(rule, arg); - return; - cleanup: - firewall->err = ENOMEM; + return; } @@ -532,10 +527,8 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, args++; } - return; - cleanup: - 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; - cleanup: - 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 00b1bd0a97..d6fff31318 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 cleanup; + 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 cleanup; + return NULL; } else { if (VIR_APPEND_ELEMENT_COPY(group->action, group->naction, rule) < 0) - goto cleanup; + return NULL; } - return rule; + return g_steal_pointer(&rule); cleanup: - 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 d6fff31318..6dace18bc4 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
Similarly to ADD_ARG, let's create an ADD_ARG_RETURN_ON_ERROR macro which will simply return instead of going to a cleanup label. By doing this, we can get rid of a few cleanup labels spread in the code. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 6dace18bc4..d632f72502 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -328,6 +328,16 @@ void virFirewallFree(virFirewallPtr firewall) rule->args[rule->argsLen++] = g_strdup(str); \ } while (0) +#define ADD_ARG_RETURN_ON_ERROR(rule, str) \ + do { \ + if (VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1) < 0) \ + return; \ + \ + rule->args[rule->argsLen++] = g_strdup(str); \ + } while (0) + static virFirewallRulePtr virFirewallAddRuleFullV(virFirewallPtr firewall, virFirewallLayer layer, @@ -490,10 +500,7 @@ void virFirewallRuleAddArg(virFirewallPtr firewall, { VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); - ADD_ARG(rule, arg); - - cleanup: - return; + ADD_ARG_RETURN_ON_ERROR(rule, arg); } @@ -510,10 +517,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, arg = g_strdup_vprintf(fmt, list); va_end(list); - ADD_ARG(rule, arg); - - cleanup: - return; + ADD_ARG_RETURN_ON_ERROR(rule, arg); } @@ -524,12 +528,9 @@ void virFirewallRuleAddArgSet(virFirewallPtr firewall, VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); while (*args) { - ADD_ARG(rule, *args); + ADD_ARG_RETURN_ON_ERROR(rule, *args); args++; } - - cleanup: - return; } -- 2.24.1
On Thu, Jan 02, 2020 at 02:57:56PM +0100, Fabiano Fidêncio wrote:
Similarly to ADD_ARG, let's create an ADD_ARG_RETURN_ON_ERROR macro which will simply return instead of going to a cleanup label.
By doing this, we can get rid of a few cleanup labels spread in the code.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 6dace18bc4..d632f72502 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -328,6 +328,16 @@ void virFirewallFree(virFirewallPtr firewall) rule->args[rule->argsLen++] = g_strdup(str); \ } while (0)
+#define ADD_ARG_RETURN_ON_ERROR(rule, str) \ + do { \ + if (VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1) < 0) \ + return; \ + \ + rule->args[rule->argsLen++] = g_strdup(str); \ + } while (0)
IMHO this is missing the benefit of using glib since VIR_RESIZE_N will never fail now. We should get rid of "ADD_ARG" entirely, and just put a g_realloc() call directly in the place it is needed without any macros. Likewise for the next patch. 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 :|
[snip]
+#define ADD_ARG_RETURN_ON_ERROR(rule, str) \ + do { \ + if (VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1) < 0) \ + return; \ + \ + rule->args[rule->argsLen++] = g_strdup(str); \ + } while (0)
IMHO this is missing the benefit of using glib since VIR_RESIZE_N will never fail now. We should get rid of "ADD_ARG" entirely, and just put a g_realloc() call directly in the place it is needed without any macros. Likewise for the next patch.
Cool, I really missed that. I'll submit a v2, soon. Best Regards, -- Fabiano Fidêncio.
Similarly to ADD_ARG, let's create an ADD_ARG_RETURN_VALUE_ON_ERROR macro which will simply return a value instead of going to a cleanup label. This patch will get rid of the cleanup label present in virFirewallAddRuleFullV(). Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virfirewall.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index d632f72502..1b04fc5ee4 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -338,6 +338,16 @@ void virFirewallFree(virFirewallPtr firewall) rule->args[rule->argsLen++] = g_strdup(str); \ } while (0) +#define ADD_ARG_RETURN_VALUE_ON_ERROR(rule, str, value) \ + do { \ + if (VIR_RESIZE_N(rule->args, \ + rule->argsAlloc, \ + rule->argsLen, 1) < 0) \ + return value; \ + \ + rule->args[rule->argsLen++] = g_strdup(str); \ + } while (0) + static virFirewallRulePtr virFirewallAddRuleFullV(virFirewallPtr firewall, virFirewallLayer layer, @@ -370,22 +380,22 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, switch (rule->layer) { case VIR_FIREWALL_LAYER_ETHERNET: if (ebtablesUseLock) - ADD_ARG(rule, "--concurrent"); + ADD_ARG_RETURN_VALUE_ON_ERROR(rule, "--concurrent", NULL); break; case VIR_FIREWALL_LAYER_IPV4: if (iptablesUseLock) - ADD_ARG(rule, "-w"); + ADD_ARG_RETURN_VALUE_ON_ERROR(rule, "-w", NULL); break; case VIR_FIREWALL_LAYER_IPV6: if (ip6tablesUseLock) - ADD_ARG(rule, "-w"); + ADD_ARG_RETURN_VALUE_ON_ERROR(rule, "-w", NULL); break; case VIR_FIREWALL_LAYER_LAST: break; } while ((str = va_arg(args, char *)) != NULL) - ADD_ARG(rule, str); + ADD_ARG_RETURN_VALUE_ON_ERROR(rule, str, NULL); if (group->addingRollback) { if (VIR_APPEND_ELEMENT_COPY(group->rollback, @@ -401,9 +411,6 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, return g_steal_pointer(&rule); - - cleanup: - return NULL; } -- 2.24.1
no_memory labels have been entirely removed from libvirt codebase. 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 (2)
-
Daniel P. Berrangé -
Fabiano Fidêncio