[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] nwfilter: cleanup return codes in nwfilter subsystem



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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]