Implement Query protocol #218
Conversation
Added packet data types Correctly parse a handshake packet Add tokio to pumpkin protocol for `AsyncReadExt` convenience functions
Switch to CString since its better to and null errors higher up the code
Respect config options Remove uncessary debug derives
Refactor code for better error handling
|
I think you don't need to save the clients into a HashMap, Your example we also don't save RCON Clients into a hashmap: https://github.com/Snowiiii/Pumpkin/blob/master/pumpkin/src/rcon/mod.rs#L33. I also very dislike to have a Struct named "QueryClients" |
|
Looking at RCON, since it uses TCP each client can take a TcpStream and not have to be saved in a central place. The thing is UDP is stateless, so there is nothing like a UdpStream where each client can own their connection. This means that the clients need to be stored in a central place and all packets can only be read from the socket. What should I rename |
There was a problem hiding this comment.
Overall this looks great! I appreciate how self-contained this PR is. I have a couple of requested changes, but it's mostly to try and clean things up.
I won't complain if you said no to the extra work, but I would truly appreciate it if you wrote a handful of tests for some of the (de)serialization to validate the format. Just something which can be used to verify that a packet gets read (or written) in a way that means if someone comes through "cleaning" up this code, we can know it matches what we expect from your original implementation.
Break up en/decoding for each packet Reduce client tracking Get rid of QueryClients struct Stop storing of magic value and check during decoding Added tests for all decoding and encoding Fix all sugestions from @StripedMonkey
|
Just tested and it works pretty well. I also like the new changed code much more (Thanks @StripedMonkey for helping reviewing). |
|
@neeleshpoli I just noticed that for some reason Query is on by default. It should be off by default |
Currently don't have time, if you want you can make a PR, I think it should be pretty easy to fix |
* Add query options to config Added packet data types Correctly parse a handshake packet Add tokio to pumpkin protocol for `AsyncReadExt` convenience functions * Add repr(u8) so that PacketType is a byte * Respond to handshake correctly * Allow for proper decoding of status packets and properly handle errors in decoding * Implement challange tokens and verify with token and address * Encode Basic status packet Switch to CString since its better to and null errors higher up the code * Encode Full status packet * Add forgotten fields to full status packet * Correctly respond to query clients when requesting full status * Return actual address and port of server Respect config options Remove uncessary debug derives * Remove packet type as it is redundant/unnecssary * Implement basic status request Refactor code for better error handling * Update README * Show players correctly in full status packet * Store all packets in structs instead of enums Break up en/decoding for each packet Reduce client tracking Get rid of QueryClients struct Stop storing of magic value and check during decoding Added tests for all decoding and encoding Fix all sugestions from @StripedMonkey
Description
This implements the Query protocol, supported by the vanilla server, to allow for a different way to get information about the server.
More info at protocol docs
Testing
Send test query packets using services like
Checklist
Things need to be done before this Pull Request can be merged.
cargo fmtcargo clippy