Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
259763002:
Add tests to make sure UnixDomainSocket and namespaces play nicely (Closed)
|
Created:
6 years, 8 months ago by mdempsky Modified:
6 years, 7 months ago Reviewers:
jln (very slow on Chromium) CC:
chromium-reviews, agl, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd tests to make sure UnixDomainSocket and namespaces play nicely
BUG=357670
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266483
Patch Set 1 #Patch Set 2 : Cleanup includes for IWYU #
Total comments: 16
Patch Set 3 : Respond to jln feedback #Patch Set 4 : Disable tests on Android #
Messages
Total messages: 18 (0 generated)
lookin' good in the neighborhood.
lgtm with some small nits. I tested locally on a kernel that supports NEW_USER. This will not receive any real testing on our WF until crbug.com/349236 is solved. I wonder if you could have made this a little simpler by using ScopedProcesses, but it's probably not worth changing it to figure out. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... File sandbox/linux/services/unix_domain_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:48: CHECK_EQ(pid, waitpid(pid, &status, 0)); HANDLE_EINTR https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:78: CHECK_EQ(0, read(read_pipe.get(), &ch, 1)); HANDLE_EINTR() https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:81: void RecvHello(int fd, Could you document RecvHello() / SendHello() (as if they were small APIs)? In particular make explicit that SendHello will block until RecvHello has returned, by way of using a pipe. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:123: WaitForExit(pid); Thanks for cleaning-up :) (In SANDBOX_TEST it's not strictly necessary since it's a subprocess, so childs will be reparented to init and not pollute other tests). https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:183: } Maybe add a "// Fallthrough." comment? https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:197: Nit: remove space? https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:207: Nit: remove space? https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:210: CHECK_EQ(pid, GetParentProcessId(sender_pid)); Maybe add a comment: "Check that sender_pid is our grandchild by checking that its parent is pid".
On 2014/04/25 23:23:31, jln wrote: > I tested locally on a kernel that supports NEW_USER. Thanks! > This will not receive any real testing on our WF until crbug.com/349236 is > solved. Acknowledged. > I wonder if you could have made this a little simpler by using ScopedProcesses, > but it's probably not worth changing it to figure out. I considered that, but came to the same conclusion of it probably not being worthwhile. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... File sandbox/linux/services/unix_domain_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:48: CHECK_EQ(pid, waitpid(pid, &status, 0)); On 2014/04/25 23:23:31, jln wrote: > HANDLE_EINTR Done. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:78: CHECK_EQ(0, read(read_pipe.get(), &ch, 1)); On 2014/04/25 23:23:31, jln wrote: > HANDLE_EINTR() Done. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:81: void RecvHello(int fd, On 2014/04/25 23:23:31, jln wrote: > Could you document RecvHello() / SendHello() (as if they were small APIs)? > > In particular make explicit that SendHello will block until RecvHello has > returned, by way of using a pipe. Done. PTAL and let me know if the documentation makes sense. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:183: } On 2014/04/25 23:23:31, jln wrote: > Maybe add a "// Fallthrough." comment? Done. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:197: On 2014/04/25 23:23:31, jln wrote: > Nit: remove space? Done. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:207: On 2014/04/25 23:23:31, jln wrote: > Nit: remove space? Done. https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:210: CHECK_EQ(pid, GetParentProcessId(sender_pid)); On 2014/04/25 23:23:31, jln wrote: > Maybe add a comment: "Check that sender_pid is our grandchild by checking that > its parent is pid". Done.
https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... File sandbox/linux/services/unix_domain_socket_unittest.cc (right): https://chromiumcodereview.appspot.com/259763002/diff/20001/sandbox/linux/ser... sandbox/linux/services/unix_domain_socket_unittest.cc:81: void RecvHello(int fd, > Done. PTAL and let me know if the documentation makes sense. Perfect, lgtm
The CQ bit was checked by mdempsky@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mdempsky@chromium.org/259763002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
On 2014/04/26 02:21:11, I haz the power (commit-bot) wrote: > Retried try job too often on android_dbg for step(s) slave_steps > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Hrm... Julien, how do you suggest dealing with this on Android? Do I need some extra #include's, define CLONE_NEW{USER,PID} myself, or just skip these tests on Android?
On 2014/04/26 03:12:26, mdempsky wrote: > On 2014/04/26 02:21:11, I haz the power (commit-bot) wrote: > > Retried try job too often on android_dbg for step(s) slave_steps > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > > Hrm... Julien, how do you suggest dealing with this on Android? Do I need some > extra #include's, define CLONE_NEW{USER,PID} myself, or just skip these tests on > Android? It's probably easier to skip on Android, and I don't think we'd get anything useful from testing it there. You could just exclude this file for Android in the GYP file (see how I did it for credentials.cc)
The CQ bit was checked by mdempsky@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mdempsky@chromium.org/259763002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by jln@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mdempsky@chromium.org/259763002/60001
Message was sent while issue was closed.
Change committed as 266483 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
