[libvirt] [PATCH] nwfilter: Fix return value of virNWFilterHashTablePut
In libvirt it is common to return value <0 on error not vice versa. --- src/conf/nwfilter_params.c | 16 ++++++++-------- src/nwfilter/nwfilter_gentech_driver.c | 4 ++-- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 12c6524..ba41972 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -490,7 +490,7 @@ hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) * @val: The value associated with the key * @freeName: Whether the name must be freed on table destruction * - * Returns 0 on success, 1 on failure. + * Returns 0 on success, -1 on failure. * * Put an entry into the hashmap replacing and freeing an existing entry * if one existed. @@ -505,11 +505,11 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, if (copyName) { name = strdup(name); if (!name) - return 1; + return -1; if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) { VIR_FREE(name); - return 1; + return -1; } table->names[table->nNames++] = (char *)name; } @@ -519,11 +519,11 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, VIR_FREE(name); table->nNames--; } - return 1; + return -1; } } else { if (virHashUpdateEntry(table->hashTable, name, val) != 0) { - return 1; + return -1; } } return 0; @@ -614,7 +614,7 @@ addToTable(void *payload, const void *name, void *data) return; } - if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) != 0) { + if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Could not put variable '%s' into hashmap"), (const char *)name); @@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHashTablePtr src, return 0; err_exit: - return 1; + return -1; } @@ -700,7 +700,7 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) value = virNWFilterParseVarValue(val); if (!value) goto skip_entry; - if (virNWFilterHashTablePut(table, nam, value, 1)) + if (virNWFilterHashTablePut(table, nam, value, 1) < 0) goto err_exit; } value = NULL; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 84a959b..946ed14 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -343,10 +343,10 @@ virNWFilterCreateVarsFrom(virNWFilterHashTablePtr vars1, return NULL; } - if (virNWFilterHashTablePutAll(vars1, res)) + if (virNWFilterHashTablePutAll(vars1, res) < 0) goto err_exit; - if (virNWFilterHashTablePutAll(vars2, res)) + if (virNWFilterHashTablePutAll(vars2, res) < 0) goto err_exit; return res; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 2e373a6..425e8a2 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -800,7 +800,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, goto err_free_req; } - if (virNWFilterHashTablePutAll(filterparams, ht)) + if (virNWFilterHashTablePutAll(filterparams, ht) < 0) goto err_free_ht; req->filtername = strdup(filtername); -- 1.7.3.4
On 11/23/2011 05:33 AM, Michal Privoznik wrote:
In libvirt it is common to return value <0 on error not vice versa. --- src/conf/nwfilter_params.c | 16 ++++++++-------- src/nwfilter/nwfilter_gentech_driver.c | 4 ++-- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-)
--- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -343,10 +343,10 @@ virNWFilterCreateVarsFrom(virNWFilterHashTablePtr vars1, return NULL; }
- if (virNWFilterHashTablePutAll(vars1, res)) + if (virNWFilterHashTablePutAll(vars1, res) < 0) goto err_exit;
- if (virNWFilterHashTablePutAll(vars2, res)) + if (virNWFilterHashTablePutAll(vars2, res) < 0) goto err_exit;
return res;
This only fixes 2 of the three affected call sites (the third wasn't even checking for failure, so that also needs fixing).
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 2e373a6..425e8a2 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -800,7 +800,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, goto err_free_req; }
- if (virNWFilterHashTablePutAll(filterparams, ht)) + if (virNWFilterHashTablePutAll(filterparams, ht) < 0)
You'll need to rebase this one on top of Stefan's recent patch, and take out the FIXME he just added. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
In libvirt it is common to return value <0 on error not vice versa. --- diff to v1: -check 3rd call as well: https://www.redhat.com/archives/libvir-list/2011-November/msg01312.html src/conf/nwfilter_params.c | 16 +++++++------- src/nwfilter/nwfilter_gentech_driver.c | 37 ++++++++++++++----------------- src/nwfilter/nwfilter_learnipaddr.c | 5 +--- 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 12c6524..ba41972 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -490,7 +490,7 @@ hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) * @val: The value associated with the key * @freeName: Whether the name must be freed on table destruction * - * Returns 0 on success, 1 on failure. + * Returns 0 on success, -1 on failure. * * Put an entry into the hashmap replacing and freeing an existing entry * if one existed. @@ -505,11 +505,11 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, if (copyName) { name = strdup(name); if (!name) - return 1; + return -1; if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) { VIR_FREE(name); - return 1; + return -1; } table->names[table->nNames++] = (char *)name; } @@ -519,11 +519,11 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, VIR_FREE(name); table->nNames--; } - return 1; + return -1; } } else { if (virHashUpdateEntry(table->hashTable, name, val) != 0) { - return 1; + return -1; } } return 0; @@ -614,7 +614,7 @@ addToTable(void *payload, const void *name, void *data) return; } - if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) != 0) { + if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Could not put variable '%s' into hashmap"), (const char *)name); @@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHashTablePtr src, return 0; err_exit: - return 1; + return -1; } @@ -700,7 +700,7 @@ virNWFilterParseParamAttributes(xmlNodePtr cur) value = virNWFilterParseVarValue(val); if (!value) goto skip_entry; - if (virNWFilterHashTablePut(table, nam, value, 1)) + if (virNWFilterHashTablePut(table, nam, value, 1) < 0) goto err_exit; } value = NULL; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 84a959b..a0cb05c 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -343,10 +343,10 @@ virNWFilterCreateVarsFrom(virNWFilterHashTablePtr vars1, return NULL; } - if (virNWFilterHashTablePutAll(vars1, res)) + if (virNWFilterHashTablePutAll(vars1, res) < 0) goto err_exit; - if (virNWFilterHashTablePutAll(vars2, res)) + if (virNWFilterHashTablePutAll(vars2, res) < 0) goto err_exit; return res; @@ -499,7 +499,7 @@ virNWFilterDetermineMissingVarsRec(virConnectPtr conn, virNWFilterDriverStatePtr driver) { virNWFilterObjPtr obj; - int rc = 0; + int rc = -1; int i, j; virNWFilterDefPtr next_filter; virNWFilterVarValuePtr val; @@ -512,16 +512,12 @@ virNWFilterDetermineMissingVarsRec(virConnectPtr conn, for (j = 0; j < rule->nvars; j++) { if (!virHashLookup(vars->hashTable, rule->vars[j])) { val = virNWFilterVarValueCreateSimpleCopyValue("1"); - if (!val) { - rc = 1; - break; - } - virNWFilterHashTablePut(missing_vars, rule->vars[j], - val, 1); + if (!val || + virNWFilterHashTablePut(missing_vars, rule->vars[j], + val, 1) < 0) + goto cleanup; } } - if (rc) - break; } else if (inc) { VIR_DEBUG("Following filter %s\n", inc->filterref); obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref); @@ -531,9 +527,8 @@ virNWFilterDetermineMissingVarsRec(virConnectPtr conn, virNWFilterReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), inc->filterref); - rc = 1; virNWFilterObjUnlock(obj); - break; + goto cleanup; } /* create a temporary hashmap for depth-first tree traversal */ @@ -542,9 +537,8 @@ virNWFilterDetermineMissingVarsRec(virConnectPtr conn, vars); if (!tmpvars) { virReportOOMError(); - rc = 1; virNWFilterObjUnlock(obj); - break; + goto cleanup; } next_filter = obj->def; @@ -569,17 +563,20 @@ virNWFilterDetermineMissingVarsRec(virConnectPtr conn, virNWFilterHashTableFree(tmpvars); virNWFilterObjUnlock(obj); - if (rc) - break; + if (rc < 0) + goto cleanup; } else { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), inc->filterref); - rc = 1; - break; + goto cleanup; } } } + + rc = 0; + +cleanup: return rc; } @@ -671,7 +668,7 @@ virNWFilterInstantiate(virConnectPtr conn, missing_vars, useNewFilter, driver); - if (rc) + if (rc < 0) goto err_exit; if (virHashSize(missing_vars->hashTable) == 1) { diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 6f2cc4c..425e8a2 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -336,9 +336,6 @@ virNWFilterAddIpAddrForIfname(const char *ifname, char *addr) goto cleanup; } ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); - /* FIXME: fix when return code of virNWFilterHashTablePut changes */ - if (ret) - ret = -1; goto cleanup; } else { if (virNWFilterVarValueAddValue(val, addr) < 0) @@ -803,7 +800,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, goto err_free_req; } - if (virNWFilterHashTablePutAll(filterparams, ht)) + if (virNWFilterHashTablePutAll(filterparams, ht) < 0) goto err_free_ht; req->filtername = strdup(filtername); -- 1.7.3.4
On 11/23/2011 07:19 AM, Michal Privoznik wrote:
In libvirt it is common to return value <0 on error not vice versa. --- diff to v1: -check 3rd call as well: https://www.redhat.com/archives/libvir-list/2011-November/msg01312.html
src/conf/nwfilter_params.c | 16 +++++++------- src/nwfilter/nwfilter_gentech_driver.c | 37 ++++++++++++++----------------- src/nwfilter/nwfilter_learnipaddr.c | 5 +--- 3 files changed, 26 insertions(+), 32 deletions(-)
@@ -671,7 +668,7 @@ virNWFilterInstantiate(virConnectPtr conn, missing_vars, useNewFilter, driver); - if (rc) + if (rc < 0) goto err_exit;
This hunk means that virNWFilterInstantiate can now return -1, where it used to return +1. If you're worried about that, then write this as: if (rc < 0) { rc = 1; goto err_exit; } But I think we're safe; all the call chains into filter instantiation are currently checking merely for non-zero values, and it probably won't be much longer before we also have a further patch to make the overall virNWFilterInstantiate return -1 on all failure paths. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/23/2011 09:41 AM, Eric Blake wrote:
On 11/23/2011 07:19 AM, Michal Privoznik wrote:
In libvirt it is common to return value<0 on error not vice versa. --- diff to v1: -check 3rd call as well: https://www.redhat.com/archives/libvir-list/2011-November/msg01312.html
src/conf/nwfilter_params.c | 16 +++++++------- src/nwfilter/nwfilter_gentech_driver.c | 37 ++++++++++++++----------------- src/nwfilter/nwfilter_learnipaddr.c | 5 +--- 3 files changed, 26 insertions(+), 32 deletions(-)
But I think we're safe; all the call chains into filter instantiation are currently checking merely for non-zero values, and it probably won't be much longer before we also have a further patch to make the overall virNWFilterInstantiate return -1 on all failure paths. Had one like that for a while. will post shortly...
Stefan
participants (3)
-
Eric Blake -
Michal Privoznik -
Stefan Berger