Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1853643007:
Migrate remoting to net::IPAddress. (Closed)
DescriptionMigrate remoting to net::IPAddress.
BUG=496258
Committed: https://crrev.com/a46bd47ae7976da94fc6e7f726841719fdfbce57
Cr-Commit-Position: refs/heads/master@{#385505}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Added ParamTraits #
Total comments: 7
Patch Set 3 : comments wez #Patch Set 4 : comments wez #
Messages
Total messages: 30 (7 generated)
Patchset #1 (id:1) has been deleted
martijn@martijnc.be changed reviewers: + wez@chromium.org
Looks like you'll need an IPC reviewer for this, since you're changing chromoting_messages.h?
martijn@martijnc.be changed reviewers: + dcheng@chromium.org
On 2016/04/04 at 18:22:28, wez wrote: > Looks like you'll need an IPC reviewer for this, since you're changing chromoting_messages.h? Ah yes, you're right. +dcheng for the IPC review.
remoting/ LGTM
dcheng@chromium.org changed reviewers: + rsleevi@chromium.org
+rsleevi, any thoughts on where we could stick a ParamTraits for net::IPAddress? would it make sense to have a //net/ipc for things like this? https://codereview.chromium.org/1853643007/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/1853643007/diff/20001/remoting/host/chromotin... remoting/host/chromoting_messages.h:93: IPC_STRUCT_MEMBER(std::vector<uint8_t>, remote_address) This is kind of unfortunate: content already has a ParamTraits serialization for net::IPAddress. It would be nice to reuse this, but I'm not sure where it'd belong. //ipc probably doesn't want to depend on //net and //net probably doesn't want to depend on //ipc.
On 2016/04/04 18:40:36, dcheng wrote: > This is kind of unfortunate: content already has a ParamTraits serialization for > net::IPAddress. It would be nice to reuse this, but I'm not sure where it'd > belong. //ipc probably doesn't want to depend on //net and //net probably > doesn't want to depend on //ipc. Correct. //net depending on //ipc is a non-starter (breaks Cronet), and //ipc depending on //net is a non-starter (breaks other IPC consumers, like Adobe and Mozilla). IMO, the right thing is to duplicate this logic, as proposed, given that //remoting is a bit of a "project within a project". I think that's fine and well, and if we should find ourselves reinventing this several more times, we can consider breaking it out into a //component (aka the only place you can throw random things that don't depend on //content in)... but really, I'm not just fine, but i'm supportive of duplicating here. (See http://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction for why I think it's a good idea, for this case)
On 2016/04/04 at 19:04:29, rsleevi wrote: > On 2016/04/04 18:40:36, dcheng wrote: > > This is kind of unfortunate: content already has a ParamTraits serialization for > > net::IPAddress. It would be nice to reuse this, but I'm not sure where it'd > > belong. //ipc probably doesn't want to depend on //net and //net probably > > doesn't want to depend on //ipc. > > Correct. //net depending on //ipc is a non-starter (breaks Cronet), and //ipc depending on //net is a non-starter (breaks other IPC consumers, like Adobe and Mozilla). > > IMO, the right thing is to duplicate this logic, as proposed, given that //remoting is a bit of a "project within a project". I think that's fine and well, and if we should find ourselves reinventing this several more times, we can consider breaking it out into a //component (aka the only place you can throw random things that don't depend on //content in)... but really, I'm not just fine, but i'm supportive of duplicating here. > > (See http://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction for why I think it's a good idea, for this case) This particular instance is straightforward, but I'm not particularly thrilled about it. What's wrong with having a //net/ipc? How would that break, say, cronet? https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:360: static_cast<protocol::TransportRoute::RouteType>(route.type); Unrelated, but IPC_STRUCT_MEMBER for type should probably just be the enum type.
On 2016/04/04 at 19:13:53, dcheng wrote: > On 2016/04/04 at 19:04:29, rsleevi wrote: > > On 2016/04/04 18:40:36, dcheng wrote: > > > This is kind of unfortunate: content already has a ParamTraits serialization for > > > net::IPAddress. It would be nice to reuse this, but I'm not sure where it'd > > > belong. //ipc probably doesn't want to depend on //net and //net probably > > > doesn't want to depend on //ipc. > > > > Correct. //net depending on //ipc is a non-starter (breaks Cronet), and //ipc depending on //net is a non-starter (breaks other IPC consumers, like Adobe and Mozilla). > > > > IMO, the right thing is to duplicate this logic, as proposed, given that //remoting is a bit of a "project within a project". I think that's fine and well, and if we should find ourselves reinventing this several more times, we can consider breaking it out into a //component (aka the only place you can throw random things that don't depend on //content in)... but really, I'm not just fine, but i'm supportive of duplicating here. > > > > (See http://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction for why I think it's a good idea, for this case) > > This particular instance is straightforward, but I'm not particularly thrilled about it. What's wrong with having a //net/ipc? How would that break, say, cronet? > > https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... > File remoting/host/daemon_process.cc (right): > > https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... > remoting/host/daemon_process.cc:360: static_cast<protocol::TransportRoute::RouteType>(route.type); > Unrelated, but IPC_STRUCT_MEMBER for type should probably just be the enum type. I suspect the current code already tried to avoid this problem as it is actually serializing two net::IPEndPoint's. Splitting these up into the address and the port avoided the problem because net::IPAddressNumber was just a std::vector. Content also has ParamTraits for net::IPEndPoint. If we fix this for net::IPAddress we might want to do the same for net::IPEndPoint and update the struct to pass two net::IPEndPoints?
On 2016/04/04 19:13:53, dcheng wrote: > This particular instance is straightforward, but I'm not particularly thrilled > about it. Why not? > What's wrong with having a //net/ipc? How would that break, say, > cronet? //cronet tries to use //net as is. In general, this seems like you're asking "Why shouldn't //net depend on //ipc" - because that seems like such a heavy hammer to the problem. Even if we segment out a target (say, "net_ipc_helpers"), it's still fundamentally //net depending on //ipc, and that definitely "feels wrong" (much like having //net depend on a thing in //components; even if it "could", it feels wrong) I totally get that we want to reduce serialization, but effectively //remoting and Chrome are two separate projects that just happen to share a source repository (or perhaps, that's my impression). And the cost of duplication doesn't seem that high, compared to the cost of trying to figure out what //net's dependencies are, and the build-system-and-flags complication of trying to conditionally allow //net to depend on //ipc. That is, it'd be something that would be permitted in the DEPS rules, but disallowed by the build system, and that sort of impedance mismatch always feels wrong to me.
On 2016/04/04 20:36:31, Ryan Sleevi wrote: > //cronet tries to use //net as is. Err, that is, "cronet". Cronet is already getting complicated from the net-but-not-net stuff (c.f. websocket stripping, ICU dependencies) that's making it hard to reason about what //net can and can't assume. Ideally, we'd avoid intentionally introducing more unless there are clear wins, and I guess I'm not seeing the argument for why the duplication would be intrinsically bad.
On 2016/04/04 at 20:39:43, rsleevi wrote: > On 2016/04/04 20:36:31, Ryan Sleevi wrote: > > //cronet tries to use //net as is. > > Err, that is, "cronet". > > Cronet is already getting complicated from the net-but-not-net stuff (c.f. websocket stripping, ICU dependencies) that's making it hard to reason about what //net can and can't assume. Ideally, we'd avoid intentionally introducing more unless there are clear wins, and I guess I'm not seeing the argument for why the duplication would be intrinsically bad. OK, I don't really feel super strongly, just wanted to explore and see what the options were. All that being said, this should still be done using a ParamTraits instead of embedded directly into the IPC message handler. Just put the param traits somewhere in //remoting I guess.
Patchset #2 (id:40001) has been deleted
I added the (duplicated) ParamTraits to remoting/host/chromoting_param_traits.cc. The other ParamTraits for //remoting are defined there as well so that seemed like a good place to put them. https://codereview.chromium.org/1853643007/diff/20001/remoting/host/chromotin... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/1853643007/diff/20001/remoting/host/chromotin... remoting/host/chromoting_messages.h:93: IPC_STRUCT_MEMBER(std::vector<uint8_t>, remote_address) On 2016/04/04 at 18:40:36, dcheng wrote: > This is kind of unfortunate: content already has a ParamTraits serialization for net::IPAddress. It would be nice to reuse this, but I'm not sure where it'd belong. //ipc probably doesn't want to depend on //net and //net probably doesn't want to depend on //ipc. Added ParamTraits for net::IPAddress and net::IPEndPoint. https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/1853643007/diff/20001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:360: static_cast<protocol::TransportRoute::RouteType>(route.type); On 2016/04/04 at 19:13:53, dcheng wrote: > Unrelated, but IPC_STRUCT_MEMBER for type should probably just be the enum type. Done. https://codereview.chromium.org/1853643007/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (left): https://codereview.chromium.org/1853643007/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:335: // Validate |route|. If I understand the IPC macro correctly, IPC should be doing bound checking for the enum now?
https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:205: bytes.size() != net::IPAddress::kIPv6AddressSize) { This would be more readable as: if (bytes.empty()) { *p = net::IPAddress(); return true; } if ((bytes.size() != ...) && (bytes.size() != ...) { return false; } https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:231: if (!address.empty() && !address.IsValid()) Similarly, this is painful to parse; better to have if (address.empty()) set an empty endpoint and early-exit?
https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:205: bytes.size() != net::IPAddress::kIPv6AddressSize) { On 2016/04/05 at 17:08:01, Wez wrote: > This would be more readable as: > > if (bytes.empty()) { > *p = net::IPAddress(); > return true; > } > if ((bytes.size() != ...) && > (bytes.size() != ...) { > return false; > } Done. https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:231: if (!address.empty() && !address.IsValid()) On 2016/04/05 at 17:08:01, Wez wrote: > Similarly, this is painful to parse; better to have if (address.empty()) set an empty endpoint and early-exit? Done. (the IPAddress should always be valid (or empty) here because the IPAddress ParamTraits does the same check.)
https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:231: if (!address.empty() && !address.IsValid()) On 2016/04/05 20:08:48, martijnc wrote: > On 2016/04/05 at 17:08:01, Wez wrote: > > Similarly, this is painful to parse; better to have if (address.empty()) set > an empty endpoint and early-exit? > > Done. > > (the IPAddress should always be valid (or empty) here because the IPAddress > ParamTraits does the same check.) The IPAddress ParamTraits doesn't seem to do an IsValid check, AFAICT - that would probably make sense as the return-code. It may be that the IsValid() check always returns true because the size of the IPAddress is all that it checks, but if some checks are added later, it would be good to have them applied in Read() for IPAddress. Since IPAddress already checks these, don't check them here as well.
https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... File remoting/host/chromoting_param_traits.cc (right): https://codereview.chromium.org/1853643007/diff/60001/remoting/host/chromotin... remoting/host/chromoting_param_traits.cc:231: if (!address.empty() && !address.IsValid()) On 2016/04/05 at 20:12:27, Wez wrote: > On 2016/04/05 20:08:48, martijnc wrote: > > On 2016/04/05 at 17:08:01, Wez wrote: > > > Similarly, this is painful to parse; better to have if (address.empty()) set > > an empty endpoint and early-exit? > > > > Done. > > > > (the IPAddress should always be valid (or empty) here because the IPAddress > > ParamTraits does the same check.) > > The IPAddress ParamTraits doesn't seem to do an IsValid check, AFAICT - that would probably make sense as the return-code. It may be that the IsValid() check always returns true because the size of the IPAddress is all that it checks, but if some checks are added later, it would be good to have them applied in Read() for IPAddress. > > Since IPAddress already checks these, don't check them here as well. Done.
remoting/ LGTM
lgtm, thanks
The CQ bit was checked by martijn@martijnc.be
Thank you.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1853643007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1853643007/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Migrate remoting to net::IPAddress. BUG=496258 ========== to ========== Migrate remoting to net::IPAddress. BUG=496258 Committed: https://crrev.com/a46bd47ae7976da94fc6e7f726841719fdfbce57 Cr-Commit-Position: refs/heads/master@{#385505} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a46bd47ae7976da94fc6e7f726841719fdfbce57 Cr-Commit-Position: refs/heads/master@{#385505}
Message was sent while issue was closed.
I kicked off a thread with brett, jam, dcheng, and tsepez and... turns out I was wrong in what I recommended. https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/Wxdo5C6aVDw/RpLj... has the thread, but TL;DR: Having a //net/ipc (and/or //net/mojo) is fine.
Message was sent while issue was closed.
On 2016/04/07 at 05:49:16, rsleevi wrote: > I kicked off a thread with brett, jam, dcheng, and tsepez and... turns out I was wrong in what I recommended. > > https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/Wxdo5C6aVDw/RpLj... has the thread, but TL;DR: Having a //net/ipc (and/or //net/mojo) is fine. This Cl also broke the IPC fuzzer (https://crbug.com/601230) because of the duplicated ParamTraits (the fuzzer tries to link them both into the same binary). Moving the ParamTraits to //net/ipc and sharing them would resolve that as well. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
