Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1742043002:
Make M-x shell work in emacs. (Closed)
|
Created:
4 years, 10 months ago by bradnelson Modified:
4 years, 9 months ago CC:
native-client-reviews_googlegroups.com, Sam Clegg Base URL:
https://chromium.googlesource.com/webports.git@master Target Ref:
refs/heads/master Project:
webports Visibility:
Public. |
DescriptionMake M-x shell work in emacs.
Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process.
Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate.
Use an O_NONBLOCK pipe for reading from the output of a child process.
R=petewil@chromium.org
BUG=https://bugs.chromium.org/p/webports/issues/detail?id=247
BUG=https://bugs.chromium.org/p/webports/issues/detail?id=248
TEST=manual
Committed: https://chromium.googlesource.com/webports/+/f4761741c30df133fbc67b389fb11219f0586dac
Patch Set 1 #Patch Set 2 : fix #
Total comments: 8
Patch Set 3 : fixes #
Total comments: 2
Patch Set 4 : fix #Patch Set 5 : fix flags, add test #Patch Set 6 : fix name #
Total comments: 4
Patch Set 7 : pipe2 #Patch Set 8 : fix #Patch Set 9 : fix #
Messages
Total messages: 41 (20 generated)
Description was changed from ========== emacs shell works!! R=petewil@chromium.org ========== to ========== Make M-x shell work in emacs. Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process. Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate. Use an O_NONBLOCK pipe for reading from the output of a child process. R=petewil@chromium.org TEST=manual ==========
Mostly looks good, but please check the use of O_RDONLY. https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.js File build_tools/pipeserver.js (right): https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.... build_tools/pipeserver.js:216: * @params {Object} Dictionary of environment variables passed to process. nit: With two input variables, I expect to see two @param comments. (Yes, I know it was pre-exiting, but let's fix it while we are here.) https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch File ports/emacs/nacl.patch (right): https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch#... ports/emacs/nacl.patch:880: + report_file_error ("Creating pipe", Qnil); Please double check indentation on this line. The source file has a <tab> here, just want to make sure the tab didn't get edited. IT may be fine, but looking at the diff in the alternate diff viewer (since the main diff viewer is broken for nacl.patch), it seems that it should be indented two spaces more, but that could just be the patch utility using a different default for number of spaces in a tab (The emacs source uses 8 spaces to a tab.) https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... ports/nacl-spawn/include/spawn.h:63: * Create a pipe that can communicate cross-process via JS. The fact that we are adding a new function to get the functionality that linux already provides is an indicator that we need some later refactoring. Can you open a bug to later refactor and use the existing linux API call everywhere, properly paying attention to the flags? (or fix fusefs, or whatever needs to happen) https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... File ports/nacl-spawn/nacl_spawn.cc (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... ports/nacl-spawn/nacl_spawn.cc:714: read_fd = open(path, O_RDONLY | flags); What would happen here if flags had O_WRONLY set? Did we mean instead to clear the low bit? IIRC, O_RDONLY is 0 and O_WRONLY is 1. Maybe instead we should have (flags & ~O_WRONLY).
Description was changed from ========== Make M-x shell work in emacs. Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process. Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate. Use an O_NONBLOCK pipe for reading from the output of a child process. R=petewil@chromium.org TEST=manual ========== to ========== Make M-x shell work in emacs. Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process. Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate. Use an O_NONBLOCK pipe for reading from the output of a child process. R=petewil@chromium.org BUG=https://bugs.chromium.org/p/webports/issues/detail?id=247 BUG=https://bugs.chromium.org/p/webports/issues/detail?id=248 TEST=manual ==========
https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.js File build_tools/pipeserver.js (right): https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.... build_tools/pipeserver.js:216: * @params {Object} Dictionary of environment variables passed to process. On 2016/02/29 20:53:47, Pete Williamson wrote: > nit: With two input variables, I expect to see two @param comments. (Yes, I know > it was pre-exiting, but let's fix it while we are here.) No by all means. And you have js readability too :-) Done. https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch File ports/emacs/nacl.patch (right): https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch#... ports/emacs/nacl.patch:880: + report_file_error ("Creating pipe", Qnil); On 2016/02/29 20:53:47, Pete Williamson wrote: > Please double check indentation on this line. The source file has a <tab> here, > just want to make sure the tab didn't get edited. IT may be fine, but looking > at the diff in the alternate diff viewer (since the main diff viewer is broken > for nacl.patch), it seems that it should be indented two spaces more, but that > could just be the patch utility using a different default for number of spaces > in a tab (The emacs source uses 8 spaces to a tab.) Done another pass to make these all match up in terms of local tabbing. https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... ports/nacl-spawn/include/spawn.h:63: * Create a pipe that can communicate cross-process via JS. On 2016/02/29 20:53:47, Pete Williamson wrote: > The fact that we are adding a new function to get the functionality that linux > already provides is an indicator that we need some later refactoring. Can you > open a bug to later refactor and use the existing linux API call everywhere, > properly paying attention to the flags? (or fix fusefs, or whatever needs to > happen) Done. https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... File ports/nacl-spawn/nacl_spawn.cc (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... ports/nacl-spawn/nacl_spawn.cc:714: read_fd = open(path, O_RDONLY | flags); On 2016/02/29 20:53:47, Pete Williamson wrote: > What would happen here if flags had O_WRONLY set? Did we mean instead to clear Nothing good :-) > the low bit? IIRC, O_RDONLY is 0 and O_WRONLY is 1. > > Maybe instead we should have (flags & ~O_WRONLY). Filtering out those two.
https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.js File build_tools/pipeserver.js (right): https://codereview.chromium.org/1742043002/diff/20001/build_tools/pipeserver.... build_tools/pipeserver.js:216: * @params {Object} Dictionary of environment variables passed to process. On 2016/02/29 20:53:47, Pete Williamson wrote: > nit: With two input variables, I expect to see two @param comments. (Yes, I know > it was pre-exiting, but let's fix it while we are here.) No by all means. And you have js readability too :-) Done. https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch File ports/emacs/nacl.patch (right): https://codereview.chromium.org/1742043002/diff/20001/ports/emacs/nacl.patch#... ports/emacs/nacl.patch:880: + report_file_error ("Creating pipe", Qnil); On 2016/02/29 20:53:47, Pete Williamson wrote: > Please double check indentation on this line. The source file has a <tab> here, > just want to make sure the tab didn't get edited. IT may be fine, but looking > at the diff in the alternate diff viewer (since the main diff viewer is broken > for nacl.patch), it seems that it should be indented two spaces more, but that > could just be the patch utility using a different default for number of spaces > in a tab (The emacs source uses 8 spaces to a tab.) Done another pass to make these all match up in terms of local tabbing. https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/includ... ports/nacl-spawn/include/spawn.h:63: * Create a pipe that can communicate cross-process via JS. On 2016/02/29 20:53:47, Pete Williamson wrote: > The fact that we are adding a new function to get the functionality that linux > already provides is an indicator that we need some later refactoring. Can you > open a bug to later refactor and use the existing linux API call everywhere, > properly paying attention to the flags? (or fix fusefs, or whatever needs to > happen) Done. https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... File ports/nacl-spawn/nacl_spawn.cc (right): https://codereview.chromium.org/1742043002/diff/20001/ports/nacl-spawn/nacl_s... ports/nacl-spawn/nacl_spawn.cc:714: read_fd = open(path, O_RDONLY | flags); On 2016/02/29 20:53:47, Pete Williamson wrote: > What would happen here if flags had O_WRONLY set? Did we mean instead to clear Nothing good :-) > the low bit? IIRC, O_RDONLY is 0 and O_WRONLY is 1. > > Maybe instead we should have (flags & ~O_WRONLY). Filtering out those two.
lgtm
The CQ bit was checked by bradnelson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: webports-presubmit on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/webports-presubmit/builds...)
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1742043002/#ps60001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/60001
bsy@chromium.org changed reviewers: + bsy@chromium.org
nit https://codereview.chromium.org/1742043002/diff/40001/ports/nacl-spawn/nacl_s... File ports/nacl-spawn/nacl_spawn.cc (right): https://codereview.chromium.org/1742043002/diff/40001/ports/nacl-spawn/nacl_s... ports/nacl-spawn/nacl_spawn.cc:714: // Filter out O_RDONLY + O_WRONLY sorry about the drive-by. filtering out should be flags &= ~(O_RDONLY | O_WRONLY); if those are the only two access flags that might matter -- and POSIX does not guarantee their values -- are what we care about. however, the correct way to do this is to use O_ACCMODE, e.g., flags &= ~O_ACCMODE; to remove all access mode flags. currently we remove O_CREAT, O_EXCL, O_TRUNC, and O_APPEND (plus possibly others -- i forget exactly which open flags are supported), most of which shouldn't apply to pipes (i assume these are named pipes?). what flags did we want to retain? if it's really the comment that is misleading, and those are the only access flags that we want to retain, then we'd want to do flags &= O_ACCMODE; and then to test if what remains is one of O_RDONLY or O_WRONLY.
The CQ bit was unchecked by bradnelson@google.com
https://codereview.chromium.org/1742043002/diff/40001/ports/nacl-spawn/nacl_s... File ports/nacl-spawn/nacl_spawn.cc (right): https://codereview.chromium.org/1742043002/diff/40001/ports/nacl-spawn/nacl_s... ports/nacl-spawn/nacl_spawn.cc:714: // Filter out O_RDONLY + O_WRONLY On 2016/03/03 18:43:33, bsy_cr wrote: > sorry about the drive-by. > > filtering out should be > > flags &= ~(O_RDONLY | O_WRONLY); > > if those are the only two access flags that might matter -- and POSIX does not > guarantee their values -- are what we care about. however, the correct way to > do this is to use O_ACCMODE, e.g., > > flags &= ~O_ACCMODE; > > to remove all access mode flags. currently we remove O_CREAT, O_EXCL, O_TRUNC, > and O_APPEND (plus possibly others -- i forget exactly which open flags are > supported), most of which shouldn't apply to pipes (i assume these are named > pipes?). what flags did we want to retain? > > if it's really the comment that is misleading, and those are the only access > flags that we want to retain, then we'd want to do > > flags &= O_ACCMODE; > > and then to test if what remains is one of O_RDONLY or O_WRONLY. Thanks, that actually caught a mistake! Good to know about O_ACCMODE, but actually, come to think of it, the only flag supported at the moment is O_NONBLOCK. I've added an assert instead. Also, added a test that O_NONBLOCK works. While emacs behaves incorrectly without O_NONBLOCK, I have to do a package install + check manually (downside of a real package manager...). So I had not checked things were still working when I added the "filtering" of the flags. Thanks Bennett!
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1742043002/#ps100001 (title: "fix name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/100001
sbc@chromium.org changed reviewers: + sbc@chromium.org
https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... ports/nacl-spawn/include/spawn.h:89: int nacl_spawn_pipe_flags(int flags, int pipefd[2]); Posix functions normally put flags at the end of the arg list don't they?
https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... ports/nacl-spawn/include/spawn.h:89: int nacl_spawn_pipe_flags(int flags, int pipefd[2]); On 2016/03/03 19:20:54, Sam Clegg wrote: > Posix functions normally put flags at the end of the arg list don't they? Also the POSIX version of this is called pipe2() right? so why not call this nacl_spawn_pipe2?
https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... File ports/nacl-spawn/include/spawn.h (right): https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... ports/nacl-spawn/include/spawn.h:89: int nacl_spawn_pipe_flags(int flags, int pipefd[2]); On 2016/03/03 19:20:54, Sam Clegg wrote: > Posix functions normally put flags at the end of the arg list don't they? Indeed. https://codereview.chromium.org/1742043002/diff/100001/ports/nacl-spawn/inclu... ports/nacl-spawn/include/spawn.h:89: int nacl_spawn_pipe_flags(int flags, int pipefd[2]); On 2016/03/03 19:22:10, Sam Clegg wrote: > On 2016/03/03 19:20:54, Sam Clegg wrote: > > Posix functions normally put flags at the end of the arg list don't they? > > Also the POSIX version of this is called pipe2() right? so why not call this > nacl_spawn_pipe2? Wow, didn't know about that one. Switched to pipe2! (and flipped the args)
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1742043002/#ps120001 (title: "pipe2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: webports-linux-glibc-0 on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/webports-linux-glibc-0/bu...) webports-linux-pnacl-0 on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/webports-linux-pnacl-0/bu...)
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1742043002/#ps140001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: webports-linux-glibc-1 on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/webports-linux-glibc-1/bu...)
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1742043002/#ps160001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1742043002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1742043002/160001
Message was sent while issue was closed.
Description was changed from ========== Make M-x shell work in emacs. Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process. Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate. Use an O_NONBLOCK pipe for reading from the output of a child process. R=petewil@chromium.org BUG=https://bugs.chromium.org/p/webports/issues/detail?id=247 BUG=https://bugs.chromium.org/p/webports/issues/detail?id=248 TEST=manual ========== to ========== Make M-x shell work in emacs. Only allow children of the current foreground process that don't have their input redirected from a pipe become the foreground process. Make nacl_spawn pipe support O_NONBLOCK, and return an error code with EAGAIN/EWOULDBLOCK when appropriate. Use an O_NONBLOCK pipe for reading from the output of a child process. R=petewil@chromium.org BUG=https://bugs.chromium.org/p/webports/issues/detail?id=247 BUG=https://bugs.chromium.org/p/webports/issues/detail?id=248 TEST=manual Committed: https://chromium.googlesource.com/webports/+/f4761741c30df133fbc67b389fb11219... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/webports/+/f4761741c30df133fbc67b389fb11219...
Message was sent while issue was closed.
Hooray! I can't wait until this hits play store! On Fri, Mar 4, 2016 at 4:01 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Committed patchset #9 (id:160001) as > > https://chromium.googlesource.com/webports/+/f4761741c30df133fbc67b389fb11219... > > https://codereview.chromium.org/1742043002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
