[libvirt PATCH 00/10] virHashNew refactorings - part V
"virHashNew" cannot return NULL, yet we check for NULL in various places. See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html. Tim Wiederhake (10): conf: Add AUTOPTR_CLEANUP_FUNC for virNWFilterVarValue virNWFilterParseParamAttributes: `virHashNew` cannot return NULL virNWFilterParseParamAttributes: Iterate over "element" children virNWFilterParseParamAttributes: Remove tautological `if` virNWFilterParseParamAttributes: Use automatic memory management virNWFilterParseParamAttributes: Simplify loop body virNWFilterParseParamAttributes: Remove superfluous `goto`s virNWFilterDoInstantiate: `virHashNew` cannot return NULL virNWFilterIPAddrMapInit: `virHashNew` cannot return NULL virNWFilterLearnInit: `virHashNew` cannot return NULL src/conf/nwfilter_ipaddrmap.c | 3 -- src/conf/nwfilter_params.c | 70 +++++++++----------------- src/conf/nwfilter_params.h | 2 + src/nwfilter/nwfilter_gentech_driver.c | 5 -- src/nwfilter/nwfilter_learnipaddr.c | 8 --- 5 files changed, 26 insertions(+), 62 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index d11b533958..f9f7f68ff3 100644 --- a/src/conf/nwfilter_params.h +++ b/src/conf/nwfilter_params.h @@ -51,6 +51,8 @@ virNWFilterVarValue *virNWFilterVarValueCreateSimple(char *); virNWFilterVarValue *virNWFilterVarValueCreateSimpleCopyValue(const char *); virNWFilterVarValue *virNWFilterVarValueCopy(const virNWFilterVarValue *); void virNWFilterVarValueFree(virNWFilterVarValue *val); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNWFilterVarValue, virNWFilterVarValueFree); + void virNWFilterVarValueHashFree(void *payload); const char *virNWFilterVarValueGetSimple(const virNWFilterVarValue *val); const char *virNWFilterVarValueGetNthValue(const virNWFilterVarValue *val, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index aeed0cff1f..c94e9679f8 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -702,8 +702,6 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) virNWFilterVarValue *value; GHashTable *table = virHashNew(virNWFilterVarValueHashFree); - if (!table) - return NULL; cur = cur->children; -- 2.31.1
"xmlNextElementSibling()" skips attribute nodes, making the explicit check for the type of `cur` redundant. This prepares for the removal of this check in the next commit. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index c94e9679f8..3754cec55f 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -703,7 +703,7 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) GHashTable *table = virHashNew(virNWFilterVarValueHashFree); - cur = cur->children; + cur = xmlFirstElementChild(cur); while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -739,7 +739,7 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) VIR_FREE(val); } } - cur = cur->next; + cur = xmlNextElementSibling(cur); } return table; -- 2.31.1
`cur` is guaranteed to be of type `XML_ELEMENT_NODE` by using `xmlFirstElementChild()` and `xmlNextElementSibling()`. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 57 +++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 3754cec55f..0b967e1194 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -706,39 +706,38 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) cur = xmlFirstElementChild(cur); while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "parameter")) { - nam = virXMLPropString(cur, "name"); - val = virXMLPropString(cur, "value"); - value = NULL; - if (nam != NULL && val != NULL) { - if (!isValidVarName(nam)) - goto skip_entry; - if (!isValidVarValue(val)) - goto skip_entry; - value = virHashLookup(table, nam); - if (value) { - /* add value to existing value -> list */ - if (virNWFilterVarValueAddValue(value, val) < 0) { - value = NULL; - goto err_exit; - } - val = NULL; - } else { - value = virNWFilterParseVarValue(val); - if (!value) - goto skip_entry; - if (virHashUpdateEntry(table, nam, value) < 0) - goto err_exit; + if (virXMLNodeNameEqual(cur, "parameter")) { + nam = virXMLPropString(cur, "name"); + val = virXMLPropString(cur, "value"); + value = NULL; + if (nam != NULL && val != NULL) { + if (!isValidVarName(nam)) + goto skip_entry; + if (!isValidVarValue(val)) + goto skip_entry; + value = virHashLookup(table, nam); + if (value) { + /* add value to existing value -> list */ + if (virNWFilterVarValueAddValue(value, val) < 0) { + value = NULL; + goto err_exit; } - value = NULL; + val = NULL; + } else { + value = virNWFilterParseVarValue(val); + if (!value) + goto skip_entry; + if (virHashUpdateEntry(table, nam, value) < 0) + goto err_exit; } - skip_entry: - virNWFilterVarValueFree(value); - VIR_FREE(nam); - VIR_FREE(val); + value = NULL; } + skip_entry: + virNWFilterVarValueFree(value); + VIR_FREE(nam); + VIR_FREE(val); } + cur = xmlNextElementSibling(cur); } return table; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 0b967e1194..0b1676e25f 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -698,18 +698,15 @@ virNWFilterParseVarValue(const char *val) GHashTable * virNWFilterParseParamAttributes(xmlNodePtr cur) { - char *nam, *val; - virNWFilterVarValue *value; - - GHashTable *table = virHashNew(virNWFilterVarValueHashFree); + g_autoptr(GHashTable) table = virHashNew(virNWFilterVarValueHashFree); cur = xmlFirstElementChild(cur); while (cur != NULL) { if (virXMLNodeNameEqual(cur, "parameter")) { - nam = virXMLPropString(cur, "name"); - val = virXMLPropString(cur, "value"); - value = NULL; + g_autofree char *nam = virXMLPropString(cur, "name"); + g_autofree char *val = virXMLPropString(cur, "value"); + g_autoptr(virNWFilterVarValue) value = NULL; if (nam != NULL && val != NULL) { if (!isValidVarName(nam)) goto skip_entry; @@ -733,20 +730,13 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) value = NULL; } skip_entry: - virNWFilterVarValueFree(value); - VIR_FREE(nam); - VIR_FREE(val); } - cur = xmlNextElementSibling(cur); } - return table; + + return g_steal_pointer(&table); err_exit: - VIR_FREE(nam); - VIR_FREE(val); - virNWFilterVarValueFree(value); - virHashFree(table); return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 0b1676e25f..63ab7e7150 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -707,28 +707,22 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) g_autofree char *nam = virXMLPropString(cur, "name"); g_autofree char *val = virXMLPropString(cur, "value"); g_autoptr(virNWFilterVarValue) value = NULL; - if (nam != NULL && val != NULL) { - if (!isValidVarName(nam)) - goto skip_entry; - if (!isValidVarValue(val)) - goto skip_entry; - value = virHashLookup(table, nam); - if (value) { - /* add value to existing value -> list */ - if (virNWFilterVarValueAddValue(value, val) < 0) { - value = NULL; - goto err_exit; - } - val = NULL; - } else { - value = virNWFilterParseVarValue(val); - if (!value) - goto skip_entry; - if (virHashUpdateEntry(table, nam, value) < 0) - goto err_exit; - } - value = NULL; + + if (nam == NULL || !isValidVarName(nam) || + val == NULL || !isValidVarValue(val)) { + goto skip_entry; + } + + if ((value = virHashLookup(table, nam))) { + /* add value to existing value -> list */ + if (virNWFilterVarValueAddValue(g_steal_pointer(&value), val) < 0) + goto err_exit; + val = NULL; + } else if ((value = virNWFilterParseVarValue(val))) { + if (virHashUpdateEntry(table, nam, value) < 0) + goto err_exit; } + value = NULL; skip_entry: } cur = xmlNextElementSibling(cur); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 63ab7e7150..35ea0256c3 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -710,28 +710,25 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) if (nam == NULL || !isValidVarName(nam) || val == NULL || !isValidVarValue(val)) { - goto skip_entry; + cur = xmlNextElementSibling(cur); + continue; } if ((value = virHashLookup(table, nam))) { /* add value to existing value -> list */ if (virNWFilterVarValueAddValue(g_steal_pointer(&value), val) < 0) - goto err_exit; + return NULL; val = NULL; } else if ((value = virNWFilterParseVarValue(val))) { if (virHashUpdateEntry(table, nam, value) < 0) - goto err_exit; + return NULL; } value = NULL; - skip_entry: } cur = xmlNextElementSibling(cur); } return g_steal_pointer(&table); - - err_exit: - return NULL; } -- 2.31.1
On a %A in %Y, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_params.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 63ab7e7150..35ea0256c3 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -710,28 +710,25 @@ virNWFilterParseParamAttributes(xmlNodePtr cur)
if (nam == NULL || !isValidVarName(nam) || val == NULL || !isValidVarValue(val)) { - goto skip_entry; + cur = xmlNextElementSibling(cur);
You can also use a for loop if you don't want to duplicate this assignment. Jano
+ continue; }
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 8aa1db23d3..114c1cef4f 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -519,11 +519,6 @@ virNWFilterDoInstantiate(virNWFilterTechDriver *techdriver, memset(&inst, 0, sizeof(inst)); - if (!missing_vars) { - rc = -1; - goto error; - } - rc = virNWFilterDetermineMissingVarsRec(filter, binding->filterparams, missing_vars, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/nwfilter_ipaddrmap.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c index d6facd749e..bc21c80536 100644 --- a/src/conf/nwfilter_ipaddrmap.c +++ b/src/conf/nwfilter_ipaddrmap.c @@ -150,9 +150,6 @@ int virNWFilterIPAddrMapInit(void) { ipAddressMap = virHashNew(virNWFilterVarValueHashFree); - if (!ipAddressMap) - return -1; - return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_learnipaddr.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 907e4b2513..2177c5540b 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -774,16 +774,8 @@ virNWFilterLearnInit(void) VIR_DEBUG("Initializing IP address learning"); threadsTerminate = false; - pendingLearnReq = virHashNew(freeLearnReqEntry); - if (!pendingLearnReq) - return -1; - ifaceLockMap = virHashNew(g_free); - if (!ifaceLockMap) { - virNWFilterLearnShutdown(); - return -1; - } return 0; } -- 2.31.1
ping On Wed, 2021-07-14 at 11:44 +0200, Tim Wiederhake wrote:
"virHashNew" cannot return NULL, yet we check for NULL in various places.
See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html .
Tim Wiederhake (10): conf: Add AUTOPTR_CLEANUP_FUNC for virNWFilterVarValue virNWFilterParseParamAttributes: `virHashNew` cannot return NULL virNWFilterParseParamAttributes: Iterate over "element" children virNWFilterParseParamAttributes: Remove tautological `if` virNWFilterParseParamAttributes: Use automatic memory management virNWFilterParseParamAttributes: Simplify loop body virNWFilterParseParamAttributes: Remove superfluous `goto`s virNWFilterDoInstantiate: `virHashNew` cannot return NULL virNWFilterIPAddrMapInit: `virHashNew` cannot return NULL virNWFilterLearnInit: `virHashNew` cannot return NULL
src/conf/nwfilter_ipaddrmap.c | 3 -- src/conf/nwfilter_params.c | 70 +++++++++--------------- -- src/conf/nwfilter_params.h | 2 + src/nwfilter/nwfilter_gentech_driver.c | 5 -- src/nwfilter/nwfilter_learnipaddr.c | 8 --- 5 files changed, 26 insertions(+), 62 deletions(-)
-- 2.31.1
On a %A in %Y, Tim Wiederhake wrote:
"virHashNew" cannot return NULL, yet we check for NULL in various places.
See https://listman.redhat.com/archives/libvir-list/2021-July/msg00074.html.
Tim Wiederhake (10): conf: Add AUTOPTR_CLEANUP_FUNC for virNWFilterVarValue virNWFilterParseParamAttributes: `virHashNew` cannot return NULL virNWFilterParseParamAttributes: Iterate over "element" children virNWFilterParseParamAttributes: Remove tautological `if` virNWFilterParseParamAttributes: Use automatic memory management virNWFilterParseParamAttributes: Simplify loop body virNWFilterParseParamAttributes: Remove superfluous `goto`s virNWFilterDoInstantiate: `virHashNew` cannot return NULL virNWFilterIPAddrMapInit: `virHashNew` cannot return NULL virNWFilterLearnInit: `virHashNew` cannot return NULL
src/conf/nwfilter_ipaddrmap.c | 3 -- src/conf/nwfilter_params.c | 70 +++++++++----------------- src/conf/nwfilter_params.h | 2 + src/nwfilter/nwfilter_gentech_driver.c | 5 -- src/nwfilter/nwfilter_learnipaddr.c | 8 --- 5 files changed, 26 insertions(+), 62 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jano Tomko -
Tim Wiederhake