Implemented protocol id for headers. #55
Conversation
| /// - Generating crc32 for the packet header. | ||
| /// - Validating if arriving packets have the same protocol version. | ||
| pub const PROTOCOL_VERSION: &'static str = "laminar-0.1.0"; |
There was a problem hiding this comment.
I want to think if there is a better way to do this. 🤔 . This is fine for now but if we do merge this, lets open an issue to follow up on.
There was a problem hiding this comment.
What is it that you don't like about it. You want somehow identify the current protocol I have seen this in a couple of libraries done like this. But if it is an issue lets discuss it in an issue.
There was a problem hiding this comment.
Its hard coded and I'd prefer to find a better place maybe to write this.
There was a problem hiding this comment.
I think the idea would be that it gets bumped here if the protocol changes? I'm not sure where else you could put it, unless you wanted to break it up into multiple variables and assemble a String, but not sure how that would help.
There was a problem hiding this comment.
We could use a file called VERSION that is at the root and use include_bytes!()
src/packet/packet_processor.rs
Outdated
| /// 3. then assert that the Packet we've gotten contains the right data. | ||
| #[test] | ||
| fn packets_ignored_with_different_protocol_id() { | ||
|
|
There was a problem hiding this comment.
Lets add a test here please
src/protocol_version.rs
Outdated
| /// This will return the crc32 from the current protocol version. | ||
| pub fn get_crc32() -> u32 { | ||
| CALCULATE_CRC32.call_once(|| { | ||
| unsafe { VERSION_CRC32 = crc32::checksum_ieee(PROTOCOL_VERSION.as_bytes()); } |
There was a problem hiding this comment.
So as I have learned if we call unsafe code and wrap it, we need to have some invariants that we check that guarantee that this is actually safe.
There was a problem hiding this comment.
How can we check if this is safe? We can use lazy_static if it is needed.
There was a problem hiding this comment.
I am using lazy_static instead now. Think this is more cleaner.
src/protocol_version.rs
Outdated
|
|
||
| /// This will return the crc32 from the current protocol version. | ||
| pub fn valid_version(protocol_version_crc32: u32) -> bool { | ||
| protocol_version_crc32 == unsafe { VERSION_CRC32 } |
|
bors r=fhaynes,LucioFranco |
55: Implemented protocol id for headers. r=fhaynes,LucioFranco a=TimonPost Implemented protocol id see #4 for more information. Please during review check these out: - laminar-0.1.0 From this value or crc32 will be generated do we agree on this notation for protocol version. - I use unsafe in on place see /src/protocol_version.rs, I can change this to `lazy_static` but like the current way, it could also be done. What should I do with this? - I use CRC32 for encoding of protocol version. Maybe an option to have CRC16? I don't know if there is some major difference between the who other than the size. O shit. I see that I have to comment some things from `ProtocolVersion` I'll update it later. Co-authored-by: Timon Post <timonpost@hotmail.nl>
Build succeeded |
Implemented protocol id see #4 for more information.
Please during review check these out:
From this value or crc32 will be generated do we agree on this notation for protocol version.
lazy_staticbut like the current way, it could also be done. What should I do with this?O shit. I see that I have to comment some things from
ProtocolVersionI'll update it later.