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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list