Add GetAppOutput function, a better replacement for popen.

Add GetAppOutput function, a better replacement for popen.

It will replace popen call in chrome_process_util_linux.

I don't see much benefit in having a Windows implementation
(not many useful programs you can launch and get output),
so POSIX-only.

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

[agl, 2009-04-16 18:21:56]

http://codereview.chromium.org/67226/diff/1/3
File base/process_util_posix.cc (right):

http://codereview.chromium.org/67226/diff/1/3#newcode336
Line 336: close(pipe_fd[0]);
You need to be a little careful here. What if pipe_fd[0] equals STDOUT_FILENO?
dup2 is a noop in that case, but here you'll close it.

I think these two should be wrapped like:

if (pipe_fd[0] != 1 && pipe_fd[0] != 2)
  close(pipe_fd[0])

http://codereview.chromium.org/67226/diff/1/3#newcode340
Line 340: char* argv_copy[argv.size() + 1];
this is a GCC extension, but since this is POSIX only I guess it's ok.

http://codereview.chromium.org/67226/diff/1/3#newcode343
Line 343: strcpy(argv_copy[i], argv[i].c_str());
Would be better to use memcpy here. strcpy is going to have to walk the string
in order to find the length, but the string already knows that.

memcpy(argv_copy[i], argv[i].c_str(), argv[i].size() + 1);

http://codereview.chromium.org/67226/diff/1/3#newcode346
Line 346: execvp(argv_copy[0], argv_copy);
I'm not sure why you're copying the strings at all here. Can't you just point
into the std::string objects from argv_copy[]? We're in a child process, the
parent can mutate the array at this point.

http://codereview.chromium.org/67226/diff/1/3#newcode368
Line 368: bytes_read = read(pipe_fd[0], &buffer, sizeof(buffer));
no & on buffer here, right?

[Paweł Hajdan Jr., 2009-04-16 19:18:03]

On 2009/04/16 18:21:56, agl wrote:
> http://codereview.chromium.org/67226/diff/1/3
> File base/process_util_posix.cc (right):
> 
> http://codereview.chromium.org/67226/diff/1/3#newcode336
> Line 336: close(pipe_fd[0]);
> You need to be a little careful here. What if pipe_fd[0] equals STDOUT_FILENO?
> dup2 is a noop in that case, but here you'll close it.

Oops. Fixed.

> http://codereview.chromium.org/67226/diff/1/3#newcode343
> Line 343: strcpy(argv_copy[i], argv[i].c_str());
> Would be better to use memcpy here. strcpy is going to have to walk the string
> in order to find the length, but the string already knows that.
> 
> memcpy(argv_copy[i], argv[i].c_str(), argv[i].size() + 1);
> 
> http://codereview.chromium.org/67226/diff/1/3#newcode346
> Line 346: execvp(argv_copy[0], argv_copy);
> I'm not sure why you're copying the strings at all here. Can't you just point
> into the std::string objects from argv_copy[]? We're in a child process, the
> parent can mutate the array at this point.

Indeed. Actually, I copied the code from LaunchApp. The copying code could be
there to workaround the const problem (and it has nothing to do with the vector
being const, I checked that). Alternative is a const_cast which I used, and I
think it's safe because execvp won't mutate its arguments.

I think that I can also apply same fixes to the original code in LaunchApp (in a
follow-up).

> http://codereview.chromium.org/67226/diff/1/3#newcode368
> Line 368: bytes_read = read(pipe_fd[0], &buffer, sizeof(buffer));
> no & on buffer here, right?

Fixed.

[agl, 2009-04-16 19:20:21]

LGTM

[Scott Hess - ex-Googler, 2009-04-16 21:20:25]

Drive-by comment...

http://codereview.chromium.org/67226/diff/1002/10
File base/process_util_posix.cc (right):

http://codereview.chromium.org/67226/diff/1002/10#newcode376
Line 376: output->assign(buf_output);
Since process output is sort of open-ended, would swap() make more sense than
assign()?