Made testserver communicate to parent process with JSON

net/test/test_server_posix.cc

Index: net/test/test_server_posix.cc
diff --git a/net/test/test_server_posix.cc b/net/test/test_server_posix.cc
index 9c0210b7d28429901ffec57384e7e3f6a7fc3c95..e5422d2777fed754e8667b19bfbcf240fd4844c3 100644
--- a/net/test/test_server_posix.cc
+++ b/net/test/test_server_posix.cc
@@ -97,22 +97,21 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
   return true;
 }
 
-bool TestServer::WaitToStart() {
-  uint16 port;
-  uint8* buffer = reinterpret_cast<uint8*>(&port);
+namespace {
+
+bool ReadData(base::TimeDelta* remaining_time,

[cbentzel, 2010/11/17 22:32:50]

General style I've seen is only one anonymous namespace per file. Not sure if
that's a "style guide" standard or just convention.

[akalin, 2010/11/17 23:21:35]

Yeah...I like putting utility functions close to where they're used.

[cbentzel, 2010/11/18 01:47:19]

I prefer your approach as well.

+              int fd, ssize_t bytes_max, uint8* buffer) {
   ssize_t bytes_read = 0;
-  ssize_t bytes_max = sizeof(port);
-  base::TimeDelta remaining_time = base::TimeDelta::FromMilliseconds(
-      TestTimeouts::action_max_timeout_ms());
   base::Time previous_time = base::Time::Now();
   while (bytes_read < bytes_max) {
     struct pollfd poll_fds[1];
 
-    poll_fds[0].fd = child_fd_;
+    poll_fds[0].fd = fd;
     poll_fds[0].events = POLLIN | POLLPRI;
     poll_fds[0].revents = 0;
 
-    int rv = HANDLE_EINTR(poll(poll_fds, 1, remaining_time.InMilliseconds()));
+    int rv = HANDLE_EINTR(poll(poll_fds, 1,
+                               remaining_time->InMilliseconds()));
     if (rv != 1) {
       LOG(ERROR) << "Failed to poll for the child file descriptor.";
       return false;
@@ -121,21 +120,36 @@ bool TestServer::WaitToStart() {
     base::Time current_time = base::Time::Now();
     base::TimeDelta elapsed_time_cycle = current_time - previous_time;
     DCHECK(elapsed_time_cycle.InMilliseconds() >= 0);
-    remaining_time -= elapsed_time_cycle;
+    *remaining_time -= elapsed_time_cycle;
     previous_time = current_time;
 
-    ssize_t num_bytes = HANDLE_EINTR(read(child_fd_, buffer + bytes_read,
+    ssize_t num_bytes = HANDLE_EINTR(read(fd, buffer + bytes_read,
                                           bytes_max - bytes_read));
     if (num_bytes <= 0)
-      break;
+      return false;
     bytes_read += num_bytes;
   }
+  return true;
+}
+
+}  // namespace
+
+bool TestServer::WaitToStart() {
+  file_util::ScopedFD child_fd_closer(child_fd_closer_.release());

[cbentzel, 2010/11/17 22:32:50]

Nice change over explicit reset. 

+
+  base::TimeDelta remaining_time = base::TimeDelta::FromMilliseconds(
+      TestTimeouts::action_max_timeout_ms());
+  uint32 server_data_len = 0;
+  if (!ReadData(&remaining_time, child_fd_, sizeof server_data_len,

[cbentzel, 2010/11/17 22:32:50]

I haven't seen "sizeof <expression>" before, but it looks valid for unary
expressions.

I'd suggest sizeof(server_data_len) so it looks less like a typo, but not
strongly

[akalin, 2010/11/17 23:21:35]

Done.

+                reinterpret_cast<uint8*>(&server_data_len)))
+    return false;
+  std::string server_data(server_data_len, '\0');

[cbentzel, 2010/11/17 22:32:50]

Why can't you just reserve server_data_len bytes instead of setting characters?
Not worried about perf, but reading this code leads me to believe there will be
<= server_data_len bytes in server_data.

[akalin, 2010/11/17 23:21:35]

reserve() doesn't actually change the size of the string.  Although &str[0]
would probably still work, it's probably still technically undefined behavior.

[cbentzel, 2010/11/18 01:47:19]

You're right, I wasn't thinking. 

+  if (!ReadData(&remaining_time, child_fd_, server_data_len,
+                reinterpret_cast<uint8*>(&server_data[0])))
+    return false;
 
-  // We don't need the FD anymore.
-  child_fd_closer_.reset(NULL);
-  if (bytes_read < bytes_max)
+  if (!ParseServerData(server_data))
     return false;
-  host_port_pair_.set_port(port);
   return true;
 }