Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
744803003:
Non-SFI mode: Add syscalls which are needed for nacl_helper_nonsfi's sandbox. (Closed)
DescriptionNon-SFI mode: Add syscalls which are needed for nacl_helper_nonsfi's sandbox.
openat and fstatat are needed for the sandbox implementation
of nacl_helper_nonsfi. The headers are already added separately.
This CL adds their implementation and tests.
Also, it turned out some oflag values (for open/fcntl etc.) are different between NaCl ABI and Linux platform. This CL introduces the ABI conversion between them.
BUG=https://code.google.com/p/chromium/issues/detail?id=358465
TEST="./scons run_file_descriptor_test bitcode=1 nonsfi_nacl=1" locally. Ran trybots.
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=14271
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 22
Patch Set 6 : Rebase #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 27 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hidehiko@chromium.org changed reviewers: + hamaji@chromium.org, mseaborn@chromium.org
Thank you for your review in advance, - hidehiko
On 2014/11/21 18:00:24, hidehiko wrote: > Thank you for your review in advance, > - hidehiko Friendly ping?
https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... File src/nonsfi/linux/abi_conversion.h (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... src/nonsfi/linux/abi_conversion.h:16: #include <stdio.h> Left over from debugging? https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... src/nonsfi/linux/abi_conversion.h:76: #define LINUX_O_WRONLY 01 Which of these flags are different from NaCl/newlib so that conversion is required? https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { What in sandbox/linux/ requires using F_GETFL? Can you say in the commit message? This flags conversion adds a lot of complexity so I'd like to check what it's needed for. https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:470: int pipe2(int pipefd[2], int flags) { Same here -- what uses pipe2()? Is it something we really need for nacl_helper_nonsfi?
Patchset #2 (id:60001) has been deleted
PTAL. https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... File src/nonsfi/linux/abi_conversion.h (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... src/nonsfi/linux/abi_conversion.h:16: #include <stdio.h> On 2014/11/27 16:12:16, Mark Seaborn wrote: > Left over from debugging? Good catch. Removed. https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... src/nonsfi/linux/abi_conversion.h:76: #define LINUX_O_WRONLY 01 On 2014/11/27 16:12:17, Mark Seaborn wrote: > Which of these flags are different from NaCl/newlib so that conversion is > required? O_SYNC and O_DIRECTORY. O_SYNC is 010000 in NaCl ABI, but 04010000 in linux. O_DIRECTORY depends on arch. On x86, it is 0200000, but on arm it is 040000. https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { On 2014/11/27 16:12:17, Mark Seaborn wrote: > What in sandbox/linux/ requires using F_GETFL? Can you say in the commit > message? This flags conversion adds a lot of complexity so I'd like to check > what it's needed for. Used in some places at least in ipc_channel_posix.cc. E.g.; https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:470: int pipe2(int pipefd[2], int flags) { On 2014/11/27 16:12:17, Mark Seaborn wrote: > Same here -- what uses pipe2()? Is it something we really need for > nacl_helper_nonsfi? Good catch. It was removed very recently, after this CL is set to review. Removed.
On 2014/11/28 16:58:15, hidehiko wrote: > PTAL. > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... > File src/nonsfi/linux/abi_conversion.h (right): > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... > src/nonsfi/linux/abi_conversion.h:16: #include <stdio.h> > On 2014/11/27 16:12:16, Mark Seaborn wrote: > > Left over from debugging? > > Good catch. Removed. > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/abi_con... > src/nonsfi/linux/abi_conversion.h:76: #define LINUX_O_WRONLY 01 > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > Which of these flags are different from NaCl/newlib so that conversion is > > required? > > O_SYNC and O_DIRECTORY. > > O_SYNC is 010000 in NaCl ABI, but 04010000 in linux. > O_DIRECTORY depends on arch. On x86, it is 0200000, but on arm it is 040000. > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > File src/nonsfi/linux/linux_sys_private.c (right): > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > What in sandbox/linux/ requires using F_GETFL? Can you say in the commit > > message? This flags conversion adds a lot of complexity so I'd like to check > > what it's needed for. > > Used in some places at least in ipc_channel_posix.cc. E.g.; > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > src/nonsfi/linux/linux_sys_private.c:470: int pipe2(int pipefd[2], int flags) { > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > Same here -- what uses pipe2()? Is it something we really need for > > nacl_helper_nonsfi? > > Good catch. It was removed very recently, after this CL is set to review. > Removed. friendly ping?
https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:214: cmode = 0; Any reason you don't call linux_syscall3 here? I'm fine with the current code though https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:216: Shouldn't we convert NACL_ABI_AT_FDCWD to AT_FDCWD?
https://codereview.chromium.org/744803003/diff/80001/tests/nonsfi/file_descri... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/80001/tests/nonsfi/file_descri... tests/nonsfi/file_descriptor_test.cc:77: puts("test for pipe2()"); s/pipe2/fstatat/ ? And we don't have a test for pipe2?
PTAL. https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:214: cmode = 0; On 2014/12/03 08:51:19, hamaji wrote: > Any reason you don't call linux_syscall3 here? I'm fine with the current code > though what do you mean? I just write a code consistent with open() above. https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:216: On 2014/12/03 08:51:19, hamaji wrote: > Shouldn't we convert NACL_ABI_AT_FDCWD to AT_FDCWD? As its value is same among NaCl, x86 and arm ABIs, I just kept it as is. https://codereview.chromium.org/744803003/diff/80001/tests/nonsfi/file_descri... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/80001/tests/nonsfi/file_descri... tests/nonsfi/file_descriptor_test.cc:77: puts("test for pipe2()"); On 2014/12/03 09:01:13, hamaji wrote: > s/pipe2/fstatat/ ? > > And we don't have a test for pipe2? Done. Note: pipe2() was gone.
Patchset #4 (id:120001) has been deleted
PTAL. https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:216: On 2014/12/03 17:34:49, hidehiko wrote: > On 2014/12/03 08:51:19, hamaji wrote: > > Shouldn't we convert NACL_ABI_AT_FDCWD to AT_FDCWD? > > As its value is same among NaCl, x86 and arm ABIs, I just kept it as is. Ah, no, my bad. AT_FDCWD is different value, though on current implementation, e.g., fdstatat seems to interpret some negative value as AT_FDCWD. Anyway, fixed. I'll also make a change in header in a separate CL.
lgtm https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/80001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:214: cmode = 0; On 2014/12/03 17:34:49, hidehiko wrote: > On 2014/12/03 08:51:19, hamaji wrote: > > Any reason you don't call linux_syscall3 here? I'm fine with the current code > > though > > what do you mean? I just write a code consistent with open() above. I didn't realize this is consistent with open(). I think this code is OK as is. https://codereview.chromium.org/744803003/diff/140001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/140001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:69: fd = openat(AT_FDCWD, test_path, O_RDONLY); Doesn't this test pass even if AT_FDCWD == dirfd? I think chdir(test_directory) + openat(AT_FDCWD, test_file_name) would be a slightly better test.
Thank you for review, Shinichiro. Mark, friendly ping? https://codereview.chromium.org/744803003/diff/140001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/140001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:69: fd = openat(AT_FDCWD, test_path, O_RDONLY); On 2014/12/08 07:38:39, hamaji wrote: > Doesn't this test pass even if AT_FDCWD == dirfd? I think chdir(test_directory) > + openat(AT_FDCWD, test_file_name) would be a slightly better test. Done.
https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { On 2014/11/28 16:58:15, hidehiko wrote: > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > What in sandbox/linux/ requires using F_GETFL? Can you say in the commit > > message? This flags conversion adds a lot of complexity so I'd like to check > > what it's needed for. > > Used in some places at least in ipc_channel_posix.cc. E.g.; > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... So this is needed for ipc/ rather than for the seccomp sandbox? But we are already using ipc/ this way for the nacl_helper_nonsfi-based tests. How come those tests work? Does enabling the sandbox cause this part of ipc/ to fail? Can you make this clearer in the commit message, please? https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... File src/nonsfi/linux/abi_conversion.h (right): https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... src/nonsfi/linux/abi_conversion.h:95: #define NACL_KNOWN_O_FLAGS (O_ACCMODE | O_CREAT | O_TRUNC | O_APPEND |\ Nit: add space before '\' https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... src/nonsfi/linux/abi_conversion.h:127: (((linux_oflags & LINUX_ ## name) == LINUX_ ## name) ? name : 0) Nit: indent by 2 more spaces so that this doesn't line up with the following line https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:17: static ssize_t read_from_fd(int fd, char* buf, size_t buf_len) { Use " *" spacing https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:36: snprintf(test_path, PATH_MAX, "%s/%s", test_directory, test_file_name); How about using std::string instead? https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:43: fclose(fp); Nit: check for error https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:59: // Open for the non-directory fd. Grammar nit: remove "for" https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:102: snprintf(test_path, PATH_MAX, "%s/%s", test_directory, test_file_name); Use std::string? https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:106: FILE *fp = fopen(test_path, "w"); Check for error https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:189: const char* test_directory = argv[1]; Use " *" spacing https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:27: 'file_descriptor_test.out', nexe, Style nit: indent by 4 spaces rather than 2 https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:28: args=[env.MakeTempDir(prefix='tmp_file_desciprtor')]) There's a typo here
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Thank you for review. PTAL. https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... File src/nonsfi/linux/linux_sys_private.c (right): https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { On 2014/12/08 22:09:02, Mark Seaborn wrote: > On 2014/11/28 16:58:15, hidehiko wrote: > > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > > What in sandbox/linux/ requires using F_GETFL? Can you say in the commit > > > message? This flags conversion adds a lot of complexity so I'd like to > check > > > what it's needed for. > > > > Used in some places at least in ipc_channel_posix.cc. E.g.; > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > So this is needed for ipc/ rather than for the seccomp sandbox? > > But we are already using ipc/ this way for the nacl_helper_nonsfi-based tests. > How come those tests work? Does enabling the sandbox cause this part of ipc/ to > fail? > > Can you make this clearer in the commit message, please? Why it is working now is because the flags used by IPC module have same value with linux platform. I.e., IPC module does not use the flags whose value is different between NaCl ABI and Linux. Updated the commit message. https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... File src/nonsfi/linux/abi_conversion.h (right): https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... src/nonsfi/linux/abi_conversion.h:95: #define NACL_KNOWN_O_FLAGS (O_ACCMODE | O_CREAT | O_TRUNC | O_APPEND |\ On 2014/12/08 22:09:02, Mark Seaborn wrote: > Nit: add space before '\' Done. https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... src/nonsfi/linux/abi_conversion.h:127: (((linux_oflags & LINUX_ ## name) == LINUX_ ## name) ? name : 0) On 2014/12/08 22:09:02, Mark Seaborn wrote: > Nit: indent by 2 more spaces so that this doesn't line up with the following > line Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:17: static ssize_t read_from_fd(int fd, char* buf, size_t buf_len) { On 2014/12/08 22:09:03, Mark Seaborn wrote: > Use " *" spacing Acknowledged. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:36: snprintf(test_path, PATH_MAX, "%s/%s", test_directory, test_file_name); On 2014/12/08 22:09:03, Mark Seaborn wrote: > How about using std::string instead? Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:43: fclose(fp); On 2014/12/08 22:09:03, Mark Seaborn wrote: > Nit: check for error Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:59: // Open for the non-directory fd. On 2014/12/08 22:09:02, Mark Seaborn wrote: > Grammar nit: remove "for" Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:102: snprintf(test_path, PATH_MAX, "%s/%s", test_directory, test_file_name); On 2014/12/08 22:09:03, Mark Seaborn wrote: > Use std::string? Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:106: FILE *fp = fopen(test_path, "w"); On 2014/12/08 22:09:03, Mark Seaborn wrote: > Check for error Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:189: const char* test_directory = argv[1]; On 2014/12/08 22:09:03, Mark Seaborn wrote: > Use " *" spacing Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons File tests/nonsfi/nacl.scons (right): https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:27: 'file_descriptor_test.out', nexe, On 2014/12/08 22:09:03, Mark Seaborn wrote: > Style nit: indent by 4 spaces rather than 2 Done. https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... tests/nonsfi/nacl.scons:28: args=[env.MakeTempDir(prefix='tmp_file_desciprtor')]) On 2014/12/08 22:09:03, Mark Seaborn wrote: > There's a typo here Oops, good catch. Fixed.
On 2014/12/09 08:38:49, hidehiko wrote: > Thank you for review. PTAL. > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > File src/nonsfi/linux/linux_sys_private.c (right): > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > On 2014/11/28 16:58:15, hidehiko wrote: > > > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > > > What in sandbox/linux/ requires using F_GETFL? Can you say in the commit > > > > message? This flags conversion adds a lot of complexity so I'd like to > > check > > > > what it's needed for. > > > > > > Used in some places at least in ipc_channel_posix.cc. E.g.; > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > > > So this is needed for ipc/ rather than for the seccomp sandbox? > > > > But we are already using ipc/ this way for the nacl_helper_nonsfi-based tests. > > > How come those tests work? Does enabling the sandbox cause this part of ipc/ > to > > fail? > > > > Can you make this clearer in the commit message, please? > > Why it is working now is because the flags used by IPC module have same value > with linux platform. I.e., IPC module does not use the flags whose value is > different between NaCl ABI and Linux. > > Updated the commit message. > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > File src/nonsfi/linux/abi_conversion.h (right): > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > src/nonsfi/linux/abi_conversion.h:95: #define NACL_KNOWN_O_FLAGS (O_ACCMODE | > O_CREAT | O_TRUNC | O_APPEND |\ > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > Nit: add space before '\' > > Done. > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > src/nonsfi/linux/abi_conversion.h:127: (((linux_oflags & LINUX_ ## name) == > LINUX_ ## name) ? name : 0) > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > Nit: indent by 2 more spaces so that this doesn't line up with the following > > line > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > File tests/nonsfi/file_descriptor_test.cc (right): > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:17: static ssize_t read_from_fd(int fd, > char* buf, size_t buf_len) { > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Use " *" spacing > > Acknowledged. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:36: snprintf(test_path, PATH_MAX, "%s/%s", > test_directory, test_file_name); > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > How about using std::string instead? > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:43: fclose(fp); > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Nit: check for error > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:59: // Open for the non-directory fd. > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > Grammar nit: remove "for" > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:102: snprintf(test_path, PATH_MAX, "%s/%s", > test_directory, test_file_name); > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Use std::string? > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:106: FILE *fp = fopen(test_path, "w"); > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Check for error > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > tests/nonsfi/file_descriptor_test.cc:189: const char* test_directory = argv[1]; > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Use " *" spacing > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons > File tests/nonsfi/nacl.scons (right): > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... > tests/nonsfi/nacl.scons:27: 'file_descriptor_test.out', nexe, > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > Style nit: indent by 4 spaces rather than 2 > > Done. > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... > tests/nonsfi/nacl.scons:28: > args=[env.MakeTempDir(prefix='tmp_file_desciprtor')]) > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > There's a typo here > > Oops, good catch. Fixed. friendly ping.
On 2014/12/10 15:53:00, hidehiko wrote: > On 2014/12/09 08:38:49, hidehiko wrote: > > Thank you for review. PTAL. > > > > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > > File src/nonsfi/linux/linux_sys_private.c (right): > > > > > https://codereview.chromium.org/744803003/diff/40001/src/nonsfi/linux/linux_s... > > src/nonsfi/linux/linux_sys_private.c:406: if (cmd == F_GETFL) { > > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > > On 2014/11/28 16:58:15, hidehiko wrote: > > > > On 2014/11/27 16:12:17, Mark Seaborn wrote: > > > > > What in sandbox/linux/ requires using F_GETFL? Can you say in the > commit > > > > > message? This flags conversion adds a lot of complexity so I'd like to > > > check > > > > > what it's needed for. > > > > > > > > Used in some places at least in ipc_channel_posix.cc. E.g.; > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_po... > > > > > > So this is needed for ipc/ rather than for the seccomp sandbox? > > > > > > But we are already using ipc/ this way for the nacl_helper_nonsfi-based > tests. > > > > > How come those tests work? Does enabling the sandbox cause this part of > ipc/ > > to > > > fail? > > > > > > Can you make this clearer in the commit message, please? > > > > Why it is working now is because the flags used by IPC module have same value > > with linux platform. I.e., IPC module does not use the flags whose value is > > different between NaCl ABI and Linux. > > > > Updated the commit message. > > > > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > > File src/nonsfi/linux/abi_conversion.h (right): > > > > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > > src/nonsfi/linux/abi_conversion.h:95: #define NACL_KNOWN_O_FLAGS (O_ACCMODE | > > O_CREAT | O_TRUNC | O_APPEND |\ > > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > > Nit: add space before '\' > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/src/nonsfi/linux/abi_co... > > src/nonsfi/linux/abi_conversion.h:127: (((linux_oflags & LINUX_ ## name) == > > LINUX_ ## name) ? name : 0) > > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > > Nit: indent by 2 more spaces so that this doesn't line up with the following > > > line > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > File tests/nonsfi/file_descriptor_test.cc (right): > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:17: static ssize_t read_from_fd(int fd, > > char* buf, size_t buf_len) { > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Use " *" spacing > > > > Acknowledged. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:36: snprintf(test_path, PATH_MAX, > "%s/%s", > > test_directory, test_file_name); > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > How about using std::string instead? > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:43: fclose(fp); > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Nit: check for error > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:59: // Open for the non-directory fd. > > On 2014/12/08 22:09:02, Mark Seaborn wrote: > > > Grammar nit: remove "for" > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:102: snprintf(test_path, PATH_MAX, > "%s/%s", > > test_directory, test_file_name); > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Use std::string? > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:106: FILE *fp = fopen(test_path, "w"); > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Check for error > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/file_descr... > > tests/nonsfi/file_descriptor_test.cc:189: const char* test_directory = > argv[1]; > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Use " *" spacing > > > > Done. > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons > > File tests/nonsfi/nacl.scons (right): > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... > > tests/nonsfi/nacl.scons:27: 'file_descriptor_test.out', nexe, > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > Style nit: indent by 4 spaces rather than 2 > > > > Done. > > > > > https://codereview.chromium.org/744803003/diff/160001/tests/nonsfi/nacl.scons... > > tests/nonsfi/nacl.scons:28: > > args=[env.MakeTempDir(prefix='tmp_file_desciprtor')]) > > On 2014/12/08 22:09:03, Mark Seaborn wrote: > > > There's a typo here > > > > Oops, good catch. Fixed. > > friendly ping. Happy new year. Ping?
In commit message: "different between on NaCl ABI and on Linux platform" -- remove "between" or "on". LGTM. https://codereview.chromium.org/744803003/diff/240001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/240001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:26: ssize_t rc = read(fd, buf, kBufLen); "rc" -> "bytes_read" for readability?
Thank you for review. Submitting. https://codereview.chromium.org/744803003/diff/240001/tests/nonsfi/file_descr... File tests/nonsfi/file_descriptor_test.cc (right): https://codereview.chromium.org/744803003/diff/240001/tests/nonsfi/file_descr... tests/nonsfi/file_descriptor_test.cc:26: ssize_t rc = read(fd, buf, kBufLen); On 2015/01/08 01:10:55, Mark Seaborn wrote: > "rc" -> "bytes_read" for readability? Done.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/744803003/260001
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as http://src.chromium.org/viewvc/native_client?view=rev&revision=14271 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
