[NaCl SDK] Enable select/poll on TTY nodes.

[NaCl SDK] Enable select/poll on TTY nodes.

R=binji@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218585

[Sam Clegg, 2013-08-20 01:34:37]

This is subset of a larger change to bring our TTY support
in line with what vim/readline require:

https://codereview.chromium.org/23005005/

Ben, do you have any tips on write a test for this?
I need to be able to call select (or ki_select) with
the FD of the tty node.

In the mean time I'm testing manually with readline library
in naclports.

[binji, 2013-08-20 18:26:49]

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_char.h (right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_char.h:19: return 0;
why?

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/mount_dev_mock.h (right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/mount_dev_mock.h:24: #endif
nit: include guard comment

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc
(right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:110:
TEST_F(TtyTest, TtySelect) {
empty?

[Sam Clegg, 2013-08-20 19:09:35]

Now with tests.

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io/mount_node_char.h (right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io/mount_node_char.h:19: return 0;
On 2013/08/20 18:26:49, binji wrote:
> why?

Unlike regular files CharDevices are not always
readable or writable.

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/mount_dev_mock.h (right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/mount_dev_mock.h:24: #endif
On 2013/08/20 18:26:49, binji wrote:
> nit: include guard comment

Done.

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
File native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc
(right):

https://codereview.chromium.org/23075013/diff/8001/native_client_sdk/src/libr...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:110:
TEST_F(TtyTest, TtySelect) {
On 2013/08/20 18:26:49, binji wrote:
> empty?

Done.

[binji, 2013-08-20 20:52:56]

lgtm

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc
(right):

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:56:
const_cast<char*>(prefix.c_str())));
nit: fix indent

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:94: 100 -
content.size()));
nit: fix indent

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:114: //   2
-> Error occured
Looks like it returns -1, 0, 1

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:154: //
Should timeout when not input is available.
s/not/no/

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:155: int rtn
= ki_select(tty_fd + 1, &readfds, NULL, &errorfds, &timeout);
writefds is unused here?

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:171:
ASSERT_FALSE(FD_ISSET(tty_fd, &errorfds));
seems like you could refactor IsReadable to return whether the fd is
readable/writable/error, and simplify the code here.

[Sam Clegg, 2013-08-20 21:20:22]

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc
(right):

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:56:
const_cast<char*>(prefix.c_str())));
On 2013/08/20 20:52:56, binji wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:94: 100 -
content.size()));
On 2013/08/20 20:52:56, binji wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:114: //   2
-> Error occured
On 2013/08/20 20:52:56, binji wrote:
> Looks like it returns -1, 0, 1

Done.

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:154: //
Should timeout when not input is available.
On 2013/08/20 20:52:56, binji wrote:
> s/not/no/

Done.

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:155: int rtn
= ki_select(tty_fd + 1, &readfds, NULL, &errorfds, &timeout);
On 2013/08/20 20:52:56, binji wrote:
> writefds is unused here?

Done.

https://codereview.chromium.org/23075013/diff/16002/native_client_sdk/src/lib...
native_client_sdk/src/libraries/nacl_io_test/mount_node_tty_test.cc:171:
ASSERT_FALSE(FD_ISSET(tty_fd, &errorfds));
On 2013/08/20 20:52:56, binji wrote:
> seems like you could refactor IsReadable to return whether the fd is
> readable/writable/error, and simplify the code here.

Its not quite that simple.  In this case we want to pass a NULL timout
and also we want to be writefds whereas the other calls want to pass
NULL.

[Sam Clegg, 2013-08-21 00:11:35]

Committed patchset #7 manually as r218585 (presubmit successful).