Make host_forwarder better handle duplicate forwards.

Make host_forwarder better handle duplicate forwards.

host_forwarder used to rely on device_forwarder to handle requests that map
already forwarded ports. The problem is that device_forwarder was closing and
re-creating the connection to handle such requests. This could make some tests
fail (e.g. python driven tests) if they were trying to forward an already
forwarded port.

BUG=242846
R=bulach@chromium.org

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

[Philippe, 2013-07-08 13:58:27]

Hi Marcus,

I believe this will fix the python-driven tests downstream. I intend to reland
the forwarder2/telemetry CL after this one.

[bulach, 2013-07-08 14:35:36]

lgtm, small suggestion below:

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:140: // The port is already
forwarded.
nit: maybe expand this a little bit, and add a log to differentiate from 156
below?

// The host already knows about this forward.
// The device side may have been restarted, so
// send the message again to reestablish the link.
LOG(INFO) << "Re-forwarding device port " << device_port << " to host port " <<
host_port;

[Philippe, 2013-07-08 15:47:49]

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:140: // The port is already
forwarded.
On 2013/07/08 14:35:36, bulach wrote:
> nit: maybe expand this a little bit, and add a log to differentiate from 156
> below?
> 
> // The host already knows about this forward.
> // The device side may have been restarted, so
> // send the message again to reestablish the link.
> LOG(INFO) << "Re-forwarding device port " << device_port << " to host port "
<<
> host_port;

Note that this wasn't unfortunately doing what your (C++) comment is saying :)
I'm preparing a more robust patch set to follow your comment.

[Philippe, 2013-07-08 17:00:38]

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/18051018/diff/1/tools/android/forwarde...
tools/android/forwarder2/host_forwarder_main.cc:140: // The port is already
forwarded.
On 2013/07/08 15:47:50, Philippe wrote:
> On 2013/07/08 14:35:36, bulach wrote:
> > nit: maybe expand this a little bit, and add a log to differentiate from 156
> > below?
> > 
> > // The host already knows about this forward.
> > // The device side may have been restarted, so
> > // send the message again to reestablish the link.
> > LOG(INFO) << "Re-forwarding device port " << device_port << " to host port "
> <<
> > host_port;
> 
> Note that this wasn't unfortunately doing what your (C++) comment is saying :)
> I'm preparing a more robust patch set to follow your comment.

The other patch set I mentioned is a much bigger change. I would like to land
this one before to fix the build downstream if that's ok with you.

[bulach, 2013-07-08 17:40:35]

lgtm

https://codereview.chromium.org/18051018/diff/1/tools/android/forwarder2/host...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/18051018/diff/1/tools/android/forwarder2/host...
tools/android/forwarder2/host_forwarder_main.cc:140: // The port is already
forwarded.
On 2013/07/08 17:00:38, Philippe wrote:
> On 2013/07/08 15:47:50, Philippe wrote:
> > On 2013/07/08 14:35:36, bulach wrote:
> > > nit: maybe expand this a little bit, and add a log to differentiate from
156
> > > below?
> > > 
> > > // The host already knows about this forward.
> > > // The device side may have been restarted, so
> > > // send the message again to reestablish the link.
> > > LOG(INFO) << "Re-forwarding device port " << device_port << " to host port
"
> > <<
> > > host_port;
> > 
> > Note that this wasn't unfortunately doing what your (C++) comment is saying
:)
> > I'm preparing a more robust patch set to follow your comment.
> 
> The other patch set I mentioned is a much bigger change. I would like to land
> this one before to fix the build downstream if that's ok with you.

sgtm.. would you like to add the LOG just in case, so at least we'll have some
information to distinguish the cases?

[Philippe, 2013-07-09 08:06:04]

On 2013/07/08 17:40:35, bulach wrote:
> lgtm
> 
>
https://codereview.chromium.org/18051018/diff/1/tools/android/forwarder2/host...
> File tools/android/forwarder2/host_forwarder_main.cc (right):
> 
>
https://codereview.chromium.org/18051018/diff/1/tools/android/forwarder2/host...
> tools/android/forwarder2/host_forwarder_main.cc:140: // The port is already
> forwarded.
> On 2013/07/08 17:00:38, Philippe wrote:
> > On 2013/07/08 15:47:50, Philippe wrote:
> > > On 2013/07/08 14:35:36, bulach wrote:
> > > > nit: maybe expand this a little bit, and add a log to differentiate from
> 156
> > > > below?
> > > > 
> > > > // The host already knows about this forward.
> > > > // The device side may have been restarted, so
> > > > // send the message again to reestablish the link.
> > > > LOG(INFO) << "Re-forwarding device port " << device_port << " to host
port
> "
> > > <<
> > > > host_port;
> > > 
> > > Note that this wasn't unfortunately doing what your (C++) comment is
saying
> :)
> > > I'm preparing a more robust patch set to follow your comment.
> > 
> > The other patch set I mentioned is a much bigger change. I would like to
land
> > this one before to fix the build downstream if that's ok with you.
> 
> sgtm.. would you like to add the LOG just in case, so at least we'll have some
> information to distinguish the cases?

Thanks Marcus! I added the log statement and modified the patch very slightly.
It wasn't working with dynamic port allocation. I will submit this and send the
more robust version that I mentioned for review later today.

[Philippe, 2013-07-09 14:10:23]

Committed patchset #2 manually as r210557 (presubmit successful).