FAForever / ice-adapter

Deprecated C++ ICE adapter - please use java-ice-adapter instead!
https://github.com/FAForever/java-ice-adapter
GNU General Public License v3.0
11 stars 10 forks source link

improve connectivity check #61

Closed muellni closed 6 years ago

muellni commented 6 years ago

suggestions from ckitching from slack:

Still, your approach to ping/pong detection is a little odd. Why do you even need a ping/pong thing here? https://github.com/FAForever/ice-adapter/blob/master/PeerRelay.cpp#L288 This is also a highly fishy if statement. When is this ever expected to be false? o.0 This just smells like you've got a state machine that's not implemented properly. https://github.com/FAForever/ice-adapter/blob/master/PeerRelay.cpp#L254 This is similarly fishy. Surely you should have concretely-defined "on"/"off" states that get toggled explicitly, instead of this weird wishy-washy "check if the world make sense whenever you do anything" stuff. About ping/pong: I'm not convinced you need them. In general, though, adding your own protocol on top of the game's own one might be useful. The way I'd suggest you do that is to have a special bit pattern at the start of the buffer that you can check for (ideally this will be a sequence of no more than 4 bytes that never occurs at the start of a game-message. We probably know enough about the protocol to do that).Then you can just check if the first four bytes match the magic "it's one of our messages" thing, and, having done so, do:

memcpy(buffer + 4, foo, length - 4);

Ta-da. :smile: Er, may have got my memcpy args backwards. But you get the idea. Note that such a copy doesn't really "happen": it's just a safe way of interpreting part of a buffer as a struct type (without violating TBAA) So then you invent a nice struct that represents one of "our" messages and bob's your uncle. Otherwise you're gonna have more and more increasingly crazy uses of std::equal to detect stuff, whch isn't going to end well.