Add the MultiprocessExec test and its test

Add the MultiprocessExec test and its test.

TEST=util_test MultiprocessExec.*
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/52064fdd1b9ed25f7432f14abdce9dac10e4e507

[Mark Mentovai, 2014-09-02 21:09:04]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Mark Mentovai, 2014-09-02 21:09:04]

I don’t think that having a separate test executable for each MultiprocessExec
test is a desirable situation or the final state. Stopping here keeps this
change bite-sized, though.

I think that what I’ll ultimately wind up with is a way to register each child’s
main() with some kind of static initializer thing, and to look for a special
value in argv that knows how to dispatch to those functions in the test
executable’s real main(). That should keep all of the child code having access
to the same libraries, etc.

[Robert Sesek, 2014-09-02 21:45:15]

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec.cc
File util/test/multiprocess_exec.cc (right):

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.cc:96: // messages directed to stdout go to the
right place. Since stdin, stdout, and
Don't test messages go to stderr?

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.cc:98: // needed in the child, use the next
available file descriptor, 3, for this
Are you really guaranteed that no other fd occupies 3? Apple doesn't add some
crazy guarded FD there?

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.cc:137: FAIL() << ErrnoMessage("execv") << ": " <<
argv_[0];
Seems like a case for ScopedNoReturn.

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec.h
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.h:37: class MultiprocessExec : public Multiprocess {
Will there be no need for a MachMultiprocesseExec?

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.h:41: //! \brief Sets the command to `exec()` in the
child.
Do I have to call this? What if I don't?

[Mark Mentovai, 2014-09-02 22:47:47]

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec.cc
File util/test/multiprocess_exec.cc (right):

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.cc:96: // messages directed to stdout go to the
right place. Since stdin, stdout, and
rsesek wrote:
> Don't test messages go to stderr?

No. Logging goes to stderr. gtest goes to stdout.

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.cc:137: FAIL() << ErrnoMessage("execv") << ": " <<
argv_[0];
rsesek wrote:
> Seems like a case for ScopedNoReturn.

You didn’t mean instead of the FAIL(), did you? You just meant between the
execv() and the FAIL() to guard against early returns in the flush/close stuff?

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec.h
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.h:37: class MultiprocessExec : public Multiprocess {
rsesek wrote:
> Will there be no need for a MachMultiprocesseExec?

There will absolutely be. It will come separately. It’s not in this one because
the parent and child bits in MachMultiprocess need to be refactored out a bit so
that this can all be done without multiple inheritance, like so:

void MachCommonParent();
void MachCommonChild();

class MachMultiprocess : public Multiprocess {
  virtual void MultiprocessParent() { MachCommonParent();
MachMultiprocessParent(); }
  virtual void MultiprocessChild() { MachCommonChild(); MachMultiprocessChild();
}
  virtual void MachMultiprocessParent() = 0;
  virtual void MachMultiprocessChild() = 0;
};

void MachMultiprocessExecChildMain() {
  MachCommonChild();
  // dispatch proper test code
}

class MachMultiprocessExec : public MultiprocessExec {
  void SetChildCommand(…) { /* add a special argument to get the
MachMultiprocessExecChildMain() */ }
  virtual void MultiprocessParent() { MachCommonParent();
MachMultiprocessParent(); }
  virtual void MachMultiprocessParent() = 0;
};

[Robert Sesek, 2014-09-03 20:56:24]

LGTM

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec.h
File util/test/multiprocess_exec.h (right):

https://codereview.chromium.org/531203002/diff/1/util/test/multiprocess_exec....
util/test/multiprocess_exec.h:37: class MultiprocessExec : public Multiprocess {
On 2014/09/02 22:47:47, Mark Mentovai wrote:
> rsesek wrote:
> > Will there be no need for a MachMultiprocesseExec?
> 
> There will absolutely be. It will come separately. It’s not in this one
because
> the parent and child bits in MachMultiprocess need to be refactored out a bit
so
> that this can all be done without multiple inheritance, like so:

Right, I figured this required some refactoring to make these elements more
composable.

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
File util/test/posix/close_multiple.cc (right):

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
util/test/posix/close_multiple.cc:135: return;
nit: indention

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
File util/test/posix/close_multiple.h (right):

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
util/test/posix/close_multiple.h:40: void CloseMultipleNowOrOnExec(int fd, int
preserve_fd);
I can imagine wanting this to be a std::set in the future…

[Mark Mentovai, 2014-09-03 22:24:05]

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
File util/test/posix/close_multiple.h (right):

https://codereview.chromium.org/531203002/diff/40001/util/test/posix/close_mu...
util/test/posix/close_multiple.h:40: void CloseMultipleNowOrOnExec(int fd, int
preserve_fd);
rsesek wrote:
> I can imagine wanting this to be a std::set in the future…

It’d have to be a set that was created before the fork, then. Gotta avoid
standard library containers after fork(), because they might enter the
allocator…

[Mark Mentovai, 2014-09-03 22:24:34]

Committed patchset #4 (id:60001) manually as 52064fd (presubmit successful).