Support HTTP test-server based net unit tests on Android.

Support HTTP test-server based net unit tests on Android.

This changes forwarder2 to support both test-server spawner and HTTP server
forwarding.

The main issue was that the device_forwarder was killed when instantiating a
second Forwarder (Python object).
Test server based unit tests require two device-to-host redirections, one for
the test server spawner and one for the HTTP server.
The port used by the HTTP server is allocated dynamically which means that we
can't know the full forwarding configuration before we spawn a HTTP server
(through the test server spawner).

This CL changes the forwarder to let it forward new ports while it is running
by making host_forwarder a daemon. This is similar to how ADB works.

This also means that a single host_forwarder process (daemon) can now handle
multiple devices.

BUG=146979
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167167

[Yaron, 2012-10-26 16:01:54]

I'd recommend splitting out the test disables (or enables in this case) to a
follow-on change in case there are stability issues from turning it on.

[Philippe, 2012-10-29 09:14:39]

On 2012/10/26 16:01:54, Yaron wrote:
> I'd recommend splitting out the test disables (or enables in this case) to a
> follow-on change in case there are stability issues from turning it on.

Sure, Yaron. I uploaded a new patch set.

[digit1, 2012-10-29 12:07:32]

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
File tools/android/forwarder2/daemon.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:30: const char kLogFilePath[] =
"/tmp/host_forwarder_log";
Generally speaking, it's better to create UID-specific temp files, in case
several people on the same machine run their own version of the daemon, i.e. use
/tmp/$USER_host_forwarder_log instead.

Same for other daemon-specific files, unless you want to prevent this use case
explicitely.

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:37: if (HANDLE_EINTR(close(fd)) < 0)
Would you want to preserve errno here?

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:135: LOG(ERROR) << "Daemon (pid=" <<
child_pid << ") died unexpectedly with ";
I'm not sure you can use LOG() in a signal handler.

[Philippe, 2012-10-30 13:54:37]

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
File tools/android/forwarder2/daemon.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:30: const char kLogFilePath[] =
"/tmp/host_forwarder_log";
On 2012/10/29 12:07:32, digit1 wrote:
> Generally speaking, it's better to create UID-specific temp files, in case
> several people on the same machine run their own version of the daemon, i.e.
use
> /tmp/$USER_host_forwarder_log instead.
> 
> Same for other daemon-specific files, unless you want to prevent this use case
> explicitely.

I see your point. I would like to prevent this use case as you suggested. I'm
planning on having a build-bot step which shows /tmp/host_forwarder_log.

Note that I made sure that the pre-existence of the file does not make the
daemon creation fail.

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:37: if (HANDLE_EINTR(close(fd)) < 0)
On 2012/10/29 12:07:32, digit1 wrote:
> Would you want to preserve errno here?

Good point.

https://chromiumcodereview.appspot.com/11269036/diff/16002/tools/android/forw...
tools/android/forwarder2/daemon.cc:135: LOG(ERROR) << "Daemon (pid=" <<
child_pid << ") died unexpectedly with ";
On 2012/10/29 12:07:32, digit1 wrote:
> I'm not sure you can use LOG() in a signal handler.

Good catch, indeed. I added common.{cc,h} that provide helper functions that can
be used both on the host and the device.

[felipeg, 2012-10-30 22:37:29]

The overall change looks good.
I still need to look at the python files.

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
File build/android/pylib/base_test_runner.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/base_test_runner.py:85: Forwarder(self.adb,
self.build_type).KillDeviceForwarder()
as said offline: make this method static

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
File build/android/pylib/base_test_sharder.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/base_test_sharder.py:65: Forwarder(None,
self.build_type).KillHostForwarder()
make it static

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
File build/android/pylib/cmd_helper.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/cmd_helper.py:29: def GetCmdOutput(args, cwd=None,
shell=False):
Change this function to call GetCmdStatusAndOutput and return only the stdout,
so we avoid Copy&paste

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

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/forwarder.py:34: build_type: 'Release' or 'Debug'.
can you add an assert(build_type == Release or Debug)

https://chromiumcodereview.appspot.com/11269036/diff/30001/tools/android/forw...
File tools/android/forwarder2/host_controller.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/tools/android/forw...
tools/android/forwarder2/host_controller.cc:96: // device_forwarder should send
a notification to the host.  At the
s/At the moment/currently/

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
File tools/android/forwarder2/daemon.h (right):

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/daemon.h:15: class Daemon {
we could consider having this file in the tools/android/common path or something
like that

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

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:74: char buf[128];
you could set buf to the size of the welcome message

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:86: return
scoped_ptr<Socket>(NULL);
don't you need to do scoped_ptr<Socket>(NULL).Pass() here ?

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:155: const int bytes_read =
client_socket.Read(&command[0], command.length());
Do you think it makes sense to have a Read(string*) method in the Socket class ?
I've seen you using it more than once.
Just an idea, your call.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:190: continue;
Move the host_controller->Start() after the if here.
Because if you need to delete the host_controller after it started you will need
to first kill it with the notifier_fd and join the thread to wait for it to
shutdown nicely.
If you just delete the object you have a race condition.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:248: daemon_socket =
ConnectToDaemon(5 /* tries */, 100 /* idle msec */);
100 msec is a lot
I guess you can use 10 tries and 10 msec

[Philippe, 2012-10-31 10:17:41]

Thanks Felipe.

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
File build/android/pylib/cmd_helper.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/cmd_helper.py:29: def GetCmdOutput(args, cwd=None,
shell=False):
On 2012/10/30 22:37:30, felipeg1 wrote:
> Change this function to call GetCmdStatusAndOutput and return only the stdout,
> so we avoid Copy&paste

Done.

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

https://chromiumcodereview.appspot.com/11269036/diff/30001/build/android/pyli...
build/android/pylib/forwarder.py:34: build_type: 'Release' or 'Debug'.
On 2012/10/30 22:37:30, felipeg1 wrote:
> can you add an assert(build_type == Release or Debug)

Done.

https://chromiumcodereview.appspot.com/11269036/diff/30001/tools/android/forw...
File tools/android/forwarder2/host_controller.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/30001/tools/android/forw...
tools/android/forwarder2/host_controller.cc:96: // device_forwarder should send
a notification to the host.  At the
On 2012/10/30 22:37:30, felipeg1 wrote:
> s/At the moment/currently/

Done.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
File tools/android/forwarder2/daemon.h (right):

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/daemon.h:15: class Daemon {
On 2012/10/30 22:37:30, felipeg1 wrote:
> we could consider having this file in the tools/android/common path or
something
> like that

We can do that in a next change. There is already a (limited) implementation of
daemon spawning in tools/android/common which is used by fakedns I believe.
Replacing it will imply some additional changes.

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

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:74: char buf[128];
On 2012/10/30 22:37:30, felipeg1 wrote:
> you could set buf to the size of the welcome message

Done.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:86: return
scoped_ptr<Socket>(NULL);
On 2012/10/30 22:37:30, felipeg1 wrote:
> don't you need to do scoped_ptr<Socket>(NULL).Pass() here ?

Pass() is a way to emulate move semantic which is not needed here since there is
no copy involved. The returned object is constructed only once on the caller's
stack frame.
If that was the case the compiler would let us know anyway (through a
compilation error) that this is invalid since scoped_ptr<>'s copy constructor is
private.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:155: const int bytes_read =
client_socket.Read(&command[0], command.length());
On 2012/10/30 22:37:30, felipeg1 wrote:
> Do you think it makes sense to have a Read(string*) method in the Socket class
?
> I've seen you using it more than once.
> Just an idea, your call.

I don't feel really strongly about it. There are a few places indeed where we do
the char* -> string conversion. I think this is the only place where I directly
write to the std::string's buffer. To be honest I wasn't really convinced  when
I uploaded the patch that this was nicer than writing to a char[] and then doing
the conversion. &command[0] is a way to violate encapsulation. std::string
doesn't provide any way to obtain write access to its internal buffer (c_str()
and data() return const char*) other than this "hack", for good reasons I think.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:190: continue;
On 2012/10/30 22:37:30, felipeg1 wrote:
> Move the host_controller->Start() after the if here.
> Because if you need to delete the host_controller after it started you will
need
> to first kill it with the notifier_fd and join the thread to wait for it to
> shutdown nicely.
> If you just delete the object you have a race condition.

Good catch.

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:248: daemon_socket =
ConnectToDaemon(5 /* tries */, 100 /* idle msec */);
On 2012/10/30 22:37:30, felipeg1 wrote:
> 100 msec is a lot
> I guess you can use 10 tries and 10 msec

Indeed. I thought about using exponential backoff in the first place but finally
thought it would be slightly overkill in a first CL.
I will add an helper class to common.h in a next CL to make it easy in several
places to implement polling with exponential backoff.

[Philippe, 2012-10-31 10:49:08]

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

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:155: const int bytes_read =
client_socket.Read(&command[0], command.length());
On 2012/10/31 10:17:41, Philippe wrote:
> On 2012/10/30 22:37:30, felipeg1 wrote:
> > Do you think it makes sense to have a Read(string*) method in the Socket
class
> ?
> > I've seen you using it more than once.
> > Just an idea, your call.
> 
> I don't feel really strongly about it. There are a few places indeed where we
do
> the char* -> string conversion. I think this is the only place where I
directly
> write to the std::string's buffer. To be honest I wasn't really convinced 
when
> I uploaded the patch that this was nicer than writing to a char[] and then
doing
> the conversion. &command[0] is a way to violate encapsulation. std::string
> doesn't provide any way to obtain write access to its internal buffer (c_str()
> and data() return const char*) other than this "hack", for good reasons I
think.

I was right here about my concerns. According to the standard there is no
guarantee that std::string's internal buffer is contiguous (as opposed to
std::vector). This allows rope implementations. So violating encapsulation by
writing to &command[0] could lead to invalid writes.

I uploaded a new patch set to fix that.

[felipeg, 2012-11-01 15:23:09]

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

https://chromiumcodereview.appspot.com/11269036/diff/30011/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:248: daemon_socket =
ConnectToDaemon(5 /* tries */, 100 /* idle msec */);
On 2012/10/31 10:17:41, Philippe wrote:
> On 2012/10/30 22:37:30, felipeg1 wrote:
> > 100 msec is a lot
> > I guess you can use 10 tries and 10 msec
> 
> Indeed. I thought about using exponential backoff in the first place but
finally
> thought it would be slightly overkill in a first CL.
> I will add an helper class to common.h in a next CL to make it easy in several
> places to implement polling with exponential backoff.

Exponential backoff is indeed overkill since I guess the case where it will fail
must be rare, and if fail once it will probably fail all the tries and show an
real error.
You can keep it like it is.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
File tools/android/forwarder2/device_controller.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/device_controller.cc:126: socket->Close();
Please remember me to ask you in person what happens when we don't
continue/break here.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
File tools/android/forwarder2/device_listener.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/device_listener.cc:109: if (listener_socket_.exited())
Would it be useful to print a LOG INFO message saying that we got an exit
notification ?

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

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:169: ParseForwardCommand(
ParseForwardCommand can succeed in the client side and fail in the server side
here.
For example, if the command is bigger than 128 chars, or if something weird
happen in the communication and the message gets corrupted.
It would help have an LOG(ERROR) message either here or in the
ParseForwardCommand itself.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:191: host_controller->Start();
Maybe adding a comment about the racecondition regarding the start / delete will
help future changes
thanks

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:257: char buf[256];
Maybe you could use one single constant to set all string buffers you use
throughout the code.
Sometimes you use 256 other times 128, I understand the reason of the
difference, but I guess we can afford spending more ram than that and using one
common value.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
File tools/android/forwarder2/socket.h (right):

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/socket.h:81: // Returns whether Accept() or Connect()
was interrupted after the socket
s/after/because/
Maybe ?

[Philippe, 2012-11-05 12:05:23]

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
File tools/android/forwarder2/device_listener.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/device_listener.cc:109: if (listener_socket_.exited())
On 2012/11/01 15:23:10, felipeg1 wrote:
> Would it be useful to print a LOG INFO message saying that we got an exit
> notification ?

Done.

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

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:169: ParseForwardCommand(
On 2012/11/01 15:23:10, felipeg1 wrote:
> ParseForwardCommand can succeed in the client side and fail in the server side
> here.
> For example, if the command is bigger than 128 chars, or if something weird
> happen in the communication and the message gets corrupted.
> It would help have an LOG(ERROR) message either here or in the
> ParseForwardCommand itself.

Done.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:191: host_controller->Start();
On 2012/11/01 15:23:10, felipeg1 wrote:
> Maybe adding a comment about the racecondition regarding the start / delete
will
> help future changes
> thanks

Do you mean the one related to signal handlers?

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:257: char buf[256];
On 2012/11/01 15:23:10, felipeg1 wrote:
> Maybe you could use one single constant to set all string buffers you use
> throughout the code.
> Sometimes you use 256 other times 128, I understand the reason of the
> difference, but I guess we can afford spending more ram than that and using
one
> common value.

Done.

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
File tools/android/forwarder2/socket.h (right):

https://chromiumcodereview.appspot.com/11269036/diff/54002/tools/android/forw...
tools/android/forwarder2/socket.h:81: // Returns whether Accept() or Connect()
was interrupted after the socket
On 2012/11/01 15:23:10, felipeg1 wrote:
> s/after/because/
> Maybe ?

Done.

[felipeg, 2012-11-05 17:14:06]

LGTM

[Philippe, 2012-11-05 17:33:35]

Thanks Felipe! FYI, I've just uploaded a new patch set (#10).

[Philippe, 2012-11-05 18:01:50]

On 2012/11/05 17:33:35, Philippe wrote:
> Thanks Felipe! FYI, I've just uploaded a new patch set (#10).

Marcus, can you please take a look at this change?

[bulach, 2012-11-05 20:15:07]

lgtm, thanks! just nits (and please make sure digit is also happy with the
overall architecture, he's more familiar than I am)

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

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:17: 
nit: need another \n here, 20, 28 and 37 (that is, two between top-levels)

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:29: def KillDeviceForwarder(adb):
suggestion: how about making all of this @staticmethods in the Forwarder class
itself? the imports would be simpler, and you could also name them as "KillHost"
and "KillDevice", so it's clearer for callers "Forwarder.Kill(Host|Device)"..

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:148: "Forwarding device port: %d to host port:
%d." % (device_port,
nit: single quotes. also, "logging" takes a vararg, so remove % and the tuple,
and just do:
logging.info(fmt, p0, p1);

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

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:178: new
HostController(device_port,
nit: caller sites should be indented like 169, as many as possible in the next
line indented by 4..

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:254: daemon_socket =
ConnectToDaemon(10 /* tries */, 10 /* idle msec */);
nit: make these constants so the comment will be unecessary :)

[Philippe, 2012-11-06 09:58:03]

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

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:17: 
On 2012/11/05 20:15:07, bulach wrote:
> nit: need another \n here, 20, 28 and 37 (that is, two between top-levels)

Done.

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:29: def KillDeviceForwarder(adb):
On 2012/11/05 20:15:07, bulach wrote:
> suggestion: how about making all of this @staticmethods in the Forwarder class
> itself? the imports would be simpler, and you could also name them as
"KillHost"
> and "KillDevice", so it's clearer for callers "Forwarder.Kill(Host|Device)"..

Good idea.

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/forwarder.py:148: "Forwarding device port: %d to host port:
%d." % (device_port,
On 2012/11/05 20:15:07, bulach wrote:
> nit: single quotes. also, "logging" takes a vararg, so remove % and the tuple,
> and just do:
> logging.info(fmt, p0, p1);

Done.

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

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:178: new
HostController(device_port,
On 2012/11/05 20:15:07, bulach wrote:
> nit: caller sites should be indented like 169, as many as possible in the next
> line indented by 4..

Indeed. This indentation style only applies to function declarations.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:254: daemon_socket =
ConnectToDaemon(10 /* tries */, 10 /* idle msec */);
On 2012/11/05 20:15:07, bulach wrote:
> nit: make these constants so the comment will be unecessary :)

Indeed :)

[digit1, 2012-11-06 10:39:24]

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
File build/android/pylib/ports.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/ports.py:94: port_info =
'(.)|(127\.0\.0\.1)|(localhost)\:%d' % host_port
A small comment here explaining what this does would be helpful.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
File tools/android/forwarder2/daemon.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/daemon.cc:63: open(path.c_str(), O_CREAT | O_WRONLY,
0600));
Note: there is a slight race condition here between the creation of the PID file
and the read in Kill() below.

It's probably to create a PID file first with a completely random name, then to
do an unlink() + rename() to ensure that it will appear automatically to the
reader process.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/daemon.cc:148: switch (fork()) {
Do you need a double-fork here to avoid zombies?

See http://fixunix.com/unix/533215-how-avoid-zombie-processes.html

[Philippe, 2012-11-06 16:13:30]

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
File build/android/pylib/ports.py (right):

https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
build/android/pylib/ports.py:94: port_info =
'(.)|(127\.0\.0\.1)|(localhost)\:%d' % host_port
On 2012/11/06 10:39:24, digit1 wrote:
> A small comment here explaining what this does would be helpful.

I replaced the . with what I was actually trying to match. This should be
explicit enough.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
File tools/android/forwarder2/daemon.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/daemon.cc:63: open(path.c_str(), O_CREAT | O_WRONLY,
0600));
On 2012/11/06 10:39:24, digit1 wrote:
> Note: there is a slight race condition here between the creation of the PID
file
> and the read in Kill() below.
> 
> It's probably to create a PID file first with a completely random name, then
to
> do an unlink() + rename() to ensure that it will appear automatically to the
> reader process.

I did what we talked about offline. I'm now fetching the daemon's PID through
the file lock to avoid the race condition.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/daemon.cc:148: switch (fork()) {
On 2012/11/06 10:39:24, digit1 wrote:
> Do you need a double-fork here to avoid zombies?
> 
> See http://fixunix.com/unix/533215-how-avoid-zombie-processes.html

As we discussed offline I think the current implementation should prevent
zombies.
My understanding is that processes go to the zombie state when their parent
doesn't wait for them when they die.
Here we correctly catch the SIG_CHLD and do the wait if the daemon dies before
its parent (the CLI process). If the parent dies before the child (which is the
expected behavior since the daemon is supposed to live longer than the CLI
process) then the daemon is reattached to the init process which will do the
wait.

https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
tools/android/forwarder2/daemon.cc:220: const int kIdleTimeMS = 100;
I decreased this value which was a bit high.

[Philippe, 2012-11-07 08:49:40]

On 2012/11/06 16:13:30, Philippe wrote:
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
> File build/android/pylib/ports.py (right):
> 
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/build/android/pyli...
> build/android/pylib/ports.py:94: port_info =
> '(.)|(127\.0\.0\.1)|(localhost)\:%d' % host_port
> On 2012/11/06 10:39:24, digit1 wrote:
> > A small comment here explaining what this does would be helpful.
> 
> I replaced the . with what I was actually trying to match. This should be
> explicit enough.
> 
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
> File tools/android/forwarder2/daemon.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
> tools/android/forwarder2/daemon.cc:63: open(path.c_str(), O_CREAT | O_WRONLY,
> 0600));
> On 2012/11/06 10:39:24, digit1 wrote:
> > Note: there is a slight race condition here between the creation of the PID
> file
> > and the read in Kill() below.
> > 
> > It's probably to create a PID file first with a completely random name, then
> to
> > do an unlink() + rename() to ensure that it will appear automatically to the
> > reader process.
> 
> I did what we talked about offline. I'm now fetching the daemon's PID through
> the file lock to avoid the race condition.
> 
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
> tools/android/forwarder2/daemon.cc:148: switch (fork()) {
> On 2012/11/06 10:39:24, digit1 wrote:
> > Do you need a double-fork here to avoid zombies?
> > 
> > See http://fixunix.com/unix/533215-how-avoid-zombie-processes.html
> 
> As we discussed offline I think the current implementation should prevent
> zombies.
> My understanding is that processes go to the zombie state when their parent
> doesn't wait for them when they die.
> Here we correctly catch the SIG_CHLD and do the wait if the daemon dies before
> its parent (the CLI process). If the parent dies before the child (which is
the
> expected behavior since the daemon is supposed to live longer than the CLI
> process) then the daemon is reattached to the init process which will do the
> wait.
> 
>
https://chromiumcodereview.appspot.com/11269036/diff/56003/tools/android/forw...
> tools/android/forwarder2/daemon.cc:220: const int kIdleTimeMS = 100;
> I decreased this value which was a bit high.

Please ignore the last patch sets (#{12,13}) until I upload a new one. There are
several things I would like to fix.

[Philippe, 2012-11-09 10:32:52]

FYI, I uploaded a new patch set. When you can review this again David, please
diff against patch set #10.

[Philippe, 2012-11-09 14:05:44]

On 2012/11/09 10:32:52, Philippe wrote:
> FYI, I uploaded a new patch set. When you can review this again David, please
> diff against patch set #10.

I'm CCing ppi@.

[digit1, 2012-11-12 09:16:42]

lgtm

[commit-bot: I haz the power, 2012-11-12 10:43:40]

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

[commit-bot: I haz the power, 2012-11-12 12:29:37]

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

[commit-bot: I haz the power, 2012-11-12 12:46:38]

Change committed as 167167

[Nico, 2013-07-02 01:07:21]

https://chromiumcodereview.appspot.com/11269036/diff/48039/tools/android/forw...
File tools/android/forwarder2/socket.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/48039/tools/android/forw...
tools/android/forwarder2/socket.cc:234: if (!getsockopt(socket_, SOL_SOCKET,
SO_ERROR, &socket_errno, &opt_len) < 0) {
The "!" here is wrong, right?

[Philippe, 2013-07-02 09:35:37]

https://chromiumcodereview.appspot.com/11269036/diff/48039/tools/android/forw...
File tools/android/forwarder2/socket.cc (right):

https://chromiumcodereview.appspot.com/11269036/diff/48039/tools/android/forw...
tools/android/forwarder2/socket.cc:234: if (!getsockopt(socket_, SOL_SOCKET,
SO_ERROR, &socket_errno, &opt_len) < 0) {
On 2013/07/02 01:07:22, Nico wrote:
> The "!" here is wrong, right?

Yeah, good catch!