Use the new forwarder2's Daemon implementation in device_forwarder.

Use the new forwarder2's Daemon implementation in device_forwarder.

This lets device_forwarder be controlled (started and killed) like
host_forwarder (see r167167).

This has the main benefit of moving the setup/tear down complexity from the
Python Forwarder wrapper code to the forwarder itself so that various clients
(including WebKit) can use forwarder2 more easily (e.g. no need to use pexpect
anymore (which is not available in WebKit)).

This implied a refactoring in the Daemon class so that its clients don't have
to duplicate the socket-related code.


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

[Philippe, 2012-11-14 14:07:44]

Can you please Felipe do a sanity check before I send this to other reviewers ?
:)

[bulach, 2012-11-14 18:34:35]

+peter in case there are any implications to the webkit side... I'll send
comments shortly :)

[bulach, 2012-11-15 02:08:18]

nice stuff, thanks! small suggestions below:

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/android...
File build/android/pylib/android_commands.py (right):

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/android...
build/android/pylib/android_commands.py:478: return (int(lines[-1]),
lines[:len(lines) - 2])
nit: lines[:-1], cute isn't it? :)

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forward...
File build/android/pylib/forwarder.py (right):

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forward...
build/android/pylib/forwarder.py:87: raise Exception('Failed to start device
forwarder:\n%s' % (
nit: no need for the parens at EOL

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forward...
build/android/pylib/forwarder.py:120: Forwarder._DEVICE_FORWARDER_PATH)
why is it needed? I'd assume that on kill it's already in the device?

http://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forward...
build/android/pylib/forwarder.py:123: # TODO(pliard): Remove the following call
to KillAllBlocking() when we are
isn't the new forwarder pushed on Run? not sure how this scenario would
happen...

http://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/de...
File tools/android/forwarder2/device_forwarder_main.cc (right):

http://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/de...
tools/android/forwarder2/device_forwarder_main.cc:50: class DaemonDelegate :
public Daemon::ServerDelegate {
nit: it'd be clearer as ServerDelegate

http://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/ho...
File tools/android/forwarder2/host_forwarder_main.cc (right):

http://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/ho...
tools/android/forwarder2/host_forwarder_main.cc:46: PipeNotifier* g_notifier;
nit: = 0; ? otherwise the DCHECK in 50 could be spurious..

http://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/ho...
tools/android/forwarder2/host_forwarder_main.cc:245: Daemon daemon(kLogFilePath,
kPIDFilePath, kDaemonIdentifier, &client_delegate,
nit: needs to be on the next line, indented +4

[Philippe, 2012-11-15 09:56:20]

Thanks Marcus!

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/androi...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/androi...
build/android/pylib/android_commands.py:478: return (int(lines[-1]),
lines[:len(lines) - 2])
On 2012/11/15 02:08:19, bulach wrote:
> nit: lines[:-1], cute isn't it? :)

Indeed :)

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
File build/android/pylib/forwarder.py (right):

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:87: raise Exception('Failed to start device
forwarder:\n%s' % (
On 2012/11/15 02:08:19, bulach wrote:
> nit: no need for the parens at EOL

Thanks for the tip, I thought it was needed to break the line.

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:120: Forwarder._DEVICE_FORWARDER_PATH)
On 2012/11/15 02:08:19, bulach wrote:
> why is it needed? I'd assume that on kill it's already in the device?

I do a very early kill in some places in case the tear down  for the previous
run didn't happen for some reason. I found this bug when I tried this change on
a brand new device.

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:123: # TODO(pliard): Remove the following call
to KillAllBlocking() when we are
On 2012/11/15 02:08:19, bulach wrote:
> isn't the new forwarder pushed on Run? not sure how this scenario would
> happen...

Your question here seems similar to the previous one. In case you are wondering
why this TODO is here:
If there is still an old version of device_forwarder running on the device
(which doesn't support the 'kill-server' command) then pushing the new version
and doing a '$ device_forwarder kill-server' won't be enough. Daemon::Kill()
fetches the daemon's PID (used to do the kill) by looking at a file lock
(acquired by the daemon when it's spawned). The old version of device_forwarder
didn't use this whole mechanism of file locking so a kill-server wouldn't
actually kill the (old) device_forwarder.

https://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/d...
File tools/android/forwarder2/device_forwarder_main.cc (right):

https://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/d...
tools/android/forwarder2/device_forwarder_main.cc:50: class DaemonDelegate :
public Daemon::ServerDelegate {
On 2012/11/15 02:08:19, bulach wrote:
> nit: it'd be clearer as ServerDelegate

Indeed.

https://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/h...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/h...
tools/android/forwarder2/host_forwarder_main.cc:46: PipeNotifier* g_notifier;
On 2012/11/15 02:08:19, bulach wrote:
> nit: = 0; ? otherwise the DCHECK in 50 could be spurious..

This is zeroed at dynamic link-time since it lives in the BSS ELF section but I
do agree that being explicit is better :)

https://codereview.chromium.org/11360248/diff/3010/tools/android/forwarder2/h...
tools/android/forwarder2/host_forwarder_main.cc:245: Daemon daemon(kLogFilePath,
kPIDFilePath, kDaemonIdentifier, &client_delegate,
On 2012/11/15 02:08:19, bulach wrote:
> nit: needs to be on the next line, indented +4

Done.

[felipeg, 2012-11-15 15:54:54]

Thanks for doing this Philippe,
The code is good, although is a little complex now,
but this is life, no pain no gain

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:107: class DaemonDelegate :
public Daemon::ServerDelegate {
One suggestion would be:
make a notifier object be owned by the DaemonDelegate class.
On the Init method of this class you just change the g_notifier pointer.
The g_notifier pointer still needs to be global to be accessed from the signal
handler, but the real object is owned by the DaemonDelegate class.

Also, you can keep the CHECK(!g_notifier) ond the Init() as you have right now.
With this we can be sure that there is only one call to the Init() function of
DaemonDelegate, and also there is no chance of memory leak since the notifier
object is owned.

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:116: DCHECK(!g_notifier);
may need a lock ?
Or add a comment saying this class is not thread safe.

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:117: g_notifier = new
PipeNotifier();
you are initializing a global var in the init method of a class.
If you have more than one object of this class things will leak.
Can you think of a better way to make sure our global var is properly
initialized only once ?
I know this was caused by my initial code where we had the global static method
for initializing this.

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:148: adb_port,
g_notifier->receiver_fd()));
Maybe use GetExitNotifierFD() for consistency ?
also maybe add a DCHECK(g_notifier)

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:244: DaemonDelegate
daemon_delegate;
I don't see you calling daemon_delegate->Init() which initializes the
g_exit_notifier
and you call GetExitNotifierFD below.

[Philippe, 2012-11-15 16:25:18]

Thanks Felipe!

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:107: class DaemonDelegate :
public Daemon::ServerDelegate {
On 2012/11/15 15:54:54, felipeg1 wrote:
> One suggestion would be:
> make a notifier object be owned by the DaemonDelegate class.
> On the Init method of this class you just change the g_notifier pointer.
> The g_notifier pointer still needs to be global to be accessed from the signal
> handler, but the real object is owned by the DaemonDelegate class.
> 
> Also, you can keep the CHECK(!g_notifier) ond the Init() as you have right
now.
> With this we can be sure that there is only one call to the Init() function of
> DaemonDelegate, and also there is no chance of memory leak since the notifier
> object is owned.

See my reply to your comment line 117.

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:116: DCHECK(!g_notifier);
On 2012/11/15 15:54:54, felipeg1 wrote:
> may need a lock ?
> Or add a comment saying this class is not thread safe.

I think it would look a little weird to say that this (private) class (which we
fully control since we are the only client) is not thread-safe in a purely
single-thread context.

What do you think?

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:148: adb_port,
g_notifier->receiver_fd()));
On 2012/11/15 15:54:54, felipeg1 wrote:
> Maybe use GetExitNotifierFD() for consistency ?
> also maybe add a DCHECK(g_notifier)

Sure. The DCHECK is in GetExitNotifierFD().

https://codereview.chromium.org/11360248/diff/13001/tools/android/forwarder2/...
tools/android/forwarder2/host_forwarder_main.cc:244: DaemonDelegate
daemon_delegate;
On 2012/11/15 15:54:54, felipeg1 wrote:
> I don't see you calling daemon_delegate->Init() which initializes the
> g_exit_notifier
> and you call GetExitNotifierFD below.

Init() is called by the Daemon. See daemon.cc:273.

[bulach, 2012-11-15 18:28:10]

thanks philippe! clarifications inline:

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
File build/android/pylib/forwarder.py (right):

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:87: raise Exception('Failed to start device
forwarder:\n%s' % (
On 2012/11/15 09:56:20, Philippe wrote:
> On 2012/11/15 02:08:19, bulach wrote:
> > nit: no need for the parens at EOL
> 
> Thanks for the tip, I thought it was needed to break the line.

yeah, the parens is needed to break line :) however, Exception( had one already,
that is, the interpreter keeps reading the following lines until the first
parens is closed..

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:120: Forwarder._DEVICE_FORWARDER_PATH)
On 2012/11/15 09:56:20, Philippe wrote:
> On 2012/11/15 02:08:19, bulach wrote:
> > why is it needed? I'd assume that on kill it's already in the device?
> 
> I do a very early kill in some places in case the tear down  for the previous
> run didn't happen for some reason. I found this bug when I tried this change
on
> a brand new device.

ahn, I see...
I think this is a real corner case, so it's probably simpler to get around with
adb.FileExistsOnDevice / early return, wdyt?

https://codereview.chromium.org/11360248/diff/3010/build/android/pylib/forwar...
build/android/pylib/forwarder.py:123: # TODO(pliard): Remove the following call
to KillAllBlocking() when we are
On 2012/11/15 09:56:20, Philippe wrote:
> On 2012/11/15 02:08:19, bulach wrote:
> > isn't the new forwarder pushed on Run? not sure how this scenario would
> > happen...
> 
> Your question here seems similar to the previous one. In case you are
wondering
> why this TODO is here:
> If there is still an old version of device_forwarder running on the device
> (which doesn't support the 'kill-server' command) then pushing the new version
> and doing a '$ device_forwarder kill-server' won't be enough. Daemon::Kill()
> fetches the daemon's PID (used to do the kill) by looking at a file lock
> (acquired by the daemon when it's spawned). The old version of
device_forwarder
> didn't use this whole mechanism of file locking so a kill-server wouldn't
> actually kill the (old) device_forwarder.

that makes sense, thanks for the detailed information!

[felipeg, 2012-11-15 18:40:58]

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

https://chromiumcodereview.appspot.com/11360248/diff/13001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:116: DCHECK(!g_notifier);
On 2012/11/15 16:25:19, Philippe wrote:
> On 2012/11/15 15:54:54, felipeg1 wrote:
> > may need a lock ?
> > Or add a comment saying this class is not thread safe.
> 
> I think it would look a little weird to say that this (private) class (which
we
> fully control since we are the only client) is not thread-safe in a purely
> single-thread context.
> 
> What do you think?
I agree.
But a comment will be nice for my future me to remember that when I make a
change that may use this class in multiple places I have to remember that this
is not thread safe :)
But it is nip tic, your call

[Philippe, 2012-11-19 10:22:13]

https://chromiumcodereview.appspot.com/11360248/diff/3010/build/android/pylib...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/11360248/diff/3010/build/android/pylib...
build/android/pylib/forwarder.py:87: raise Exception('Failed to start device
forwarder:\n%s' % (
On 2012/11/15 18:28:10, bulach wrote:
> On 2012/11/15 09:56:20, Philippe wrote:
> > On 2012/11/15 02:08:19, bulach wrote:
> > > nit: no need for the parens at EOL
> > 
> > Thanks for the tip, I thought it was needed to break the line.
> 
> yeah, the parens is needed to break line :) however, Exception( had one
already,
> that is, the interpreter keeps reading the following lines until the first
> parens is closed..

Ah indeed, I should have noticed it :)

https://chromiumcodereview.appspot.com/11360248/diff/3010/build/android/pylib...
build/android/pylib/forwarder.py:120: Forwarder._DEVICE_FORWARDER_PATH)
On 2012/11/15 18:28:10, bulach wrote:
> On 2012/11/15 09:56:20, Philippe wrote:
> > On 2012/11/15 02:08:19, bulach wrote:
> > > why is it needed? I'd assume that on kill it's already in the device?
> > 
> > I do a very early kill in some places in case the tear down  for the
previous
> > run didn't happen for some reason. I found this bug when I tried this change
> on
> > a brand new device.
> 
> ahn, I see...
> I think this is a real corner case, so it's probably simpler to get around
with
> adb.FileExistsOnDevice / early return, wdyt?

Indeed.

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

https://chromiumcodereview.appspot.com/11360248/diff/13001/tools/android/forw...
tools/android/forwarder2/host_forwarder_main.cc:116: DCHECK(!g_notifier);
On 2012/11/15 18:40:58, felipeg1 wrote:
> On 2012/11/15 16:25:19, Philippe wrote:
> > On 2012/11/15 15:54:54, felipeg1 wrote:
> > > may need a lock ?
> > > Or add a comment saying this class is not thread safe.
> > 
> > I think it would look a little weird to say that this (private) class (which
> we
> > fully control since we are the only client) is not thread-safe in a purely
> > single-thread context.
> > 
> > What do you think?
> I agree.
> But a comment will be nice for my future me to remember that when I make a
> change that may use this class in multiple places I have to remember that this
> is not thread safe :)
> But it is nip tic, your call

I wish I could help you here but I don't think this will in that specific case
:) I even think that this comment could be misleading since it could make the
reader think that OnClientConnected() for instance is called on a new thread.
The only client dealing with this class is Daemon therefore you shouldn't have
to worry too much here (you can't reuse it).
If you can think of a non-misleading comment here though I would be glad to add
it. I just think that "this class is not thread-safe" doesn't make a lot of
sense here.

What do you think?

[felipeg, 2012-11-19 14:43:37]

LGTM

please ask david to take a look at the daemon implementation

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
File tools/android/forwarder2/device_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/device_forwarder_main.cc:87: g_notifier->receiver_fd(),
base::Passed(&client_socket)));
You can use the function GetExitNotifierFD() here

[Philippe, 2012-11-19 14:51:43]

Thanks Felipe. I did a sync too.

David, can you please take a look at the changes in daemon.{h,cc}?

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
File tools/android/forwarder2/device_forwarder_main.cc (right):

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/device_forwarder_main.cc:87: g_notifier->receiver_fd(),
base::Passed(&client_socket)));
On 2012/11/19 14:43:37, felipeg1 wrote:
> You can use the function GetExitNotifierFD() here

Done.

[digit1, 2012-11-19 15:27:42]

https://chromiumcodereview.appspot.com/11360248/diff/17002/build/android/pyli...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/11360248/diff/17002/build/android/pyli...
build/android/pylib/android_commands.py:477: command + '; echo $?',
timeout_time, log_result)
Nit: This assumes that the command's output will be newline-terminated. This is
not always guaranteed. One thing I did in one of my implementations of this
wrapper is to do something like that instead:

  adb shell command '; echo %$?'  => lines
  last_line = lines[-1]
  status_pos = last_line.rfind('%')
  assert status_pos >= 0
  status = int(last_line[status_pos+1:])   # extract status
  if status_pos == 0:
    lines = lines[:-1]
  else:
    lines = lines[:-1] + last_line[:status_pos]

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

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/daemon.cc:255: return false;
Returning from this function in case of error in the child seems a bit
dangerous. It's probably better to exit(1) directly instead, since this will be
detected after 200ms by the parent code below.

This is not an issue in this version of the code, because the caller will do a
'return 1' followed by 'exit(1)' in the child process when this happens, but
it's easy to imagine a change in the caller code that doesn't take into account
that the function can return in two different PIDs.

Another commonly implemented option is to make the parent wait for the child by
creating an anonymous pipe and have the child send a single byte through it to
the parent to indicate success/failure. Just mentioning it here, no need to
implement this here right now.

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/daemon.cc:283: signal(SIGCHLD, SigChildHandler);
since you're writing a general Daemon class, you might want to store the hold
SIGCHLD handler, and restore it on scope exit (with an appropriate template).
Use sigprocmask() instead of sigmask() to ensure you save the SA_SIGINFO flag
associated with the previous signal handler.

[Philippe, 2012-11-19 17:18:58]

https://chromiumcodereview.appspot.com/11360248/diff/17002/build/android/pyli...
File build/android/pylib/android_commands.py (right):

https://chromiumcodereview.appspot.com/11360248/diff/17002/build/android/pyli...
build/android/pylib/android_commands.py:477: command + '; echo $?',
timeout_time, log_result)
On 2012/11/19 15:27:43, digit1 wrote:
> Nit: This assumes that the command's output will be newline-terminated. This
is
> not always guaranteed. One thing I did in one of my implementations of this
> wrapper is to do something like that instead:
> 
>   adb shell command '; echo %$?'  => lines
>   last_line = lines[-1]
>   status_pos = last_line.rfind('%')
>   assert status_pos >= 0
>   status = int(last_line[status_pos+1:])   # extract status
>   if status_pos == 0:
>     lines = lines[:-1]
>   else:
>     lines = lines[:-1] + last_line[:status_pos]
> 

Good point.

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

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/daemon.cc:255: return false;
On 2012/11/19 15:27:43, digit1 wrote:
> Returning from this function in case of error in the child seems a bit
> dangerous. It's probably better to exit(1) directly instead, since this will
be
> detected after 200ms by the parent code below.
> 
> This is not an issue in this version of the code, because the caller will do a
> 'return 1' followed by 'exit(1)' in the child process when this happens, but
> it's easy to imagine a change in the caller code that doesn't take into
account
> that the function can return in two different PIDs.
> 
> Another commonly implemented option is to make the parent wait for the child
by
> creating an anonymous pipe and have the child send a single byte through it to
> the parent to indicate success/failure. Just mentioning it here, no need to
> implement this here right now.
> 

Good point.

https://chromiumcodereview.appspot.com/11360248/diff/17002/tools/android/forw...
tools/android/forwarder2/daemon.cc:283: signal(SIGCHLD, SigChildHandler);
On 2012/11/19 15:27:43, digit1 wrote:
> since you're writing a general Daemon class, you might want to store the hold
> SIGCHLD handler, and restore it on scope exit (with an appropriate template).
> Use sigprocmask() instead of sigmask() to ensure you save the SA_SIGINFO flag
> associated with the previous signal handler.

Done. Note that I didn't use an extra RAII object this time for restoring the
sigaction to avoid making this file more complicated.

[digit1, 2012-11-23 10:08:40]

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

https://chromiumcodereview.appspot.com/11360248/diff/15012/tools/android/forw...
tools/android/forwarder2/daemon.cc:295: memcpy(&new_action.sa_mask,
&blocked_signals_set, sizeof(new_action.sa_mask));
this code can be simplied. You don't need to set anything in the sa_mask of the
handler (which is different from the global signal mask). I.e. try something
like that instead:

  // Set the new signal handler, and save the old one.
  struct sigaction old_action, new_action;
  memset(&new_action, 0, sizeof(new_action));
  new_action.sa_handler = SigChildHandler;
  new_action.sa_flags = SA_NOCLDSTOP;
  sigprocmask(SIGCHLD, &new_action, &old_action);

  ...

  // restore the old signal handler
  sigprocmask(SIGCHLD, &old_action, NULL);

[Philippe, 2012-11-23 10:31:14]

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

https://chromiumcodereview.appspot.com/11360248/diff/15012/tools/android/forw...
tools/android/forwarder2/daemon.cc:295: memcpy(&new_action.sa_mask,
&blocked_signals_set, sizeof(new_action.sa_mask));
On 2012/11/23 10:08:41, digit1 wrote:
> this code can be simplied. You don't need to set anything in the sa_mask of
the
> handler (which is different from the global signal mask). I.e. try something
> like that instead:
> 
>   // Set the new signal handler, and save the old one.
>   struct sigaction old_action, new_action;
>   memset(&new_action, 0, sizeof(new_action));
>   new_action.sa_handler = SigChildHandler;
>   new_action.sa_flags = SA_NOCLDSTOP;
>   sigprocmask(SIGCHLD, &new_action, &old_action);
> 
>   ...
> 
>   // restore the old signal handler
>   sigprocmask(SIGCHLD, &old_action, NULL);

Done.

[digit1, 2012-11-23 10:32:47]

lgtm

[Philippe, 2012-11-23 10:38:45]

On 2012/11/23 10:32:47, digit1 wrote:
> lgtm

Thanks David. Marcus can you please take a final look? I need your approval.

[bulach, 2012-11-23 10:42:02]

lgtm, thanks!

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

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

[commit-bot: I haz the power, 2012-11-23 12:39:36]

Change committed as 169334