Code review comment for lp:~niclasl/pantheon-terminal/fix-1327526

Revision history for this message
Raphael Isemann (teemperor) wrote :

Fixes said bug and the rest still works. Code-style is correct too.

The thing that bugs me is the loop at the end:

1.
Posix.usleep (100);
is also available in the GLib via
GLib.Thread.usleep (100);

2. I those lines should to be the other way round, as we first should send a signal and then wait.

Posix.usleep (100);
Posix.kill (this.child_pid, Posix.SIGTERM);

3. This
Posix.kill (this.child_pid, 0) != -1
can be
Posix.kill (this.child_pid, 0) == 0
on both occurrences.

Also a comment like
// Check if PID is valid
should be added as it's not that obvious what kill with sig 0 does.

4. I think we shouldn't SIGKILL here, but not sure what we should do instead. Let's add shnatsel and see what his opinion is on this topic.

Posix.kill (this.child_pid, Posix.SIGKILL);

review: Needs Fixing (code and functionality)

« Back to merge proposal