Simplify enums in BandwidthController.

1. Ensure that the code always uses all enum values. This
   provides a clear compile-time error if a passed-in enum value
   is not handled, and allows us to remove several default
   case labels and unreachable error logging code.
2. Factor out to common functions the code that converts enum
   values to parts of iptables command lines.

(cherry picked from commit d9db08c4a12d6a2953b597d39bb3ac37c43d3658)

Bug: 32073253
Test: netd_{unit,integration}_test pass
Change-Id: I7136055100dc312fa7cb8bba5506fe86412b1f4d
Merged-In: I7136055100dc312fa7cb8bba5506fe86412b1f4d

2 files changed, 57 insertions, 89 deletions

diff --git a/server/BandwidthController.cpp b/server/BandwidthController.cpp
index 30a048aa..828ef3ad 100644
--- a/server/BandwidthController.cpp
+++ b/server/BandwidthController.cpp
@@ -229,22 +229,7 @@ int BandwidthController::runIptablesCmd(const char *cmd, IptJumpOp jumpHandling,
     int status = 0;
 
     std::string fullCmd = cmd;
-
-    switch (jumpHandling) {
-    case IptJumpReject:
-        /*
-         * Must be carefull what one rejects with, as uper layer protocols will just
-         * keep on hammering the device until the number of retries are done.
-         * For port-unreachable (default), TCP should consider as an abort (RFC1122).
-         */
-        fullCmd += " --jump REJECT";
-        break;
-    case IptJumpReturn:
-        fullCmd += " --jump RETURN";
-        break;
-    case IptJumpNoAdd:
-        break;
-    }
+    fullCmd += jumpToString(jumpHandling);
 
     fullCmd.insert(0, " -w ");
     fullCmd.insert(0, iptVer == IptIpV4 ? IPTABLES_PATH : IP6TABLES_PATH);
@@ -345,15 +330,6 @@ std::string BandwidthController::makeIptablesSpecialAppCmd(IptOp op, int uid, co
     case IptOpInsert:
         opFlag = "-I";
         break;
-    case IptOpAppend:
-        ALOGE("Append op not supported for %s uids", chain);
-        res = "";
-        return res;
-        break;
-    case IptOpReplace:
-        opFlag = "-R";
-        break;
-    default:
     case IptOpDelete:
         opFlag = "-D";
         break;
@@ -365,52 +341,46 @@ std::string BandwidthController::makeIptablesSpecialAppCmd(IptOp op, int uid, co
 }
 
 int BandwidthController::addNaughtyApps(int numUids, char *appUids[]) {
-    return manipulateNaughtyApps(numUids, appUids, SpecialAppOpAdd);
+    return manipulateNaughtyApps(numUids, appUids, IptOpInsert);
 }
 
 int BandwidthController::removeNaughtyApps(int numUids, char *appUids[]) {
-    return manipulateNaughtyApps(numUids, appUids, SpecialAppOpRemove);
+    return manipulateNaughtyApps(numUids, appUids, IptOpDelete);
 }
 
 int BandwidthController::addNiceApps(int numUids, char *appUids[]) {
-    return manipulateNiceApps(numUids, appUids, SpecialAppOpAdd);
+    return manipulateNiceApps(numUids, appUids, IptOpInsert);
 }
 
 int BandwidthController::removeNiceApps(int numUids, char *appUids[]) {
-    return manipulateNiceApps(numUids, appUids, SpecialAppOpRemove);
+    return manipulateNiceApps(numUids, appUids, IptOpDelete);
 }
 
-int BandwidthController::manipulateNaughtyApps(int numUids, char *appStrUids[], SpecialAppOp appOp) {
-    return manipulateSpecialApps(numUids, appStrUids, "bw_penalty_box", IptJumpReject, appOp);
+int BandwidthController::manipulateNaughtyApps(int numUids, char *appStrUids[], IptOp op) {
+    return manipulateSpecialApps(numUids, appStrUids, "bw_penalty_box", IptJumpReject, op);
 }
 
-int BandwidthController::manipulateNiceApps(int numUids, char *appStrUids[], SpecialAppOp appOp) {
-    return manipulateSpecialApps(numUids, appStrUids, "bw_happy_box", IptJumpReturn, appOp);
+int BandwidthController::manipulateNiceApps(int numUids, char *appStrUids[], IptOp op) {
+    return manipulateSpecialApps(numUids, appStrUids, "bw_happy_box", IptJumpReturn, op);
 }
 
 
 int BandwidthController::manipulateSpecialApps(int numUids, char *appStrUids[],
                                                const char *chain,
-                                               IptJumpOp jumpHandling, SpecialAppOp appOp) {
+                                               IptJumpOp jumpHandling, IptOp op) {
 
     int uidNum;
     const char *failLogTemplate;
-    IptOp op;
     int appUids[numUids];
     std::string iptCmd;
 
-    switch (appOp) {
-    case SpecialAppOpAdd:
-        op = IptOpInsert;
+    switch (op) {
+    case IptOpInsert:
         failLogTemplate = "Failed to add app uid %s(%d) to %s.";
         break;
-    case SpecialAppOpRemove:
-        op = IptOpDelete;
+    case IptOpDelete:
         failLogTemplate = "Failed to delete app uid %s(%d) from %s box.";
         break;
-    default:
-        ALOGE("Unexpected app Op %d", appOp);
-        return -1;
     }
 
     for (uidNum = 0; uidNum < numUids; uidNum++) {
@@ -441,7 +411,7 @@ fail_parse:
     return -1;
 }
 
-std::string BandwidthController::makeIptablesQuotaCmd(IptOp op, const char *costName, int64_t quota) {
+std::string BandwidthController::makeIptablesQuotaCmd(IptFullOp op, const char *costName, int64_t quota) {
     std::string res;
     char *buff;
     const char *opFlag;
@@ -449,17 +419,13 @@ std::string BandwidthController::makeIptablesQuotaCmd(IptOp op, const char *cost
     ALOGV("makeIptablesQuotaCmd(%d, %" PRId64")", op, quota);
 
     switch (op) {
-    case IptOpInsert:
+    case IptFullOpInsert:
         opFlag = "-I";
         break;
-    case IptOpAppend:
+    case IptFullOpAppend:
         opFlag = "-A";
         break;
-    case IptOpReplace:
-        opFlag = "-R";
-        break;
-    default:
-    case IptOpDelete:
+    case IptFullOpDelete:
         opFlag = "-D";
         break;
     }
@@ -502,9 +468,6 @@ int BandwidthController::prepCostlyIface(const char *ifn, QuotaType quotaType) {
     case QuotaShared:
         costCString = "bw_costly_shared";
         break;
-    default:
-        ALOGE("Unexpected quotatype %d", quotaType);
-        return -1;
     }
 
     if (globalAlertBytes) {
@@ -547,9 +510,6 @@ int BandwidthController::cleanupCostlyIface(const char *ifn, QuotaType quotaType
     case QuotaShared:
         costCString = "bw_costly_shared";
         break;
-    default:
-        ALOGE("Unexpected quotatype %d", quotaType);
-        return -1;
     }
 
     snprintf(cmd, sizeof(cmd), "-D bw_INPUT -i %s --jump %s", ifn, costCString);
@@ -604,7 +564,7 @@ int BandwidthController::setInterfaceSharedQuota(const char *iface, int64_t maxB
     if (it == sharedQuotaIfaces.end()) {
         res |= prepCostlyIface(ifn, QuotaShared);
         if (sharedQuotaIfaces.empty()) {
-            quotaCmd = makeIptablesQuotaCmd(IptOpInsert, costName, maxBytes);
+            quotaCmd = makeIptablesQuotaCmd(IptFullOpInsert, costName, maxBytes);
             res |= runIpxtablesCmd(quotaCmd.c_str(), IptJumpReject);
             if (res) {
                 ALOGE("Failed set quota rule");
@@ -667,7 +627,7 @@ int BandwidthController::removeInterfaceSharedQuota(const char *iface) {
 
     if (sharedQuotaIfaces.empty()) {
         std::string quotaCmd;
-        quotaCmd = makeIptablesQuotaCmd(IptOpDelete, costName, sharedQuotaBytes);
+        quotaCmd = makeIptablesQuotaCmd(IptFullOpDelete, costName, sharedQuotaBytes);
         res |= runIpxtablesCmd(quotaCmd.c_str(), IptJumpReject);
         sharedQuotaBytes = 0;
         if (sharedAlertBytes) {
@@ -719,7 +679,7 @@ int BandwidthController::setInterfaceQuota(const char *iface, int64_t maxBytes)
          * or else a naughty app could just eat up the quota.
          * So we append here.
          */
-        quotaCmd = makeIptablesQuotaCmd(IptOpAppend, costName, maxBytes);
+        quotaCmd = makeIptablesQuotaCmd(IptFullOpAppend, costName, maxBytes);
         res |= runIpxtablesCmd(quotaCmd.c_str(), IptJumpReject);
         if (res) {
             ALOGE("Failed set quota rule");
@@ -829,19 +789,9 @@ int BandwidthController::updateQuota(const char *quotaName, int64_t bytes) {
 }
 
 int BandwidthController::runIptablesAlertCmd(IptOp op, const char *alertName, int64_t bytes) {
-    const char *opFlag;
+    const char *opFlag = opToString(op);
     std::string alertQuotaCmd = "*filter\n";
 
-    switch (op) {
-    case IptOpInsert:
-        opFlag = "-I";
-        break;
-    default:
-    case IptOpDelete:
-        opFlag = "-D";
-        break;
-    }
-
     // TODO: consider using an alternate template for the delete that does not include the --quota
     // value. This code works because the --quota value is ignored by deletes
     StringAppendF(&alertQuotaCmd, ALERT_IPT_TEMPLATE, opFlag, "bw_INPUT", bytes, alertName);
@@ -852,19 +802,8 @@ int BandwidthController::runIptablesAlertCmd(IptOp op, const char *alertName, in
 }
 
 int BandwidthController::runIptablesAlertFwdCmd(IptOp op, const char *alertName, int64_t bytes) {
-    const char *opFlag;
+    const char *opFlag = opToString(op);
     std::string alertQuotaCmd = "*filter\n";
-
-    switch (op) {
-    case IptOpInsert:
-        opFlag = "-I";
-        break;
-    default:
-    case IptOpDelete:
-        opFlag = "-D";
-        break;
-    }
-
     StringAppendF(&alertQuotaCmd, ALERT_IPT_TEMPLATE, opFlag, "bw_FORWARD", bytes, alertName);
     StringAppendF(&alertQuotaCmd, "COMMIT\n");
 
@@ -1296,3 +1235,28 @@ void BandwidthController::parseAndFlushCostlyTables(const std::string& ruleList,
     clearCommands.push_back("COMMIT\n");
     iptablesRestoreFunction(V4V6, android::base::Join(clearCommands, '\n'), nullptr);
 }
+
+inline const char *BandwidthController::opToString(IptOp op) {
+    switch (op) {
+    case IptOpInsert:
+        return "-I";
+    case IptOpDelete:
+        return "-D";
+    }
+}
+
+inline const char *BandwidthController::jumpToString(IptJumpOp jumpHandling) {
+    /*
+     * Must be careful what one rejects with, as upper layer protocols will just
+     * keep on hammering the device until the number of retries are done.
+     * For port-unreachable (default), TCP should consider as an abort (RFC1122).
+     */
+    switch (jumpHandling) {
+    case IptJumpNoAdd:
+        return "";
+    case IptJumpReject:
+        return " --jump REJECT";
+    case IptJumpReturn:
+        return " --jump RETURN";
+    }
+}
diff --git a/server/BandwidthController.h b/server/BandwidthController.h
index 0398ce9b..0a51346b 100644
--- a/server/BandwidthController.h
+++ b/server/BandwidthController.h
@@ -124,9 +124,9 @@ protected:
     };
 
     enum IptIpVer { IptIpV4, IptIpV6 };
-    enum IptOp { IptOpInsert, IptOpReplace, IptOpDelete, IptOpAppend };
+    enum IptFullOp { IptFullOpInsert, IptFullOpDelete, IptFullOpAppend };
     enum IptJumpOp { IptJumpReject, IptJumpReturn, IptJumpNoAdd };
-    enum SpecialAppOp { SpecialAppOpAdd, SpecialAppOpRemove };
+    enum IptOp { IptOpInsert, IptOpDelete };
     enum QuotaType { QuotaUnique, QuotaShared };
     enum RunCmdErrHandling { RunCmdFailureBad, RunCmdFailureOk };
 #if LOG_NDEBUG
@@ -137,15 +137,15 @@ protected:
 
     int manipulateSpecialApps(int numUids, char *appStrUids[],
                                const char *chain,
-                               IptJumpOp jumpHandling, SpecialAppOp appOp);
-    int manipulateNaughtyApps(int numUids, char *appStrUids[], SpecialAppOp appOp);
-    int manipulateNiceApps(int numUids, char *appStrUids[], SpecialAppOp appOp);
+                               IptJumpOp jumpHandling, IptOp appOp);
+    int manipulateNaughtyApps(int numUids, char *appStrUids[], IptOp appOp);
+    int manipulateNiceApps(int numUids, char *appStrUids[], IptOp appOp);
 
     int prepCostlyIface(const char *ifn, QuotaType quotaType);
     int cleanupCostlyIface(const char *ifn, QuotaType quotaType);
 
     std::string makeIptablesSpecialAppCmd(IptOp op, int uid, const char *chain);
-    std::string makeIptablesQuotaCmd(IptOp op, const char *costName, int64_t quota);
+    std::string makeIptablesQuotaCmd(IptFullOp op, const char *costName, int64_t quota);
 
     int runIptablesAlertCmd(IptOp op, const char *alertName, int64_t bytes);
     int runIptablesAlertFwdCmd(IptOp op, const char *alertName, int64_t bytes);
@@ -223,6 +223,10 @@ protected:
     static int (*execFunction)(int, char **, int *, bool, bool);
     static FILE *(*popenFunction)(const char *, const char *);
     static int (*iptablesRestoreFunction)(IptablesTarget, const std::string&, std::string *);
+
+private:
+    static const char *opToString(IptOp op);
+    static const char *jumpToString(IptJumpOp jumpHandling);
 };
 
 #endif