Merge "Simplify and improve error logging in sendNetlinkRequest."

1 files changed, 42 insertions, 17 deletions

diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index 48c609d6..a50dab32 100644
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -133,6 +133,19 @@ uint8_t PADDING_BUFFER[RTA_ALIGNTO] = {0, 0, 0, 0};
 
 // END CONSTANTS ----------------------------------------------------------------------------------
 
+const char *actionName(uint16_t action) {
+    static const char *ops[4] = {"adding", "deleting", "getting", "???"};
+    return ops[action % 4];
+}
+
+const char *familyName(uint8_t family) {
+    switch (family) {
+        case AF_INET: return "IPv4";
+        case AF_INET6: return "IPv6";
+        default: return "???";
+    }
+}
+
 // No locks needed because RouteController is accessed only from one thread (in CommandListener).
 std::map<std::string, uint32_t> interfaceToTable;
 
@@ -183,7 +196,7 @@ void updateTableNamesFile() {
     }
 }
 
-// Sends a netlink request and expects an ack.
+// Sends a netlink request and possibly expects an ack.
 // |iov| is an array of struct iovec that contains the netlink message payload.
 // The netlink header is generated by this function based on |action| and |flags|.
 // Returns -errno if there was an error or if the kernel reported an error.
@@ -207,30 +220,37 @@ WARN_UNUSED_RESULT int sendNetlinkRequest(uint16_t action, uint16_t flags, iovec
         nlmsg.nlmsg_len += iov[i].iov_len;
     }
 
-    int ret;
+    int sock = socket(AF_NETLINK, SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_ROUTE);
+    if (sock == -1) {
+        return -errno;
+    }
+
+    int ret = 0;
     struct {
         nlmsghdr msg;
         nlmsgerr err;
     } response;
 
-    int sock = socket(AF_NETLINK, SOCK_DGRAM | SOCK_CLOEXEC, NETLINK_ROUTE);
-    if (sock != -1 &&
-            connect(sock, reinterpret_cast<const sockaddr*>(&NETLINK_ADDRESS),
-                    sizeof(NETLINK_ADDRESS)) != -1 &&
-            writev(sock, iov, iovlen) != -1 &&
-            (ret = recv(sock, &response, sizeof(response), 0)) != -1) {
+
+    if (connect(sock, reinterpret_cast<const sockaddr*>(&NETLINK_ADDRESS),
+                sizeof(NETLINK_ADDRESS)) == -1 || writev(sock, iov, iovlen) == -1) {
+        ret = -errno;
+        ALOGE("netlink socket connect/writev failed (%s)", strerror(-ret));
+        close(sock);
+        return ret;
+    }
+
+    if (flags & NLM_F_ACK) {
+        ret = recv(sock, &response, sizeof(response), 0);
         if (ret == sizeof(response)) {
             ret = response.err.error;  // Netlink errors are negative errno.
-            if (ret) {
-                ALOGE("netlink response contains error (%s)", strerror(-ret));
-            }
+        } else if (ret == -1) {
+            ret = -errno;
+            ALOGE("netlink recv failed (%s)", strerror(-ret));
         } else {
             ALOGE("bad netlink response message size (%d != %zu)", ret, sizeof(response));
             ret = -EBADMSG;
         }
-    } else {
-        ALOGE("netlink socket/connect/writev/recv failed (%s)", strerror(errno));
-        ret = -errno;
     }
 
     if (sock != -1) {
@@ -353,6 +373,8 @@ WARN_UNUSED_RESULT int modifyIpRule(uint16_t action, uint32_t priority, uint8_t
     for (size_t i = 0; i < ARRAY_SIZE(AF_FAMILIES); ++i) {
         rule.family = AF_FAMILIES[i];
         if (int ret = sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov))) {
+            ALOGE("Error %s %s rule: %s", actionName(action), familyName(rule.family),
+                  strerror(-ret));
             return ret;
         }
     }
@@ -458,7 +480,12 @@ WARN_UNUSED_RESULT int modifyIpRoute(uint16_t action, uint32_t table, const char
 
     uint16_t flags = (action == RTM_NEWROUTE) ? NETLINK_CREATE_REQUEST_FLAGS :
                                                 NETLINK_REQUEST_FLAGS;
-    return sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov));
+    int ret = sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov));
+    if (ret) {
+        ALOGE("Error %s route %s -> %s %s to table %u: %s",
+              actionName(action), destination, nexthop, interface, table, strerror(-ret));
+    }
+    return ret;
 }
 
 // An iptables rule to mark incoming packets on a network with the netId of the network.
@@ -720,12 +747,10 @@ int configureDummyNetwork() {
     }
 
     if ((ret = modifyIpRoute(RTM_NEWROUTE, table, interface, "0.0.0.0/0", NULL))) {
-        ALOGE("Can't add IPv4 default route to %s: %s", interface, strerror(-ret));
         return ret;
     }
 
     if ((ret = modifyIpRoute(RTM_NEWROUTE, table, interface, "::/0", NULL))) {
-        ALOGE("Can't add IPv6 default route to %s: %s", interface, strerror(-ret));
         return ret;
     }