Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1323823002:
Adding nonsfi content handler (Closed)
|
Created:
5 years, 3 months ago by Sean Klein Modified:
5 years, 3 months ago Reviewers:
Mark Seaborn CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionAdding nonsfi content handler to Mojo.
Tested manually by prepending the following:
>>> "#!mojo mojo:nacl_nonsfi_content_handler"
to a nonfi nexe, and then running this nexe as follows:
>>> ./out/Debug/mojo_shell NEXE
BUG=https://github.com/domokit/mojo/issues/396
R=mseaborn@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/4f0aa600c9f6483d97772850374136f614de7128
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Responding to Code Review #
Total comments: 28
Patch Set 3 : #
Total comments: 12
Patch Set 4 : Respond to code review, git cl format #
Total comments: 1
Messages
Total messages: 15 (5 generated)
smklein@chromium.org changed reviewers: + mseaborn@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:31: "nonsfi_content_handler_main.cc", Nit: "monacl_shell_nonsfi.cc" is named with "_nonsfi" as a suffix. How about using a suffix here too for consistency? (That's assuming we keep monacl_shell_nonsfi.cc.) https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:42: "//nacl_bindings:monacl_sel", Shouldn't be needed: this refers to SFI NaCl. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... File services/nacl/nonsfi_content_handler_main.cc (right): https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:15: #include "nacl_bindings/monacl_sel_main.h" Shouldn't be needed: this refers to SFI NaCl. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:23: MojoHandle g_mojo_handle = MOJO_HANDLE_INVALID; This can go inside the anon namespace. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:33: const struct nacl_irt_mojo kIrtMojo = { This is duplicated from monacl_shell_nonsfi.cc. Let's not duplicate code. :-) Can you split this and mojo_irt_nonsfi_query() into a separate file in order to share it? https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:78: if (!mojo::common::BlockingCopyToFile(response->body.Pass(), *file_path)) { Could you make this operate on an FD rather than a filename? Then we can unlink() the temp file immediately after open()ing it, which would address the TODO below. You could do that as a separate change, as a cleanup to the existing code in content_handler_main.cc. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:89: strncpy(pathname, path.value().c_str(), PATH_MAX); Can you avoid using fixed length buffers? Why not pass path.value().c_str() to open()? https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:121: // Aquire the nexe. "Acquire". Same below. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:127: //mojo::URLLoaderPtr url_loader = url_loader_.Pass(); Remove commented-out code. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:137: char* argvp = &argv[0]; You're using the temp filename as the program's argv[0], but it would be better not to expose the temp filename. You could pass a fixed argv[0] string here. For comparison, sel_main.c uses "NaClMain". https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:139: nacl_irt_nonsfi_entry(argc, &argvp, environ, This function is not supposed to return. You can call abort() after it. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:142: // TODO(ncbray): are the file handles actually closed at this point? Yes, |fd| gets closed by NaClLoadElfFile(). See native_client/src/nonsfi/loader/elf_loader.c. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:145: if (!base::DeleteFile(nexe_path, false)) { Since control won't reach here, this cleanup is not going to happen. You could do this cleanup immediately after we open() the file.
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:31: "nonsfi_content_handler_main.cc", On 2015/09/01 15:55:45, Mark Seaborn wrote: > Nit: "monacl_shell_nonsfi.cc" is named with "_nonsfi" as a suffix. How about > using a suffix here too for consistency? > > (That's assuming we keep monacl_shell_nonsfi.cc.) Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:42: "//nacl_bindings:monacl_sel", On 2015/09/01 15:55:45, Mark Seaborn wrote: > Shouldn't be needed: this refers to SFI NaCl. Done (it was needed for "mojo::NaClExit", which I am no longer using anyway). https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... File services/nacl/nonsfi_content_handler_main.cc (right): https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:15: #include "nacl_bindings/monacl_sel_main.h" On 2015/09/01 15:55:45, Mark Seaborn wrote: > Shouldn't be needed: this refers to SFI NaCl. Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:23: MojoHandle g_mojo_handle = MOJO_HANDLE_INVALID; On 2015/09/01 15:55:45, Mark Seaborn wrote: > This can go inside the anon namespace. Done -- sort of. Moved into anon namespace of the interface file. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:33: const struct nacl_irt_mojo kIrtMojo = { On 2015/09/01 15:55:45, Mark Seaborn wrote: > This is duplicated from monacl_shell_nonsfi.cc. Let's not duplicate code. :-) > Can you split this and mojo_irt_nonsfi_query() into a separate file in order to > share it? Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:78: if (!mojo::common::BlockingCopyToFile(response->body.Pass(), *file_path)) { On 2015/09/01 15:55:45, Mark Seaborn wrote: > Could you make this operate on an FD rather than a filename? Then we can > unlink() the temp file immediately after open()ing it, which would address the > TODO below. > > You could do that as a separate change, as a cleanup to the existing code in > content_handler_main.cc. I will do it in this change for the nonsfi version -- I'll add a separate change for the SFI version. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:89: strncpy(pathname, path.value().c_str(), PATH_MAX); On 2015/09/01 15:55:45, Mark Seaborn wrote: > Can you avoid using fixed length buffers? Why not pass path.value().c_str() to > open()? Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:121: // Aquire the nexe. On 2015/09/01 15:55:45, Mark Seaborn wrote: > "Acquire". Same below. Whoops. Fixed. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:127: //mojo::URLLoaderPtr url_loader = url_loader_.Pass(); On 2015/09/01 15:55:45, Mark Seaborn wrote: > Remove commented-out code. Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:137: char* argvp = &argv[0]; On 2015/09/01 15:55:45, Mark Seaborn wrote: > You're using the temp filename as the program's argv[0], but it would be better > not to expose the temp filename. You could pass a fixed argv[0] string here. > For comparison, sel_main.c uses "NaClMain". Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:139: nacl_irt_nonsfi_entry(argc, &argvp, environ, On 2015/09/01 15:55:45, Mark Seaborn wrote: > This function is not supposed to return. You can call abort() after it. Done. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:142: // TODO(ncbray): are the file handles actually closed at this point? On 2015/09/01 15:55:45, Mark Seaborn wrote: > Yes, |fd| gets closed by NaClLoadElfFile(). See > native_client/src/nonsfi/loader/elf_loader.c. Added comment noting this. https://codereview.chromium.org/1323823002/diff/60001/services/nacl/nonsfi_co... services/nacl/nonsfi_content_handler_main.cc:145: if (!base::DeleteFile(nexe_path, false)) { On 2015/09/01 15:55:45, Mark Seaborn wrote: > Since control won't reach here, this cleanup is not going to happen. > > You could do this cleanup immediately after we open() the file. Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... File mojo/nacl/mojo_interface_nacl_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.cc:9: MojoHandle g_mojo_handle = MOJO_HANDLE_INVALID; Is there potentially an issue with having a single, global "g_mojo_handle" between any functions which want to launch nexes? If I use both the "monacl shell" and "content handler + mojo shell" (both nonsfi versions) to run two nexes at the same time, and both of them try to call "mojo_set_initial_handle", it seems like a race condition could exist.
https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/BUILD.gn#new... mojo/nacl/BUILD.gn:77: "mojo_interface_nacl_nonsfi.cc", Naming nit: irt_mojo_nonsfi.cc perhaps? It would be good to have "irt" in the name. "nacl" is redundant given this is in mojo/nacl/. (Well, "mojo" is redundant too, but "irt_mojo" matches the naming scheme in native_client/src/untrusted/irt/, components/nacl/loader/nonsfi/ and ppapi/nacl_irt/ at least.) https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... File mojo/nacl/mojo_interface_nacl_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.cc:9: MojoHandle g_mojo_handle = MOJO_HANDLE_INVALID; On 2015/09/01 20:24:48, smklein1 wrote: > Is there potentially an issue with having a single, global "g_mojo_handle" > between any functions which want to launch nexes? > > If I use both the "monacl shell" and "content handler + mojo shell" (both nonsfi > versions) to run two nexes at the same time, and both of them try to call > "mojo_set_initial_handle", it seems like a race condition could exist. Yes. We don't really want to run multiple nexes in the same process though. If one crashes or calls exit(), it will bring any others down with it. The global handle is OK for now. We can revisit this when we work out what the process model should be -- i.e. how to get mojo_shell to create separate processes. I think we'd also want to revisit whether GetInitialHandle() should consume the handle, so that if it's called twice, but the handle is closed between those calls, the second caller can't get a dangling handle. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.cc:12: // TODO(smklein): Test this handle Nit: I would avoid TODOs where the TODO would be addressed in a different location in the codebase. It's likely we'd forget to remove it. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... File mojo/nacl/mojo_interface_nacl_nonsfi.h (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:5: #ifndef MOJO_INTERFACE_NACL_NONSFI_H_ Style nit: I believe these #define names are supposed to follow the full pathname, to avoid potential clashes, i.e. MOJO_NACL_MOJO_INTERFACE_NACL_NONSFI_H_. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:9: #include "mojo/public/platform/nacl/mojo_irt.h" Nit: not needed, following #include-what-you-use. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:10: #include "native_client/src/public/irt_core.h" Also not needed. You just need stdlib.h for size_t. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:14: void mojo_set_initial_handle(MojoHandle handle); Can you use CamelCase and put these in a namespace, following Chromium C++ style, please? underscore_naming_style is from the NaCl codebase, and it's always been oddly inconsistent that NaCl uses that in untrusted code but not in trusted code. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:16: // Standard mojo nonsfi query function which may be passed to Can you say "IRT interface query function" to add more context? https://codereview.chromium.org/1323823002/diff/100001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:29: mojo_native_application("nacl_nonsfi_content_handler") { Nit: make "nonsfi" a suffix here too? https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:28: if (!mojo::common::BlockingCopyToFile(response->body.Pass(), path)) { This still has the issue that if the process is killed part-way through the download, a non-empty temp file is left behind. It would be better if there were a variant of BlockingCopyToFile() that writes to an FD. That could be done in a separate change? https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:33: if (fd < 0) { fd is a pointer. This is "fd < NULL". :-) https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:83: char* argvp = (char *) "NaClMain"; Nit: use C++ style cast https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:84: nacl_irt_nonsfi_entry(argc, &argvp, environ, Let's use an empty set of env vars instead of passing environment variables through by default. Nick's SFI NaCl version doesn't pass env vars through (since it uses NaClChromeMainStart()).
https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/BUILD.gn#new... mojo/nacl/BUILD.gn:77: "mojo_interface_nacl_nonsfi.cc", On 2015/09/01 21:05:46, Mark Seaborn wrote: > Naming nit: irt_mojo_nonsfi.cc perhaps? > > It would be good to have "irt" in the name. > > "nacl" is redundant given this is in mojo/nacl/. (Well, "mojo" is redundant > too, but "irt_mojo" matches the naming scheme in > native_client/src/untrusted/irt/, components/nacl/loader/nonsfi/ and > ppapi/nacl_irt/ at least.) Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... File mojo/nacl/mojo_interface_nacl_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.cc:9: MojoHandle g_mojo_handle = MOJO_HANDLE_INVALID; On 2015/09/01 21:05:46, Mark Seaborn wrote: > On 2015/09/01 20:24:48, smklein1 wrote: > > Is there potentially an issue with having a single, global "g_mojo_handle" > > between any functions which want to launch nexes? > > > > If I use both the "monacl shell" and "content handler + mojo shell" (both > nonsfi > > versions) to run two nexes at the same time, and both of them try to call > > "mojo_set_initial_handle", it seems like a race condition could exist. > > Yes. We don't really want to run multiple nexes in the same process though. If > one crashes or calls exit(), it will bring any others down with it. > > The global handle is OK for now. We can revisit this when we work out what the > process model should be -- i.e. how to get mojo_shell to create separate > processes. > > I think we'd also want to revisit whether GetInitialHandle() should consume the > handle, so that if it's called twice, but the handle is closed between those > calls, the second caller can't get a dangling handle. Acknowledged. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.cc:12: // TODO(smklein): Test this handle On 2015/09/01 21:05:46, Mark Seaborn wrote: > Nit: I would avoid TODOs where the TODO would be addressed in a different > location in the codebase. It's likely we'd forget to remove it. Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... File mojo/nacl/mojo_interface_nacl_nonsfi.h (right): https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:5: #ifndef MOJO_INTERFACE_NACL_NONSFI_H_ On 2015/09/01 21:05:46, Mark Seaborn wrote: > Style nit: I believe these #define names are supposed to follow the full > pathname, to avoid potential clashes, i.e. > MOJO_NACL_MOJO_INTERFACE_NACL_NONSFI_H_. Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:9: #include "mojo/public/platform/nacl/mojo_irt.h" On 2015/09/01 21:05:46, Mark Seaborn wrote: > Nit: not needed, following #include-what-you-use. Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:10: #include "native_client/src/public/irt_core.h" On 2015/09/01 21:05:46, Mark Seaborn wrote: > Also not needed. You just need stdlib.h for size_t. Done -- though stdlib is not needed either. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:14: void mojo_set_initial_handle(MojoHandle handle); On 2015/09/01 21:05:46, Mark Seaborn wrote: > Can you use CamelCase and put these in a namespace, following Chromium C++ > style, please? > > underscore_naming_style is from the NaCl codebase, and it's always been oddly > inconsistent that NaCl uses that in untrusted code but not in trusted code. Done. https://codereview.chromium.org/1323823002/diff/100001/mojo/nacl/mojo_interfa... mojo/nacl/mojo_interface_nacl_nonsfi.h:16: // Standard mojo nonsfi query function which may be passed to On 2015/09/01 21:05:46, Mark Seaborn wrote: > Can you say "IRT interface query function" to add more context? Done. https://codereview.chromium.org/1323823002/diff/100001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:29: mojo_native_application("nacl_nonsfi_content_handler") { On 2015/09/01 21:05:46, Mark Seaborn wrote: > Nit: make "nonsfi" a suffix here too? Done. https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:28: if (!mojo::common::BlockingCopyToFile(response->body.Pass(), path)) { On 2015/09/01 21:05:47, Mark Seaborn wrote: > This still has the issue that if the process is killed part-way through the > download, a non-empty temp file is left behind. It would be better if there > were a variant of BlockingCopyToFile() that writes to an FD. That could be done > in a separate change? Made a separate CL for this issue (I'll post it as soon as this one goes through). It seems to work though -- I made a new "BlockingCopyToFile"-esque function which creates a temp file, unlinks the file, copies, and then returns a file descriptor. Isn't there still the possibility of leaving an empty temp file between creating and unlinking? Or is this considered an acceptable failure state? https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:33: if (fd < 0) { On 2015/09/01 21:05:47, Mark Seaborn wrote: > fd is a pointer. This is "fd < NULL". :-) Whoops! Changed to (*fd < 0) https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:83: char* argvp = (char *) "NaClMain"; On 2015/09/01 21:05:47, Mark Seaborn wrote: > Nit: use C++ style cast Using const_cast. https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:84: nacl_irt_nonsfi_entry(argc, &argvp, environ, On 2015/09/01 21:05:47, Mark Seaborn wrote: > Let's use an empty set of env vars instead of passing environment variables > through by default. > > Nick's SFI NaCl version doesn't pass env vars through (since it uses > NaClChromeMainStart()). Done.
LGTM https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/100001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:28: if (!mojo::common::BlockingCopyToFile(response->body.Pass(), path)) { On 2015/09/01 22:55:27, smklein1 wrote: > Isn't there still the possibility of leaving an empty temp file between > creating and unlinking? Or is this considered an acceptable failure state? Yes, that's still a possibility, but the window is much smaller, and the file would only be zero-size. Using Linux's new memfd_create() syscall would avoid this problem, but we can't rely on it being available yet. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/BUILD.gn#new... mojo/nacl/BUILD.gn:75: source_set("irt_mojo_nonsfi") { Nit: Doesn't source_set mean this would get compiled twice, for each target that uses it? If so, making this a library would be better. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... File mojo/nacl/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.cc:5: #include "irt_mojo_nonsfi.h" Nit: #includes should use the full path, so add "mojo/nacl/" https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.cc:26: const struct nacl_irt_mojo kIrtMojo = { Nit: these two vars can go inside the anon namespace too https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... File mojo/nacl/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.h:12: void MojoSetInitialHandle(MojoHandle handle); Can you put both of these into a C++ namespace, please? https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.h:16: size_t mojo_irt_nonsfi_query(const char* interface_ident, Can you make this CamelCase too? https://codereview.chromium.org/1323823002/diff/120001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/120001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:78: // Run -- Also, closes the fd, removing the temp file. Grammar nit: Don't capitalise "Also"
https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/BUILD.gn#new... mojo/nacl/BUILD.gn:75: source_set("irt_mojo_nonsfi") { On 2015/09/02 20:27:15, Mark Seaborn wrote: > Nit: Doesn't source_set mean this would get compiled twice, for each target that > uses it? If so, making this a library would be better. Switched to static_library. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... File mojo/nacl/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.cc:5: #include "irt_mojo_nonsfi.h" On 2015/09/02 20:27:16, Mark Seaborn wrote: > Nit: #includes should use the full path, so add "mojo/nacl/" Done. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.cc:26: const struct nacl_irt_mojo kIrtMojo = { On 2015/09/02 20:27:16, Mark Seaborn wrote: > Nit: these two vars can go inside the anon namespace too Done. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... File mojo/nacl/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.h:12: void MojoSetInitialHandle(MojoHandle handle); On 2015/09/02 20:27:16, Mark Seaborn wrote: > Can you put both of these into a C++ namespace, please? Done. https://codereview.chromium.org/1323823002/diff/120001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.h:16: size_t mojo_irt_nonsfi_query(const char* interface_ident, On 2015/09/02 20:27:16, Mark Seaborn wrote: > Can you make this CamelCase too? Done. https://codereview.chromium.org/1323823002/diff/120001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1323823002/diff/120001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:78: // Run -- Also, closes the fd, removing the temp file. On 2015/09/02 20:27:16, Mark Seaborn wrote: > Grammar nit: Don't capitalise "Also" Done.
Message was sent while issue was closed.
Committed patchset #4 (id:140001) manually as 4f0aa600c9f6483d97772850374136f614de7128 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1323823002/diff/140001/mojo/nacl/irt_mojo_non... File mojo/nacl/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1323823002/diff/140001/mojo/nacl/irt_mojo_non... mojo/nacl/irt_mojo_nonsfi.h:10: namespace irtNonsfi { Nit: namespace names should be all lower-case -- https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespace_Names "namespace nacl" would be OK. It's not really essential to use namespaces, it just seems cleaner to start with this convention. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
