5:19 p.m.
On 11/23/2011 02:19 PM, Stefan Berger wrote:
This patch cleans up return codes in the nwfilter subsystem.
Some functions in nwfilter_conf.c (validators and formatters) are
keeping their bool return for now and I am converting their return
code to true/false.
All other functions now return -1 on failure and 0 on success.
[I searched for all occurences of ' 1;' and checked all 'if ' and
adapted where needed. After that I did a grep for 'NWFilter' in the source
tree.]
Resuming where I left off last time...
Index: libvirt-acl/src/conf/nwfilter_params.c
@@ -505,25 +505,25 @@ virNWFilterHashTablePut(virNWFilterHashT
if (copyName) {
name = strdup(name);
if (!name)
- return 1;
+ return -1;
virNWFilterDetermineMissingVarsRec() has an unchecked call to this
function, meaning that an OOM error can go undetected in that call site.
All other calls to this function look sanely converted.
@@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHa
return 0;
err_exit:
- return 1;
+ return -1;
All callers look sanely converted.
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c
@@ -106,7 +106,7 @@ virNWFilterRuleInstAddData(virNWFilterRu
Comment prior to the function needs an update.
{
if (VIR_REALLOC_N(res->data, res->ndata+1) < 0) {
virReportOOMError();
- return 1;
+ return -1;
All callers look sanely converted.
@@ -151,28 +151,28 @@ virNWFilterVarHashmapAddStdValues(virNWF
if (macaddr) {
val = virNWFilterVarValueCreateSimple(macaddr);
if (!val)
- return 1;
+ return -1;
Comment prior to the function needs an update. All callers correctly
converted.
@@ -404,13 +404,13 @@ _virNWFilterInstantiateRec(virNWFilterTe
ifname,
vars);
if (!inst) {
- rc = 1;
+ rc = -1;
Looks sane.
@@ -504,7 +504,7 @@ virNWFilterDetermineMissingVarsRec(virNW
if (!virHashLookup(vars->hashTable, rule->vars[j])) {
val = virNWFilterVarValueCreateSimpleCopyValue("1");
if (!val) {
- rc = 1;
+ rc = -1;
Looks sane.
@@ -592,7 +592,7 @@ virNWFilterRuleInstancesToArray(int nEnt
if (VIR_ALLOC_N((*ptrs), (*nptrs)) < 0) {
virReportOOMError();
- return 1;
+ return -1;
Looks sane.
@@ -649,7 +649,7 @@ virNWFilterInstantiate(virNWFilterTechDr
virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0);
if (!missing_vars) {
virReportOOMError();
- rc = 1;
+ rc = -1;
This one calls through a callback that I did not audit:
rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
but assuming the callback is okay (or that you change things to ensure
rc is -1 even if the callback returns positive), then clients of this
look okay.
@@ -792,7 +792,7 @@ __virNWFilterInstantiateFilter(bool tear
_("Could not get access to ACL tech "
"driver '%s'"),
drvname);
- return 1;
+ return -1;
Ultimately called via virNWFilterInstantiateFilterLate, in turn called
by nwfilter_learnipaddr.c:learnIPAddressThread, which collects the
return value and prints it in a VIR_DEBUG but does nothing if the value
was negative. Is that a problem?
Also called by the .instantiateFilter callback installed by
nwfilter_driver.c, which feeds virDomainConfNWFilterInstantiate, and all
callers look clean.
@@ -1012,7 +1012,7 @@ int virNWFilterRollbackUpdateFilter(cons
_("Could not get access to ACL tech "
"driver '%s'"),
drvname);
- return 1;
+ return -1;
Should this function be static? No one outside of gentech_driver calls
it. At any rate, callers inside the file are sane.
@@ -1038,7 +1038,7 @@ virNWFilterTearOldFilter(virDomainNetDef
_("Could not get access to ACL tech "
"driver '%s'"),
drvname);
- return 1;
+ return -1;
Same comments about static, and sane usage.
@@ -1063,13 +1063,13 @@ _virNWFilterTeardownFilter(const char *i
_("Could not get access to ACL tech "
"driver '%s'"),
drvname);
- return 1;
+ return -1;
Doesn't strictly matter (the ultimate caller nwfilterTeardownFilter
returns void), but might as well be consistent.
Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
@@ -184,7 +184,7 @@ virNWFilterLockIface(const char *ifname)
err_exit:
virMutexUnlock(&ifaceMapLock);
- return 1;
+ return -1;
Looks sane.
@@ -248,7 +248,7 @@ virNWFilterRegisterLearnReq(virNWFilterI
int
virNWFilterTerminateLearnReq(const char *ifname) {
- int rc = 1;
+ int rc = -1;
The only caller (in gentech_driver) doesn't pay attention to the return
value; but that's probably okay. Looks sane.
@@ -336,9 +336,6 @@ virNWFilterAddIpAddrForIfname(const char
goto cleanup;
}
ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1);
- /* FIXME: fix when return code of virNWFilterHashTablePut changes */
- if (ret)
- ret = -1;
Always fun to get rid of a FIXME.
@@ -530,7 +527,7 @@ learnIPAddressThread(void *arg)
break;
default:
if (techdriver->applyBasicRules(req->ifname,
- req->macaddr)) {
+ req->macaddr) < 0) {
I didn't audit where this callback came from, but assume it was okay.
@@ -781,14 +778,14 @@ virNWFilterLearnIPAddress(virNWFilterTec
virNWFilterHashTablePtr ht = NULL;
if (howDetect == 0)
- return 1;
+ return -1;
Looks sane.
@@ -895,35 +892,35 @@ virNWFilterLearnInit(void) {
pendingLearnReq = virHashCreate(0, freeLearnReqEntry);
if (!pendingLearnReq) {
- return 1;
+ return -1;
Looks sane.
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -233,15 +233,15 @@ printVar(virNWFilterVarCombIterPtr vars,
val = virNWFilterVarCombIterGetVarValue(vars, item->var);
if (!val) {
/* error has been reported */
- return 1;
+ return -1;
Looks sane.
@@ -259,8 +259,8 @@ _printDataType(virNWFilterVarCombIterPtr
int done;
char *data;
- if (printVar(vars, buf, bufsize, item, &done))
- return 1;
+ if (printVar(vars, buf, bufsize, item, &done) < 0)
+ return -1;
Looks sane.
@@ -417,7 +417,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns
if (VIR_ALLOC(inst) < 0) {
virReportOOMError();
- return 1;
+ return -1;
Looks sane.
@@ -492,7 +492,7 @@ ebtablesHandleEthHdr(virBufferPtr buf,
err_exit:
virBufferFreeAndReset(buf);
- return 1;
+ return -1;
Looks sane.
@@ -909,7 +909,7 @@ iptablesHandleSrcMacAddr(virBufferPtr bu
err_exit:
virBufferFreeAndReset(buf);
- return 1;
+ return -1;
Looks sane.
@@ -1057,7 +1057,7 @@ iptablesHandleIpHdr(virBufferPtr buf,
@@ -1085,7 +1085,7 @@ err_exit:
virBufferFreeAndReset(buf);
virBufferFreeAndReset(afterStateMatch);
- return 1;
+ return -1;
Looks sane.
@@ -1154,7 +1154,7 @@ iptablesHandlePortData(virBufferPtr buf,
return 0;
err_exit:
- return 1;
+ return -1;
Looks sane.
@@ -1664,7 +1664,7 @@ printStateMatchFlags(int32_t flags, char
if (virBufferError(&buf)) {
virBufferFreeAndReset(&buf);
virReportOOMError();
- return 1;
+ return -1;
Looks sane.
@@ -1704,8 +1704,8 @@ iptablesCreateRuleInstanceStateCtrl(virN
}
if (create && (rule->flags & IPTABLES_STATE_FLAGS)) {
- if (printStateMatchFlags(rule->flags, &matchState))
- return 1;
+ if (printStateMatchFlags(rule->flags, &matchState) < 0)
+ return -1;
Looks sane.
@@ -2542,7 +2554,7 @@ ebiptablesCreateRuleInstance(enum virDom
vars,
res,
rule->tt ==
VIR_NWFILTER_RULE_DIRECTION_INOUT);
- if (rc)
+ if (rc < 0)
return rc;
Looks sane.
@@ -3111,7 +3123,7 @@ ebtablesApplyBasicRules(const char *ifna
virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot create rules since ebtables tool is
"
"missing."));
- return 1;
+ return -1;
Comment before function needs touch up. Otherwise looks sane.
@@ -3207,13 +3219,15 @@ ebtablesApplyDHCPOnlyRules(const char *i
virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot create rules since ebtables tool is
"
"missing."));
- return 1;
+ return -1;
Looks sane, although I didn't closely audit if all uses of the
ebiptables_driver tech callbacks were adjusted.
@@ -3322,7 +3336,7 @@ ebtablesApplyDropAllRules(const char *if
virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot create rules since ebtables tool is
"
"missing."));
- return 1;
+ return -1;
Looks sane.
@@ -4086,7 +4100,7 @@ ebiptablesDriverInit(bool privileged)
_("firewall tools were not found or "
"cannot be used"));
ebiptablesDriverShutdown();
- return ENOTSUP;
+ return -ENOTSUP;
Looks sane.
Overall, I found one or two nits between my two-stage review, but they
seemed easy to fix.
ACK. Up to you if you want to post a delta to this patch for your
touchups, or just go ahead and commit, since I know you have other
patches backed up behind this one.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org