Add --serial-id option to host_forwarder.

Add --serial-id option to host_forwarder.                                                                                                                                                                                                                                                                                      

This moves the forwarder's control channel set up logic (which can be
considered as an internal implementation detail) from the Python script to the 
host_forwarder binary.

This also changes the command line interface for the host_forwarder and
device_forwarder binaries.

BUG=242846
R=bulach@chromium.org, digit@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209945

[bulach, 2013-07-02 12:42:49]

(tonyg@ fyi)

lgtm, cool, thanks!! I hope this will help the perf bots reliability! one small
suggestion below:

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:270: for (int i = 1; i < argc;
++i) {
only saw this now, sorry..
rather than iterating through argv, how about using CommandLine directly?


bool kill_server = CommandLine::ForCurrentProcess()->HasSwitch("kill-server");
if (CommandLine::ForCurrentProcess()->HasSwitch("--serial-id"))
 
pickle.WriteString(CommandLine::ForCurrentProcess()->GetSwitchValueASCII("--serial-id"));
StringVector ports = CommandLine::ForCurrentProcess()->GetArgs();
for (StringVector::const_iterator port = ports.begin(); port != port.end();
++port) {
  int numeric_port;
  if (!base::StringToInt(*port, &numeric_port))
    ExitWithUsage(argv[0]);
  pickle.WriteInt(numeric_port);
}

[Philippe, 2013-07-02 12:51:22]

thanks Marcus!

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:270: for (int i = 1; i < argc;
++i) {
On 2013/07/02 12:42:49, bulach wrote:
> only saw this now, sorry..
> rather than iterating through argv, how about using CommandLine directly?
> 
> 
> bool kill_server = CommandLine::ForCurrentProcess()->HasSwitch("kill-server");
> if (CommandLine::ForCurrentProcess()->HasSwitch("--serial-id"))
>  
>
pickle.WriteString(CommandLine::ForCurrentProcess()->GetSwitchValueASCII("--serial-id"));
> StringVector ports = CommandLine::ForCurrentProcess()->GetArgs();
> for (StringVector::const_iterator port = ports.begin(); port != port.end();
> ++port) {
>   int numeric_port;
>   if (!base::StringToInt(*port, &numeric_port))
>     ExitWithUsage(argv[0]);
>   pickle.WriteInt(numeric_port);
> }

Yeah that was the initial code until I notice that it was not working with
negative ports :) We use negative ports to unbind a previous mapping. The '-'
sign makes CommandLine believe that the following integer is an option rather
than an arg. What do you think?

[bulach, 2013-07-02 13:13:06]

still lgtm, but perhaps if we also change the python side, the interface between
that and the native code would be a bit clearer..

https://chromiumcodereview.appspot.com/18354005/diff/10001/build/android/pyli...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/build/android/pyli...
build/android/pylib/forwarder.py:122: # Please note the minus sign below.
how about changing this too? :)
could remove the command, then pass '-u=' + str(device_port).. wdyt?

[digit1, 2013-07-02 15:32:17]

lgtm

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:194: device_serial.empty() ?
I assume you wanted to use serial_part.c_str() here instead?

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:270: for (int i = 1; i < argc;
++i) {
I think the current code is largely enough, but please add a comment to explain
why CommandLine cannot be used here :-)

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:271: if (!strncmp(argv[i],
"kill-server", 11)) {
I'd recommend using:

  if (!strcmp(argv[i], "kill-server')) {
    ..
  }

  static const char kSerialIdPrefix[] = "--serial-id=";
  static const size_t kSerialIdPrefixLen = sizeof(kSerialIdPrefix) - 1;

  if (!strncmp(argv[i], kSerialIdPrefix, kSerialIdPrefixLen)) {
    pickle.WriteString(argv[i] + kSerialIdPrefixLen);
    continue;
  }

This is more error-proof.

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:282: if
(!base::StringToInt(argv[i], &port))
Might add a comment here explaining that negative ports are ok.
What about value 0? Has it a special meaning or is it an error to use it?

Want to check the range too (e.g. -65535 <= port <= 65535) ?

[Philippe, 2013-07-02 15:58:25]

Thanks guys!

https://chromiumcodereview.appspot.com/18354005/diff/10001/build/android/pyli...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/build/android/pyli...
build/android/pylib/forwarder.py:122: # Please note the minus sign below.
On 2013/07/02 13:13:06, bulach wrote:
> how about changing this too? :)
> could remove the command, then pass '-u=' + str(device_port).. wdyt?

Yeah, good point :) The '-' sign was introduced initially as a hack to avoid
breaking the command line interface but this CL is breaking it anyway so there
is no reason not to improve the whole thing :)

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:194: device_serial.empty() ?
On 2013/07/02 15:32:18, digit1 wrote:
> I assume you wanted to use serial_part.c_str() here instead?

Oops :)

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:270: for (int i = 1; i < argc;
++i) {
On 2013/07/02 15:32:18, digit1 wrote:
> I think the current code is largely enough, but please add a comment to
explain
> why CommandLine cannot be used here :-)

I moved to CommandLine since I changed the command line interface in the last
patch set.

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:271: if (!strncmp(argv[i],
"kill-server", 11)) {
On 2013/07/02 15:32:18, digit1 wrote:
> I'd recommend using:
> 
>   if (!strcmp(argv[i], "kill-server')) {
>     ..
>   }
> 
>   static const char kSerialIdPrefix[] = "--serial-id=";
>   static const size_t kSerialIdPrefixLen = sizeof(kSerialIdPrefix) - 1;
> 
>   if (!strncmp(argv[i], kSerialIdPrefix, kSerialIdPrefixLen)) {
>     pickle.WriteString(argv[i] + kSerialIdPrefixLen);
>     continue;
>   }
> 
> This is more error-proof.

Yes, good point (although I'm not using strncmp() anymore).

https://chromiumcodereview.appspot.com/18354005/diff/10001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:282: if
(!base::StringToInt(argv[i], &port))
On 2013/07/02 15:32:18, digit1 wrote:
> Might add a comment here explaining that negative ports are ok.
> What about value 0? Has it a special meaning or is it an error to use it?
> 
> Want to check the range too (e.g. -65535 <= port <= 65535) ?

Good point. I also changed this while addressing Marcus' comments and added a
--unmap option which now expects a positive integer.

[bulach, 2013-07-02 16:12:37]

lgtm, tiny nit:

https://chromiumcodereview.appspot.com/18354005/diff/19001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/19001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:118: }
nit: 113-118 could be moved down to 137, with a CHECK(device_port >= 0); instead
of an if.

[Philippe, 2013-07-02 16:24:19]

https://chromiumcodereview.appspot.com/18354005/diff/19001/tools/android/forw...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18354005/diff/19001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:118: }
On 2013/07/02 16:12:38, bulach wrote:
> nit: 113-118 could be moved down to 137, with a CHECK(device_port >= 0);
instead
> of an if.

Yes, good idea.

[commit-bot: I haz the power, 2013-07-02 17:28:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/18354005/31001

[commit-bot: I haz the power, 2013-07-02 21:56:47]

Failed to apply patch for build/android/pylib/forwarder.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/android/pylib/forwarder.py
  Hunk #2 FAILED at 68.
  Hunk #3 succeeded at 97 (offset -1 lines).
  Hunk #4 succeeded at 105 (offset -1 lines).
  Hunk #5 succeeded at 119 (offset -1 lines).
  Hunk #6 succeeded at 139 (offset -1 lines).
  Hunk #7 succeeded at 160 (offset -1 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
build/android/pylib/forwarder.py.rej

Patch:       build/android/pylib/forwarder.py
Index: build/android/pylib/forwarder.py
diff --git a/build/android/pylib/forwarder.py b/build/android/pylib/forwarder.py
index
965823ee4dace1d0e3a33e64ecb95220771e9d24..1a5897f24e2c28773002274a9de13aacaab09b91
100644
--- a/build/android/pylib/forwarder.py
+++ b/build/android/pylib/forwarder.py
@@ -24,10 +24,6 @@ def _MakeBinaryPath(build_type, binary_name):
 class Forwarder(object):
   """Thread-safe class to manage port forwards from the device to the host."""
 
-  # Unix Abstract socket path:
-  _DEVICE_ADB_CONTROL_PORT = 'chrome_device_forwarder'
-  _TIMEOUT_SECS = 30
-
   _DEVICE_FORWARDER_FOLDER = (constants.TEST_EXECUTABLE_DIR +
                               '/forwarder/')
   _DEVICE_FORWARDER_PATH = (constants.TEST_EXECUTABLE_DIR +
@@ -72,18 +68,15 @@ class Forwarder(object):
     """
     with self._lock:
       self._InitDeviceLocked(tool)
-      self._InitHostLocked()
       redirection_commands = [
-          '%d:%d:%d:%s' % (self._host_adb_control_port, device, host,
-                           host_name) for device, host in port_pairs]
-      logging.info('Command format: <ADB port>:<Device port>' +
-                   '[:<Forward to port>:<Forward to address>]')
+          ['--serial-id=' + self._adb.Adb().GetSerialNumber(), '--map',
+           str(device), str(host)] for device, host in port_pairs]
       logging.info('Forwarding using commands: %s', redirection_commands)
 
       for redirection_command in redirection_commands:
         try:
           (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-              [self._host_forwarder_path, redirection_command])
+              [self._host_forwarder_path] + redirection_command)
         except OSError as e:
           if e.errno == 2:
             raise Exception('Unable to start host forwarder. Make sure you
have'
@@ -102,19 +95,6 @@ class Forwarder(object):
         logging.info('Forwarding device port: %d to host port: %d.',
                      device_port, host_port)
 
-  def _InitHostLocked(self):
-    """Initializes the host forwarder process (only once)."""
-    if self._host_adb_control_port:
-      return
-    self._host_adb_control_port = ports.AllocateTestServerPort()
-    if not self._host_adb_control_port:
-      raise Exception('Failed to allocate a TCP port in the host machine.')
-    if cmd_helper.RunCmd(
-        ['adb', '-s', self._adb._adb.GetSerialNumber(), 'forward',
-         'tcp:%s' % self._host_adb_control_port,
-         'localabstract:%s' % Forwarder._DEVICE_ADB_CONTROL_PORT]) != 0:
-      raise Exception('Error while running adb forward.')
-
   def _InitDeviceLocked(self, tool):
     """Initializes the device forwarder process (only once)."""
     if self._device_initialized:
@@ -123,9 +103,8 @@ class Forwarder(object):
         self._device_forwarder_path_on_host,
         Forwarder._DEVICE_FORWARDER_FOLDER)
     (exit_code, output) = self._adb.GetShellCommandStatusAndOutput(
-        '%s %s %s %s' % (Forwarder._LD_LIBRARY_PATH, tool.GetUtilWrapper(),
-                         Forwarder._DEVICE_FORWARDER_PATH,
-                         Forwarder._DEVICE_ADB_CONTROL_PORT))
+        '%s %s %s' % (Forwarder._LD_LIBRARY_PATH, tool.GetUtilWrapper(),
+                      Forwarder._DEVICE_FORWARDER_PATH))
     if exit_code != 0:
       raise Exception(
           'Failed to start device forwarder:\n%s' % '\n'.join(output))
@@ -138,11 +117,11 @@ class Forwarder(object):
       device_port: A previously forwarded port (through Run()).
     """
     with self._lock:
-      # Please note the minus sign below.
-      redirection_command = '%d:-%d' % (
-          self._host_adb_control_port, device_port)
+      redirection_command = [
+          '--serial-id=' + self._adb.Adb().GetSerialNumber(), '--unmap',
+          str(device_port)]
       (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-          [self._host_forwarder_path, redirection_command])
+          [self._host_forwarder_path] + redirection_command)
       if exit_code != 0:
         raise Exception('%s exited with %d:\n%s' % (
             self._host_forwarder_path, exit_code, '\n'.join(output)))
@@ -158,7 +137,7 @@ class Forwarder(object):
     host_forwarder_path = _MakeBinaryPath(build_type, 'host_forwarder')
     assert os.path.exists(host_forwarder_path), 'Please build forwarder2'
     (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-        [host_forwarder_path, 'kill-server'])
+        [host_forwarder_path, '--kill-server'])
     if exit_code != 0:
       (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
           ['pkill', 'host_forwarder'])
@@ -179,8 +158,8 @@ class Forwarder(object):
     if not adb.FileExistsOnDevice(Forwarder._DEVICE_FORWARDER_PATH):
       return
     (exit_code, output) = adb.GetShellCommandStatusAndOutput(
-        '%s %s kill-server' % (tool.GetUtilWrapper(),
-                               Forwarder._DEVICE_FORWARDER_PATH))
+        '%s %s --kill-server' % (tool.GetUtilWrapper(),
+                                 Forwarder._DEVICE_FORWARDER_PATH))
     # TODO(pliard): Remove the following call to KillAllBlocking() when we are
     # sure that the old version of device_forwarder (not supporting
     # 'kill-server') is not running on the bots anymore.

[Philippe, 2013-07-03 10:21:31]

Committed patchset #6 manually as r209945 (presubmit successful).