Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235973002:
Add a SetDoNotFragment() method to DatagramSocket, and call this for (Closed)
|
Created:
4 years, 4 months ago by Ryan Hamilton Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a SetDoNotFragment() method to DatagramSocket, and call this for
QUIC sockets.
BUG=636544
Committed: https://crrev.com/ff006a1aab6168613e272a763d34c3cd34b58cc0
Cr-Commit-Position: refs/heads/master@{#414206}
Patch Set 1 #Patch Set 2 : jingle #Patch Set 3 : remoting #Patch Set 4 : Blimp #
Total comments: 7
Patch Set 5 : more jingle #Patch Set 6 : content #Patch Set 7 : UDP only #Patch Set 8 : net_fuzzer_test_support #Patch Set 9 : socket_host_udp_unittest.cc #Patch Set 10 : better #Patch Set 11 : fix v6 #Patch Set 12 : progress #
Total comments: 6
Patch Set 13 : Fix !$ #
Total comments: 17
Patch Set 14 : Fix mmenke's comments #
Total comments: 4
Patch Set 15 : better fixes #Patch Set 16 : Rebase #Patch Set 17 : git cl try --help #Patch Set 18 : may fail #Patch Set 19 : better v6 #Patch Set 20 : more #Patch Set 21 : win #Patch Set 22 : better win #Patch Set 23 : more win #Patch Set 24 : Rebase #Messages
Total messages: 134 (110 generated)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Add a SetDoNotFragment() method to Socket, and call this for QUIC sockets. BUG= ========== to ========== Add a SetDoNotFragment() method to Socket, and call this for QUIC sockets. BUG=636544 ==========
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rch@chromium.org changed reviewers: + jdorfman@google.com
jdorfman: can you take a look at net/udp/udp_socket_*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/10 22:07:23, Ryan Hamilton wrote: > jdorfman: can you take a look at net/udp/udp_socket_* Done. Mostly comments around some of the flexible defaults.
https://codereview.chromium.org/2235973002/diff/60001/net/socket/tcp_client_s... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/2235973002/diff/60001/net/socket/tcp_client_s... net/socket/tcp_client_socket.cc:313: return socket_->SetSendBufferSize(do_not_fragment); Copy/paste error? https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:432: int val = do_not_fragment ? IP_PMTUDISC_DO : IP_PMTUDISC_DONT; Strictly speaking, the system default is configurable and can be changed with /proc/sys/net/ipv4/ip_no_pmtu_disc. It isn't necessarily IP_PMTUDISC_DONT (it could be _WANT). You may want to read the original value when the socket is created and save it off somewhere. https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:433: int rv = setsockopt(socket_, IPPROTO_IP, IP_MTU_DISCOVER, &val, sizeof(val)); I'm not sure what will happen if the socket is v6-only (i.e. IPV6_V6ONLY was set, or /proc/sys/net/ipv6/bindv6only is 1). I think it will probably fail. It may be worth querying the socket to check the state of IPV6_V6ONLY first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rch@chromium.org to run a CQ dry run
https://codereview.chromium.org/2235973002/diff/60001/net/socket/tcp_client_s... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/2235973002/diff/60001/net/socket/tcp_client_s... net/socket/tcp_client_socket.cc:313: return socket_->SetSendBufferSize(do_not_fragment); On 2016/08/10 23:56:57, Jeremy Dorfman wrote: > Copy/paste error? Indeed, but I've reverted this change since I made SetDoNotFragment UDP specific. https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:432: int val = do_not_fragment ? IP_PMTUDISC_DO : IP_PMTUDISC_DONT; On 2016/08/10 23:56:57, Jeremy Dorfman wrote: > Strictly speaking, the system default is configurable and can be changed with > /proc/sys/net/ipv4/ip_no_pmtu_disc. It isn't necessarily IP_PMTUDISC_DONT (it > could be _WANT). You may want to read the original value when the socket is > created and save it off somewhere. Good point. I removed the argument so that this method only sets DF. https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:433: int rv = setsockopt(socket_, IPPROTO_IP, IP_MTU_DISCOVER, &val, sizeof(val)); On 2016/08/10 23:56:57, Jeremy Dorfman wrote: > I'm not sure what will happen if the socket is v6-only (i.e. IPV6_V6ONLY was > set, or /proc/sys/net/ipv6/bindv6only is 1). I think it will probably fail. It > may be worth querying the socket to check the state of IPV6_V6ONLY first. How 'bout we only do one of these codepaths depending on the address family?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:433: int rv = setsockopt(socket_, IPPROTO_IP, IP_MTU_DISCOVER, &val, sizeof(val)); On 2016/08/11 19:47:31, Ryan Hamilton wrote: > On 2016/08/10 23:56:57, Jeremy Dorfman wrote: > > I'm not sure what will happen if the socket is v6-only (i.e. IPV6_V6ONLY was > > set, or /proc/sys/net/ipv6/bindv6only is 1). I think it will probably fail. It > > may be worth querying the socket to check the state of IPV6_V6ONLY first. > > How 'bout we only do one of these codepaths depending on the address family? As discussed offline, doing only one codepath is problematic. I've changed this to call both if the socket is dual stack. The QUIC code which calls this method ignores the error code, so we *could* skip the v6-only check but I suspect the performance difference is not important relative to the correctness.
Comments on comments. Otherwise LGTM. https://codereview.chromium.org/2235973002/diff/220001/net/udp/datagram_socket.h File net/udp/datagram_socket.h (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/datagram_socke... net/udp/datagram_socket.h:38: // which prevents routers from fragmenting them. Perhaps worth noting that this may not be available depending on the OS. https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:443: DCHECK_EQ(addr_family_, AF_INET6); This line seems redundant. https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.h (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:122: // Configures the socket to set or clear the DF (Do not Fragment) bit on This can't clear the DF bit now. Also, for IPv6, it prevents hosts from fragmenting them -- routers aren't allowed to fragment IPv6 packets.
Description was changed from ========== Add a SetDoNotFragment() method to Socket, and call this for QUIC sockets. BUG=636544 ========== to ========== Add a SetDoNotFragment() method to DatagramSocket, and call this for QUIC sockets. BUG=636544 ==========
rch@chromium.org changed reviewers: + mmenke@chromium.org, sergeyu@chromium.org
sergeyu: content/browser/renderer_host/p2p/socket_host_udp_unittest.cc mmenke: net/ Thanks! https://codereview.chromium.org/2235973002/diff/220001/net/udp/datagram_socket.h File net/udp/datagram_socket.h (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/datagram_socke... net/udp/datagram_socket.h:38: // which prevents routers from fragmenting them. On 2016/08/12 17:23:41, Jeremy Dorfman wrote: > Perhaps worth noting that this may not be available depending on the OS. Done. https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:443: DCHECK_EQ(addr_family_, AF_INET6); On 2016/08/12 17:23:41, Jeremy Dorfman wrote: > This line seems redundant. Heh, whoops. Done. https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.h (right): https://codereview.chromium.org/2235973002/diff/220001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:122: // Configures the socket to set or clear the DF (Do not Fragment) bit on On 2016/08/12 17:23:41, Jeremy Dorfman wrote: > This can't clear the DF bit now. Also, for IPv6, it prevents hosts from > fragmenting them -- routers aren't allowed to fragment IPv6 packets. Done.
So what happens if we're happily sending packets with a 1450 MTU, all is going well...And then, suddenly, the path packets follow changes, and it has a smaller MTU? Packets are blackholed, and all requests to the server fail? We get an ICMP error message, fail requests, and don't use QUIC for subsequent requests? We get an ICMP error message, reduce MTU, and QUIC magically continues to work?
FWIW, QUIC servers already set the DF so this change brings Chrome's behavior inline with the servers. (We expect to make the QUIC spec require DF be set for QUIC packets). On 2016/08/12 17:52:13, mmenke wrote: > So what happens if we're happily sending packets with a 1450 MTU, all is going > well...And then, suddenly, the path packets follow changes, and it has a smaller > MTU? > > Packets are blackholed, and all requests to the server fail? > We get an ICMP error message, fail requests, and don't use QUIC for subsequent > requests? > We get an ICMP error message, reduce MTU, and QUIC magically continues to work? Currently we have no logic to handle reducing the QUIC packets size. So if we get an ICMP message back, that *should* translate into a write error which will tear down the connection. If we don't hear back, it'll be blackholed and after a timeout the connection will be blackholed. (With IPv6, routers are prohibited from fragmenting packet so we're moving to this world eventually :>) (BTW: We use 1350 byte packets (not 1450) because we found that to be a sweet spot in terms of reachability. )
On 2016/08/12 19:21:57, Ryan Hamilton wrote: > FWIW, QUIC servers already set the DF so this change brings Chrome's behavior > inline with the servers. (We expect to make the QUIC spec require DF be set for > QUIC packets). > > On 2016/08/12 17:52:13, mmenke wrote: > > So what happens if we're happily sending packets with a 1450 MTU, all is going > > well...And then, suddenly, the path packets follow changes, and it has a > smaller > > MTU? > > > > Packets are blackholed, and all requests to the server fail? > > We get an ICMP error message, fail requests, and don't use QUIC for subsequent > > requests? > > We get an ICMP error message, reduce MTU, and QUIC magically continues to > work? > > Currently we have no logic to handle reducing the QUIC packets size. So if we > get an ICMP message back, that *should* translate into a write error which will > tear down the connection. If we don't hear back, it'll be blackholed and after a > timeout the connection will be blackholed. (With IPv6, routers are prohibited > from fragmenting packet so we're moving to this world eventually :>) > > (BTW: We use 1350 byte packets (not 1450) because we found that to be a sweet > spot in terms of reachability. ) Great, as long as we don't end up in a state where we're repeatedly using (only) QUIC for a host, and QUIC is effectively broken, I'm happy.
On 2016/08/12 19:29:59, mmenke wrote: > > Great, as long as we don't end up in a state where we're repeatedly using (only) > QUIC for a host, and QUIC is effectively broken, I'm happy. Excellent! Thanks for digging into this. It's always good to have a second set of eyes... Does "happy"=> L G T M?
https://codereview.chromium.org/2235973002/diff/240001/net/udp/datagram_socket.h File net/udp/datagram_socket.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/datagram_socke... net/udp/datagram_socket.h:39: // them. Also, for IPv6, it prevents hosts from fragmenting them. Mention the return value, and if the socket is still usable on error. And whether it can be ERR_IO_PENDING (Which I assume it can't be) https://codereview.chromium.org/2235973002/diff/240001/net/udp/datagram_socke... net/udp/datagram_socket.h:39: // them. Also, for IPv6, it prevents hosts from fragmenting them. hosts -> remote endpoint (Note that this is used for both server and client sockets) https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:433: return OK; It's considered a success if we can't set this? If setting do not fragment fails, aren't we in basically the same situations as if we can't set it at all? https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:438: getsockopt(socket_, IPPROTO_IPV6, IPV6_V6ONLY, &v6_only, &v6_only_len); What if getsockopt fails? Do we care? https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:442: if (addr_family_ == AF_INET6) { Should this be merged with the above if block? Could also then early return if v6_only (And maybe only check it after setting IPPROTO_IPV6. Think that may be a little clearer. It also minimizes variable scope, which is recommended by google style guide. And saves a system call when the setsocket fails. Which I guess is good? https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:443: int val = IPV6_PMTUDISC_DO; IP_PMTUDISC_DO being defined guarantees IPV6_PMTUDISC_DO is? https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:124: // them. Also, for IPv6, it prevents hosts from fragmenting them. See other comment. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:125: int SetDoNotFragment(); Should we actually call these in tests? https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_win... net/udp/udp_socket_win.h:128: // packets, which prevents routers from fragmenting them. Does this not "for IPv6, it prevents hosts from fragmenting them"?
On 2016/08/12 19:42:45, Ryan Hamilton wrote: > On 2016/08/12 19:29:59, mmenke wrote: > > > > Great, as long as we don't end up in a state where we're repeatedly using > (only) > > QUIC for a host, and QUIC is effectively broken, I'm happy. > > Excellent! Thanks for digging into this. It's always good to have a second set > of eyes... > > Does "happy"=> L G T M? I just meant I'm fine conceptually with doing this - just wanted to make sure there were no architectural bugs with doing this. Hadn't reviewed the code yet (I have, now)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
The CQ bit was checked by rch@chromium.org to run a CQ dry run
https://codereview.chromium.org/2235973002/diff/240001/net/udp/datagram_socket.h File net/udp/datagram_socket.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/datagram_socke... net/udp/datagram_socket.h:39: // them. Also, for IPv6, it prevents hosts from fragmenting them. On 2016/08/12 19:46:05, mmenke wrote: > Mention the return value, and if the socket is still usable on error. And > whether it can be ERR_IO_PENDING (Which I assume it can't be) Done. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:433: return OK; On 2016/08/12 19:46:05, mmenke wrote: > It's considered a success if we can't set this? If setting do not fragment > fails, aren't we in basically the same situations as if we can't set it at all? Good point. Changed it to return NOT_IMPLEMENTED. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:438: getsockopt(socket_, IPPROTO_IPV6, IPV6_V6ONLY, &v6_only, &v6_only_len); On 2016/08/12 19:46:05, mmenke wrote: > What if getsockopt fails? Do we care? Good point. I don't think we *really* care, but might as well be more pedantic. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:442: if (addr_family_ == AF_INET6) { On 2016/08/12 19:46:05, mmenke wrote: > Should this be merged with the above if block? Ah, yes. This code moved around so I missed that. > Could also then early return if > v6_only (And maybe only check it after setting IPPROTO_IPV6. Think that may be > a little clearer. It also minimizes variable scope, which is recommended by > google style guide. And saves a system call when the setsocket fails. Which I > guess is good? WFM. It ends up duplicating the: return rv == 0 ? OK : MapSystemError(errno); but I'm always a fan of early return. Done. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:443: int val = IPV6_PMTUDISC_DO; On 2016/08/12 19:46:05, mmenke wrote: > IP_PMTUDISC_DO being defined guarantees IPV6_PMTUDISC_DO is? As far as I can tell. It's true for all our trybots at least. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:124: // them. Also, for IPv6, it prevents hosts from fragmenting them. On 2016/08/12 19:46:06, mmenke wrote: > See other comment. Done. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.h:125: int SetDoNotFragment(); On 2016/08/12 19:46:06, mmenke wrote: > Should we actually call these in tests? Good point. Done. https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_win.h File net/udp/udp_socket_win.h (right): https://codereview.chromium.org/2235973002/diff/240001/net/udp/udp_socket_win... net/udp/udp_socket_win.h:128: // packets, which prevents routers from fragmenting them. On 2016/08/12 19:46:06, mmenke wrote: > Does this not "for IPv6, it prevents hosts from fragmenting them"? Missed this. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:449: return rv == 0 ? OK : MapSystemError(errno); rv is always 0 here (See if statement on above line). My suggestion is to move the getsockopt just above the v6_only check, and then do: if (rv != 0 || v6_only) return rv == 0 ? OK : MapSystemError(errno); Note that you could just merge the two if's you have down here as things are, but then you'd combining the results of the two platform API calls. If you moave the getsocketopt down here, this is would just be handling the result of one call. Or could just use two if lines. Either way, think the getsocketopt makes more sense second, since it doesn't affect the setsockopt call. https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_uni... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_uni... net/udp/udp_socket_unittest.cc:588: EXPECT_TRUE(ip_address.AssignFromIPLiteral("127.0.0.1")); Should we do it with IPv6, too? The IPv6 path is more interesting. Could just use a loop, or duplicate the test.
Thanks! https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_pos... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_pos... net/udp/udp_socket_posix.cc:449: return rv == 0 ? OK : MapSystemError(errno); On 2016/08/12 21:55:49, mmenke wrote: > rv is always 0 here (See if statement on above line). My suggestion is to move > the getsockopt just above the v6_only check, and then do: > > if (rv != 0 || v6_only) > return rv == 0 ? OK : MapSystemError(errno); > > Note that you could just merge the two if's you have down here as things are, > but then you'd combining the results of the two platform API calls. If you > moave the getsocketopt down here, this is would just be handling the result of > one call. Or could just use two if lines. Either way, think the getsocketopt > makes more sense second, since it doesn't affect the setsockopt call. Done. I realize that we don't actually need rv here since we use errno not rv to determine the return value so I actually simplified the code ever so slightly. https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_uni... File net/udp/udp_socket_unittest.cc (right): https://codereview.chromium.org/2235973002/diff/260001/net/udp/udp_socket_uni... net/udp/udp_socket_unittest.cc:588: EXPECT_TRUE(ip_address.AssignFromIPLiteral("127.0.0.1")); On 2016/08/12 21:55:49, mmenke wrote: > Should we do it with IPv6, too? The IPv6 path is more interesting. Could just > use a loop, or duplicate the test. Sure. Done.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdorfman@google.com, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2235973002/#ps220002 (title: "better fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, jdorfman@google.com, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2235973002/#ps450001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a SetDoNotFragment() method to DatagramSocket, and call this for QUIC sockets. BUG=636544 ========== to ========== Add a SetDoNotFragment() method to DatagramSocket, and call this for QUIC sockets. BUG=636544 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== Add a SetDoNotFragment() method to DatagramSocket, and call this for QUIC sockets. BUG=636544 ========== to ========== Add a SetDoNotFragment() method to DatagramSocket, and call this for QUIC sockets. BUG=636544 Committed: https://crrev.com/ff006a1aab6168613e272a763d34c3cd34b58cc0 Cr-Commit-Position: refs/heads/master@{#414206} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/ff006a1aab6168613e272a763d34c3cd34b58cc0 Cr-Commit-Position: refs/heads/master@{#414206} |
