commit da86b2bade447369bc9a7554c2e56e475b5d0400
author Hugo Benichi <hugobenichi@google.com> Thu Aug 19 14:08:05 2021 +0900
committer Commit Bot <commit-bot@chromium.org> Mon Aug 23 22:53:36 2021 +0000
tree e2733dad62baa0db269fec0344257c7cee89cfbd
parent 0962226e8491fb9622ba010314316cfd678ff439

patchpanel: system-proxy: better error logs for Socket

This patch fixes several logging issues with the use of patchpanel
Socket class.

When socket.cc itself is in charge of logging an error, any context
about the call site is lost. This makes debugging and analyzing issues
from logs much harder than logging the error at the call site. See
b/195507716 for an example.

To fix this, errors are now logged at call sites in patchpanel and
system-proxy.

Additionally, some errors for some Socket calls were until ignored or
incorrectly processed. For instance SendTo() errors in system-proxy were
always ignored since SendTo() < 0 implies SendTo == true.

BUG=b:195507716
TEST= - FEATURES="test" emerge-trogdor system-proxy
      - FEATURES="test" emerge-trogdor patchpanel
      - flashed DUT, started user session, observed patchapnel
      multicast_forwader.cc logs and adb_proxy.cc logs.

Change-Id: I5a472b43c4cb47d2b13d2b7749c857c4865a3ea4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3105860
Reviewed-by: Andreea-Elena Costinas <acostinas@google.com>
Reviewed-by: Garrick Evans <garrick@chromium.org>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Tested-by: Hugo Benichi <hugobenichi@google.com>

patchpanel/socket.cc

diff --git a/patchpanel/socket.cc b/patchpanel/socket.cc
index bd6e248..fea1792 100644
--- a/patchpanel/socket.cc
+++ b/patchpanel/socket.cc
@@ -26,46 +26,26 @@
 }
 }  // namespace
 
-Socket::Socket(int family, int type) : fd_(socket(family, type, 0)) {
-  if (!fd_.is_valid())
-    PLOG(ERROR) << "socket failed (" << family << ", " << type << ")";
-}
+Socket::Socket(int family, int type) : fd_(socket(family, type, 0)) {}
 
-Socket::Socket(base::ScopedFD fd) : fd_(std::move(fd)) {
-  if (!fd_.is_valid())
-    LOG(ERROR) << "invalid fd";
-}
+Socket::Socket(base::ScopedFD fd) : fd_(std::move(fd)) {}
 
 bool Socket::Bind(const struct sockaddr* addr, socklen_t addrlen) {
-  if (bind(fd_.get(), addr, addrlen) < 0) {
-    PLOG(WARNING) << "bind failed: " << *addr;
-    return false;
-  }
-  return true;
+  return bind(fd_.get(), addr, addrlen) == 0;
 }
 
 bool Socket::Connect(const struct sockaddr* addr, socklen_t addrlen) {
-  if (connect(fd_.get(), addr, addrlen) < 0) {
-    PLOG(WARNING) << "connect failed: " << *addr;
-    return false;
-  }
-  return true;
+  return connect(fd_.get(), addr, addrlen) == 0;
 }
 
 bool Socket::Listen(int backlog) const {
-  if (listen(fd_.get(), backlog) != 0) {
-    PLOG(WARNING) << "listen failed";
-    return false;
-  }
-  return true;
+  return listen(fd_.get(), backlog) == 0;
 }
 
 std::unique_ptr<Socket> Socket::Accept(struct sockaddr* addr,
                                        socklen_t* addrlen) const {
   base::ScopedFD fd(accept(fd_.get(), addr, addrlen));
   if (!fd.is_valid()) {
-    if (!WouldBlock())
-      PLOG(WARNING) << "accept failed";
     return nullptr;
   }
   return std::make_unique<Socket>(std::move(fd));
@@ -91,7 +71,6 @@
   if (WouldBlock())
     return 0;
 
-  PLOG(WARNING) << "sendto failed";
   return bytes;
 }
 
@@ -103,15 +82,13 @@
   ssize_t bytes = recvfrom(fd_.get(), data, len, 0, addr, &recvlen);
   if (bytes >= 0) {
     if (recvlen != addrlen)
-      PLOG(WARNING) << "recvfrom failed: unexpected src addr length "
-                    << recvlen;
+      LOG(WARNING) << "recvfrom failed: unexpected src addr length " << recvlen;
     return bytes;
   }
 
   if (WouldBlock())
     return 0;
 
-  PLOG(WARNING) << "recvfrom failed";
   return bytes;
 }