Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
304373008:
Add support for modifying TCP and UDP socket buffer size in nacl_io (Closed)
|
Created:
6 years, 6 months ago by mbajpai Modified:
6 years, 1 month ago CC:
chromium-reviews, binji+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for modifying TCP and UDP socket buffer size in nacl_io
setsockopt() can now be called with these options:
SO_RCVBUF
SO_SNDBUF
BUG=none
Committed: https://crrev.com/97bfef2c02f89b66e3b5d48cdca796796062fc62
Cr-Commit-Position: refs/heads/master@{#302167}
Patch Set 1 #Patch Set 2 : Added unit tests #
Total comments: 1
Patch Set 3 : Corrected the unit tests #
Total comments: 3
Patch Set 4 : Fixed the bug in the TCP test and added comments #
Total comments: 1
Patch Set 5 : in socket_test.cc, change accept() to ki_accept() per review comments #
Messages
Total messages: 29 (1 generated)
This review is per discussion: https://groups.google.com/forum/#!searchin/native-client-discuss/nacl_io$20so...
On 2014/05/30 22:17:32, mbajpai wrote: > This review is per discussion: > https://groups.google.com/forum/#!searchin/native-client-discuss/nacl_io$20so... Thanks Manish! This change looks good. Do you think you could add a unit test to nacl_io_socket_test?
On 2014/05/30 22:49:22, Sam Clegg wrote: > On 2014/05/30 22:17:32, mbajpai wrote: > > This review is per discussion: > > > https://groups.google.com/forum/#!searchin/native-client-discuss/nacl_io$20so... > > Thanks Manish! This change looks good. > > Do you think you could add a unit test to nacl_io_socket_test? Also, you didn't add any reviewers to the issue yet.. just FYI.
On 2014/05/30 22:49:52, Sam Clegg wrote: > On 2014/05/30 22:49:22, Sam Clegg wrote: > > On 2014/05/30 22:17:32, mbajpai wrote: > > > This review is per discussion: > > > > > > https://groups.google.com/forum/#!searchin/native-client-discuss/nacl_io$20so... > > > > Thanks Manish! This change looks good. > > > > Do you think you could add a unit test to nacl_io_socket_test? > > Also, you didn't add any reviewers to the issue yet.. just FYI. Added reviewers. Please add/remove as needed. Working on adding a test.
> Added reviewers. Please add/remove as needed. Working on adding a test. Have removed the reviewers pending the result of trybots
Quick review note. https://codereview.chromium.org/304373008/diff/20001/native_client_sdk/src/te... File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): https://codereview.chromium.org/304373008/diff/20001/native_client_sdk/src/te... native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:526: sock2_ = socket(AF_INET, SOCK_STREAM, 0); I think you meant to call ki_socket here, instead of socket?
On 2014/06/02 19:38:18, CodeByThePound wrote: > Quick review note. > > https://codereview.chromium.org/304373008/diff/20001/native_client_sdk/src/te... > File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): > > https://codereview.chromium.org/304373008/diff/20001/native_client_sdk/src/te... > native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:526: sock2_ = > socket(AF_INET, SOCK_STREAM, 0); > I think you meant to call ki_socket here, instead of socket? Thanks. Fixed the unit tests. Please review.
https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:595: << "failed with: " << strerror(errno); Can you verify the result with getsockopt()? https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:852: << "failed with: " << strerror(errno); ditto.
https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:595: << "failed with: " << strerror(errno); On 2014/06/03 00:07:54, Sam Clegg wrote: > Can you verify the result with getsockopt()? It is not clear to me how to do it. Unlike SetOption, TCP/UDP interface doesn't have GetOption call implemented. Other option would be to save the value in udp_node.cc class but since the OS can change the actual buffer size, I did not like that idea. I notice that some trybot tests are failing. I have no idea, why.
On 2014/06/03 05:11:07, mbajpai wrote: > https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... > File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): > > https://codereview.chromium.org/304373008/diff/40001/native_client_sdk/src/te... > native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:595: << "failed > with: " << strerror(errno); > On 2014/06/03 00:07:54, Sam Clegg wrote: > > Can you verify the result with getsockopt()? > > It is not clear to me how to do it. Unlike SetOption, TCP/UDP interface doesn't > have GetOption call implemented. > Other option would be to save the value in udp_node.cc class but since the OS > can change the actual buffer size, I did not like that idea. Oh, that is a shame. Could you add a note to the test code about this. It sounds like we really need to extend the pepper interfaces for this purpose. > > I notice that some trybot tests are failing. I have no idea, why. Well its look like it your new test that is failing. Have you manged to run it locally? In what configuration?
From the logs it looks like just the TCP BUFSIZE test is failing (in all configurations) but the UDP test is passing.
On 2014/06/03 05:47:27, Sam Clegg wrote: > From the logs it looks like just the TCP BUFSIZE test is failing (in all > configurations) but the UDP test is passing. Oh actually the test passed locally on all the configs - which included linux, newlib, and some more. The trybot test is not failing on all configs when I looked at the logs. Having said that, I feel there is clearly something wrong with the test coz I have followed the exact pattern from another test from the same class and yet that one is not failing - most likely a copy-paste-replace error.
Have fixed the error with TCP BUFSIZE test. Please review.
lgtm, please wait for green trybots.
On 2014/06/05 18:22:13, Sam Clegg wrote: > lgtm, please wait for green trybots. The test is still failing on one trybot. How do I debug it?
https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:849: int new_sock = accept(server_sock, (sockaddr*)&addr, &addrlen); this should be ki_accept I think that will fix your test failure.
On 2014/06/05 23:10:20, binji wrote: > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): > > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:849: int new_sock > = accept(server_sock, (sockaddr*)&addr, &addrlen); > this should be ki_accept > > I think that will fix your test failure. Could be. But I noticed that nowhere else is ki_accept being used in the test file. I will do some testing and resubmit the review.
On 2014/06/05 23:28:55, mbajpai wrote: > On 2014/06/05 23:10:20, binji wrote: > > > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > > File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): > > > > > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > > native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:849: int > new_sock > > = accept(server_sock, (sockaddr*)&addr, &addrlen); > > this should be ki_accept > > > > I think that will fix your test failure. > > Could be. But I noticed that nowhere else is ki_accept being used in the test > file. I will do some testing and resubmit the review. Would be good to get his landed. Any progress on the failing test?
On 2014/10/17 17:48:03, Sam Clegg wrote: > On 2014/06/05 23:28:55, mbajpai wrote: > > On 2014/06/05 23:10:20, binji wrote: > > > > > > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > > > File native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc (right): > > > > > > > > > https://codereview.chromium.org/304373008/diff/60001/native_client_sdk/src/te... > > > native_client_sdk/src/tests/nacl_io_socket_test/socket_test.cc:849: int > > new_sock > > > = accept(server_sock, (sockaddr*)&addr, &addrlen); > > > this should be ki_accept > > > > > > I think that will fix your test failure. > > > > Could be. But I noticed that nowhere else is ki_accept being used in the test > > file. I will do some testing and resubmit the review. > > Would be good to get his landed. Any progress on the failing test? I tried the change locally. The test didn't fail on my linux before or after the change. I will upload the suggested change if you want to fire a trybot - but I have no idea why it didn't work in the first place.
Uploaded the suggested change. Please run the trybots.
Sam, Is there anything else we are waiting for? Thanks, Manish On 2014/10/18 21:19:57, mbajpai wrote: > Uploaded the suggested change. Please run the trybots.
On 2014/10/23 17:13:15, mbajpai wrote: > Sam, > Is there anything else we are waiting for? > > Thanks, > Manish > > On 2014/10/18 21:19:57, mbajpai wrote: > > Uploaded the suggested change. Please run the trybots. Sorry, no. Trybots running now.
> > Sorry, no. Trybots running now. No idea why Linux build ran into trouble. Could it be because I haven't synced to the top of the tree in a long time?
On 2014/10/24 17:02:58, mbajpai wrote: > > > > Sorry, no. Trybots running now. > > No idea why Linux build ran into trouble. Could it be because I haven't synced > to the top of the tree in a long time? Is there a better way of getting this checked in? Looks like early linux bot failure is gone but now there is a new failure in linux_bionic
The CQ bit was checked by sbc@chromium.org
On 2014/10/30 20:33:49, mbajpai wrote: > On 2014/10/24 17:02:58, mbajpai wrote: > > > > > > Sorry, no. Trybots running now. > > > > No idea why Linux build ran into trouble. Could it be because I haven't synced > > to the top of the tree in a long time? > > Is there a better way of getting this checked in? Looks like early linux bot > failure is gone but now there is a new failure in linux_bionic I checked the commit queue box, so this change should land as soon as the bots pass.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/304373008/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/97bfef2c02f89b66e3b5d48cdca796796062fc62 Cr-Commit-Position: refs/heads/master@{#302167} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
