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

8 Dec 2011

On 12/08/2011 06:19 PM, Eric Blake wrote:
...
On 11/23/2011 02:19 PM, Stefan Berger wrote:
...
This patch cleans up return codes in the nwfilter subsystem.
[...]
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.
putting a virReportOOMError() into this one...
...
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.
Thanks. Fixed.
...
...
{
      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.
Fixed.
...
...
@@ -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);
ebiptablesApplyNewRules is being called. The whole ebiptables driver has 
only -1 as failure codes now. So should be ok.
...
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?
Well, if the instantiation for any reason fails, then the code currently 
'downs' the interface. I didn't know how else it should react since this
path is invoked in a thread and tearing down the VM would be too severe.
...
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.
Yes, indeed. I changed it now to be static.
...
...
@@ -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.
Also changed this one.@@ -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.
ebtablesApplyBasicRules now returns -1 in case of failure, so it's ok.
...
...
@@ -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.
Fixed. Thanks.
...
...
@@ -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.
I went ahead and pushed the patch. Thanks for this thorough review with 
all the cross-checking and looking through the function descriptions.

Cheers!
  Stefan

    