libchromeos: add process control library, syslog logging capability, and run unit tests

libchromeos: add process control library, syslog logging capability, and run unit tests

Change-Id: I9d9a00eeb649666efc1f2fdac1914e4bf6f1149c

BUG=n0ne
TEST=unit tests

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=f7cb327

[kmixter1, 2011-02-25 03:19:42]

petkov: for the process.* stuff
rginda: for the syslog_logging.* stuff

[petkov, 2011-02-25 19:20:16]

Some comments on the process* stuff. Haven't looked at the syslog...

http://codereview.chromium.org/6509006/diff/6001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6509006/diff/6001/SConstruct#newcode12
SConstruct:12: 'chromeos/process.cc',
sort?

http://codereview.chromium.org/6509006/diff/6001/SConstruct#newcode56
SConstruct:56: 'chromeos/process_test.cc',
s/tab/spaces/ -- switch emacs to python-mode :-)

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc
File chromeos/process.cc (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode43
chromeos/process.cc:43: // Copy off a writeable version of arguments.  Fork
wants to be able
fork or exec?

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode59
chromeos/process.cc:59: *buffer_pointer = '\0';
no need -- strcpy copies '\0'.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode84
chromeos/process.cc:84: dup2(output_handle, 2);
don't you need to close output_handle after this call?

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode152
chromeos/process.cc:152: } while (time(NULL) - start_signal <= timeout);
in the unlikely event that the system time changes in the middle of this call
this might block. you might want to consider either simply counting the number
of usleep's or using a monotonic clock (from libbase, for example).

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode172
chromeos/process.cc:172: pid_t pid = strtoul(contents.c_str(), &end, 10);
base::StringToInt64?

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.h
File chromeos/process.h (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.h#newcode19
chromeos/process.h:19: class Process {
You need brief docstrings for the individual methods of this class.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc
File chromeos/process_test.cc (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:25: class ProcessTest : public ::testing::Test {
maybe add a test that checks that you're not leaking file descriptors? :-)

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:36: void TearDown() {
delete test_path_?

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:123: EXPECT_FALSE(process_.ProcessExists(0));
shouldn't you test Process::ProcessExists directly given that it's static?

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:144: if (Process::ProcessExists(pid))
why would this return false?

[kmixter1, 2011-02-26 00:54:31]

PTAL

http://codereview.chromium.org/6509006/diff/6001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6509006/diff/6001/SConstruct#newcode12
SConstruct:12: 'chromeos/process.cc',
On 2011/02/25 19:20:16, petkov wrote:
> sort?

Done.

http://codereview.chromium.org/6509006/diff/6001/SConstruct#newcode56
SConstruct:56: 'chromeos/process_test.cc',
On 2011/02/25 19:20:16, petkov wrote:
> s/tab/spaces/ -- switch emacs to python-mode :-)

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc
File chromeos/process.cc (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode43
chromeos/process.cc:43: // Copy off a writeable version of arguments.  Fork
wants to be able
On 2011/02/25 19:20:16, petkov wrote:
> fork or exec?

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode59
chromeos/process.cc:59: *buffer_pointer = '\0';
On 2011/02/25 19:20:16, petkov wrote:
> no need -- strcpy copies '\0'.

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode84
chromeos/process.cc:84: dup2(output_handle, 2);
On 2011/02/25 19:20:16, petkov wrote:
> don't you need to close output_handle after this call?

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode152
chromeos/process.cc:152: } while (time(NULL) - start_signal <= timeout);
On 2011/02/25 19:20:16, petkov wrote:
> in the unlikely event that the system time changes in the middle of this call
> this might block. you might want to consider either simply counting the number
> of usleep's or using a monotonic clock (from libbase, for example).

Good catch - I wouldn't say it's that unlikely.  Thanks.  Can't just count
usleeps because of EINTR.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.cc#newcode172
chromeos/process.cc:172: pid_t pid = strtoul(contents.c_str(), &end, 10);
On 2011/02/25 19:20:16, petkov wrote:
> base::StringToInt64?

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.h
File chromeos/process.h (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process.h#newcode19
chromeos/process.h:19: class Process {
On 2011/02/25 19:20:16, petkov wrote:
> You need brief docstrings for the individual methods of this class.

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc
File chromeos/process_test.cc (right):

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:25: class ProcessTest : public ::testing::Test {
On 2011/02/25 19:20:16, petkov wrote:
> maybe add a test that checks that you're not leaking file descriptors? :-)
Fixed the code.  Not really sure of an easy/non-fragile way to do that since the
leak was happening from the context of the child process and even if you used
procfs to count open filedescs you would need to know that the child doesn't
open anything itself.  If you can think of an easy way to check this, let me
know.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:36: void TearDown() {
On 2011/02/25 19:20:16, petkov wrote:
> delete test_path_?
> 

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:123: EXPECT_FALSE(process_.ProcessExists(0));
On 2011/02/25 19:20:16, petkov wrote:
> shouldn't you test Process::ProcessExists directly given that it's static?

Done.

http://codereview.chromium.org/6509006/diff/6001/chromeos/process_test.cc#new...
chromeos/process_test.cc:144: if (Process::ProcessExists(pid))
On 2011/02/25 19:20:16, petkov wrote:
> why would this return false?
ugh. yeah, I was thinking about waiting until after exec, but of course this
loop wouldn't do that.  So removing.

[petkov, 2011-02-28 08:00:14]

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc
File chromeos/process.cc (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc#newcode62
chromeos/process.cc:62: buffer_pointer += arguments_[i].size();
+ 1 here and remove ++buffer_pointer below?

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc#newcode121
chromeos/process.cc:121: pid_ = 0;
So is this ReleaseIfSamePID?

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h
File chromeos/process.h (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode31
chromeos/process.h:31: inline void AddStringOption(const std::string& option,
const std::string& value) {
80 chars

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode50
chromeos/process.h:50: // exited normally, or otherwise returns -1.
is it an issue that you can't distinguish between normal exit with -1, and the
"otherwise" case? you could change the interface to return true/false and set an
(optional?) argument with the exit code.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode57
chromeos/process.h:57: virtual bool HandleSigChild(pid_t pid, int status) = 0;
I don't quite get what this method does -- and I can't match the implementation
with the docstring.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode59
chromeos/process.h:59: // Start and wait for this process to finish.  Returns
same value as Wait().
Same question on the return value as Wait.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode67
chromeos/process.h:67: // Kills this process with |signal|.  If process is not a
child, returns immediately
80 chars the whole docstring

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode73
chromeos/process.h:73: // Resets this Process object to refer to the process
with |pid|.  If |pid| is zero,
80 chars

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode77
chromeos/process.h:77: // Same as Reset but reads the pid from |pid_file|. 
Returns false only when the
80 chars

[rginda, 2011-02-28 18:15:20]

http://codereview.chromium.org/6509006/diff/9002/chromeos/syslog_logging.h
File chromeos/syslog_logging.h (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/syslog_logging.h#ne...
chromeos/syslog_logging.h:14: void InitLog(bool log_to_syslog, bool
log_to_stderr);
How about a bitfield instead of the two booleans, in case we need to express
more options in the future?

[kmixter1, 2011-03-02 01:38:07]

PTAL. addressed all comments.  Updated other CLs (6517001 and 6594001) with
rginda's suggested api change.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc
File chromeos/process.cc (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc#newcode62
chromeos/process.cc:62: buffer_pointer += arguments_[i].size();
On 2011/02/28 08:00:14, petkov wrote:
> + 1 here and remove ++buffer_pointer below?

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.cc#newcode121
chromeos/process.cc:121: pid_ = 0;
On 2011/02/28 08:00:14, petkov wrote:
> So is this ReleaseIfSamePID?

Conceptually it is.  It's also what you do to clean up a child process during
sigchild or a wait-based event loop.  It's also a partially formed idea.  I've
removed it.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h
File chromeos/process.h (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode31
chromeos/process.h:31: inline void AddStringOption(const std::string& option,
const std::string& value) {
On 2011/02/28 08:00:14, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode50
chromeos/process.h:50: // exited normally, or otherwise returns -1.
On 2011/02/28 08:00:14, petkov wrote:
> is it an issue that you can't distinguish between normal exit with -1, and the
> "otherwise" case? you could change the interface to return true/false and set
an
> (optional?) argument with the exit code.

Exit statuses are 8 bits and unsigned, so -1 is not valid.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode57
chromeos/process.h:57: virtual bool HandleSigChild(pid_t pid, int status) = 0;
On 2011/02/28 08:00:14, petkov wrote:
> I don't quite get what this method does -- and I can't match the
implementation
> with the docstring.

Removed.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode59
chromeos/process.h:59: // Start and wait for this process to finish.  Returns
same value as Wait().
On 2011/02/28 08:00:14, petkov wrote:
> Same question on the return value as Wait.

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode67
chromeos/process.h:67: // Kills this process with |signal|.  If process is not a
child, returns immediately
On 2011/02/28 08:00:14, petkov wrote:
> 80 chars the whole docstring

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode73
chromeos/process.h:73: // Resets this Process object to refer to the process
with |pid|.  If |pid| is zero,
On 2011/02/28 08:00:14, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/process.h#newcode77
chromeos/process.h:77: // Same as Reset but reads the pid from |pid_file|. 
Returns false only when the
On 2011/02/28 08:00:14, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/6509006/diff/9002/chromeos/syslog_logging.h
File chromeos/syslog_logging.h (right):

http://codereview.chromium.org/6509006/diff/9002/chromeos/syslog_logging.h#ne...
chromeos/syslog_logging.h:14: void InitLog(bool log_to_syslog, bool
log_to_stderr);
On 2011/02/28 18:15:20, rginda wrote:
> How about a bitfield instead of the two booleans, in case we need to express
> more options in the future?

Done.

[petkov, 2011-03-02 05:49:40]

process* LGTM

[kmixter1, 2011-03-02 12:09:47]

petkov: PTAL at 4->5 - took me sadly a few hours to figure out why closing this
file descriptor was breaking crash reporter.

[petkov, 2011-03-02 17:46:09]

heh... LGTM

[rginda, 2011-03-02 19:23:43]

On 2011/03/02 17:46:09, petkov wrote:
> heh... LGTM

logging LGTM