Remove superfluous quota rule delete commands.

When setting shared or interface quota, BandwidthController will
delete rules in bw_{FORWARD,INPUT,OUTPUT} before re-adding them.
These deletes are guaranteed to fail because the rules being
deleted only exist when bandwidth control is enabled and the
applicable interface is in mQuotaIfaces. Specifically, as long
as no intermediate iptables commands fail:

1. When bandwidth control is enabled or disabled, all the
   bw_{FORWARD,INPUT,OUTPUT} chains are cleared by
   flushCleanTables.
2. The rules that were being deleted are only added when
   bandwidth control is enabled and an interface is added to
   mQuotaIfaces.
3. Adding a quota is a no-op if the interface is already in
   mQuotaIfaces (or mSharedQuotaIfaces for shared quotas).
4. When an interface is removed from mQuotaIfaces (or
   mSharedQuotaIfaces), the rules are always deleted.

In the presence of intermediate iptables command failures this
change could make things worse, but an upcoming change will move
the quota commands to iptables-restore, which will ensure that
iptables commands in a quota operation either all succeed or all
fail.

In addition to removing the superfluous deletes, also change the
order of the commands that create a chain from "-F then -N" to
"-N then -F". This simplifies the code and the tests a bit.

Bug: 28362720
Test: bullhead builds, boots
Test: netd_{unit,integration}_test pass
Test: quota rules are added and removed when quotas are enabled/disabled
Change-Id: I64a0a2aa16066163c71f6d3ead36839b51c34620
Merged-In: I2b68d17d7c7640e3956ae010f9882d34bf24d9fc

2 files changed, 7 insertions, 30 deletions

diff --git a/server/BandwidthController.cpp b/server/BandwidthController.cpp
index 2a83d91a..f1ba45d7 100644
--- a/server/BandwidthController.cpp
+++ b/server/BandwidthController.cpp
@@ -375,7 +375,7 @@ std::string BandwidthController::makeIptablesQuotaCmd(IptFullOp op, const std::s
 
 int BandwidthController::prepCostlyIface(const std::string& ifn, QuotaType quotaType) {
     char cmd[MAX_CMD_LEN];
-    int res = 0, res1, res2;
+    int res = 0;
     int ruleInsertPos = 1;
     std::string costString;
     const char *costCString;
@@ -387,15 +387,14 @@ int BandwidthController::prepCostlyIface(const std::string& ifn, QuotaType quota
         costString += ifn;
         costCString = costString.c_str();
         /*
-         * Flush the bw_costly_<iface> is allowed to fail in case it didn't exist.
-         * Creating a new one is allowed to fail in case it existed.
+         * Creating a new bw_costly_<iface> is allowed to fail in case it existed.
+         * Regardless of whether it previously existed or not, flushing it should never fail.
          * This helps with netd restarts.
          */
-        snprintf(cmd, sizeof(cmd), "-F %s", costCString);
-        res1 = runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
         snprintf(cmd, sizeof(cmd), "-N %s", costCString);
-        res2 = runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
-        res = (res1 && res2) || (!res1 && !res2);
+        runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
+        snprintf(cmd, sizeof(cmd), "-F %s", costCString);
+        res |= runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailShow);
 
         snprintf(cmd, sizeof(cmd), "-A %s -j bw_penalty_box", costCString);
         res |= runIpxtablesCmd(cmd, IptJumpNoAdd);
@@ -410,22 +409,14 @@ int BandwidthController::prepCostlyIface(const std::string& ifn, QuotaType quota
         ruleInsertPos = 2;
     }
 
-    snprintf(cmd, sizeof(cmd), "-D bw_INPUT -i %s --jump %s", ifn.c_str(), costCString);
-    runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
-
     snprintf(cmd, sizeof(cmd), "-I bw_INPUT %d -i %s --jump %s", ruleInsertPos, ifn.c_str(),
              costCString);
     res |= runIpxtablesCmd(cmd, IptJumpNoAdd);
 
-    snprintf(cmd, sizeof(cmd), "-D bw_OUTPUT -o %s --jump %s", ifn.c_str(), costCString);
-    runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
-
     snprintf(cmd, sizeof(cmd), "-I bw_OUTPUT %d -o %s --jump %s", ruleInsertPos, ifn.c_str(),
              costCString);
     res |= runIpxtablesCmd(cmd, IptJumpNoAdd);
 
-    snprintf(cmd, sizeof(cmd), "-D bw_FORWARD -o %s --jump %s", ifn.c_str(), costCString);
-    runIpxtablesCmd(cmd, IptJumpNoAdd, IptFailHide);
     snprintf(cmd, sizeof(cmd), "-A bw_FORWARD -o %s --jump %s", ifn.c_str(), costCString);
     res |= runIpxtablesCmd(cmd, IptJumpNoAdd);
 
diff --git a/server/BandwidthControllerTest.cpp b/server/BandwidthControllerTest.cpp
index dd466122..1886ad67 100644
--- a/server/BandwidthControllerTest.cpp
+++ b/server/BandwidthControllerTest.cpp
@@ -386,15 +386,11 @@ const std::vector<std::string> makeInterfaceQuotaCommands(const std::string& ifa
     const char* c_chain = chain.c_str();
     const char* c_iface = iface.c_str();
     std::vector<std::string> cmds = {
-        //      StringPrintf(":%s -", c_chain),
-        StringPrintf("-F %s", c_chain),
         StringPrintf("-N %s", c_chain),
+        StringPrintf("-F %s", c_chain),
         StringPrintf("-A %s -j bw_penalty_box", c_chain),
-        StringPrintf("-D bw_INPUT -i %s --jump %s", c_iface, c_chain),
         StringPrintf("-I bw_INPUT %d -i %s --jump %s", ruleIndex, c_iface, c_chain),
-        StringPrintf("-D bw_OUTPUT -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-I bw_OUTPUT %d -o %s --jump %s", ruleIndex, c_iface, c_chain),
-        StringPrintf("-D bw_FORWARD -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-A bw_FORWARD -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-A %s -m quota2 ! --quota %" PRIu64 " --name %s --jump REJECT", c_chain,
                      quota, c_iface),
@@ -421,13 +417,6 @@ TEST_F(BandwidthControllerTest, TestSetInterfaceQuota) {
     const std::string iface = mTun.name();
     std::vector<std::string> expected = makeInterfaceQuotaCommands(iface, 1, kOldQuota);
 
-    // prepCostlyInterface assumes that exactly one of the "-F chain" and "-N chain" commands fails.
-    // So pretend that the first two commands (the IPv4 -F and the IPv6 -F) fail.
-    std::deque<int> returnValues(expected.size() * 2, 0);
-    returnValues[0] = 1;
-    returnValues[1] = 1;
-    setReturnValues(returnValues);
-
     EXPECT_EQ(0, mBw.setInterfaceQuota(iface, kOldQuota));
     expectIptablesCommands(expected);
 
@@ -448,11 +437,8 @@ const std::vector<std::string> makeInterfaceSharedQuotaCommands(const std::strin
     const char* c_chain = chain.c_str();
     const char* c_iface = iface.c_str();
     std::vector<std::string> cmds = {
-        StringPrintf("-D bw_INPUT -i %s --jump %s", c_iface, c_chain),
         StringPrintf("-I bw_INPUT %d -i %s --jump %s", ruleIndex, c_iface, c_chain),
-        StringPrintf("-D bw_OUTPUT -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-I bw_OUTPUT %d -o %s --jump %s", ruleIndex, c_iface, c_chain),
-        StringPrintf("-D bw_FORWARD -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-A bw_FORWARD -o %s --jump %s", c_iface, c_chain),
         StringPrintf("-I %s -m quota2 ! --quota %" PRIu64 " --name shared --jump REJECT", c_chain,
                      quota),