Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add types and de/encoding for SCMP messages #101

Merged
merged 2 commits into from
Dec 22, 2023
Merged

Conversation

mlegner
Copy link
Contributor

@mlegner mlegner commented Dec 14, 2023

Contributes to #54

Closes #112

@mlegner mlegner added documentation Improvements or additions to documentation enhancement New feature or request rust refactor labels Dec 14, 2023
@mlegner mlegner self-assigned this Dec 14, 2023
@mlegner mlegner changed the title feat(wip): add types and de/encoding for SCMP messages feat: add types and de/encoding for SCMP messages Dec 14, 2023
Copy link
Contributor

github-actions bot commented Dec 14, 2023

Code Coverage

Package Line Rate Health
crates/scion/src 79%
crates/scion-proto/src/scmp 78%
crates/scion-proto/src 80%
crates/scion-proto/src/path 82%
crates/scion-proto/src/address 70%
crates/scion/src/pan 49%
crates/scion-proto/src/path/metadata 100%
crates/scion/src/daemon 95%
crates/scion-proto/src/packet/headers 86%
crates/scion-proto/src/path/standard 90%
crates/scion-proto/src/reliable 95%
crates/scion-proto/src/packet 83%
Summary 80% (1542 / 1927)

@mlegner mlegner force-pushed the feat/scmp-types branch 3 times, most recently from a0a7b57 to 9dd2430 Compare December 14, 2023 14:04
Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Markus, looking good.

One thing that wasn't clear is why the Other* enum variants start from 0.

crates/scion-proto/src/scmp.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp.rs Outdated Show resolved Hide resolved
@mlegner
Copy link
Contributor Author

mlegner commented Dec 20, 2023

I've created a separate issue for the tests (#112) so that we can get this merged more quickly and continue with actually handling SCMP messages.

Added some basic conversion tests.

@mlegner mlegner marked this pull request as ready for review December 21, 2023 09:22
Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Markus,

Thanks for all of your work on this. I made an initial pass, but still need to go through the details of each of the messages. In the mean time, I figured I could already let you have my thoughts so far. I feel that most of what you have here is good, my main issue being with your usage of traits (here comes object-safety).

First, since traits need to be imported to use the associated methods, having the methods spread over too many traits can be uncomfortable to use. Currently there are traits for the basic methods, one with the checksum methods, and ones for information and error messages, which do not follow the object relationship:

  1. ScmpMessageBasicData -> Sized
  2. ScmpMessageEncodeDecode -> ScmpMessageBasicData
  3. ScmpErrorMessage
  4. ScmpInformationalMessage

To use most functionality, one would need to import at least n1, n3, or n1 and n4, and additionally n2 if one needs information about the checksum. Perhaps the following structure would be more usable:

  1. ScmpMessageBase
  2. ScmpMessageEncodeDecode -> ScmpMessageBase + Sized
  3. ScmpErrorMessage -> ScmpMessageBase
  4. ScmpInformationalMessage -> ScmpMessageBase

This would allow importing only n3 or n4 depending on the use-case. n2 could also be rolled into n1, since all ScmpMessages would implement it. But I imagine that logic may be used more rarely, so perhaps that's not necessary.

Second, it would be nice if n1, n3, and n4 could all be object safe. This would allow creating a Box<dyn ScmpMessageBase> where one does not care about the type of message being passed around. The current trait forces ScmpMessageBasicData to only be used with static dispatch. Doing this would require removing the associated const MESSAGE_TYPE, which could be placed on the the actual struct and then used in the implementation of the trait.

Rolling n2 into n1, would complicate the above, since that uses generic functions and returns Self in one of its methods.

Finally, ScmpMessageRaw, despite technically being a ScmpMessage, does not implement any of the traits. I would have expected it to implement n1 and n2.

crates/scion-proto/src/scmp.rs Show resolved Hide resolved
crates/scion-proto/src/scmp.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
@mlegner
Copy link
Contributor Author

mlegner commented Dec 21, 2023

@jpcsmith I've reorganized the traits in 0c2bc28. I made ScmpMessageBase public and split off a new public trait ScmpMessageChecksum, both of which are now also implemented by ScmpMessageRaw. I kept the ScmpMessageEncodeDecode private, as this mainly consists of helper functions.

Let me know what you think.

Copy link
Contributor

@jpcsmith jpcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Markus, thanks for the changes. Looks good to go.

crates/scion-proto/src/scmp/messages.rs Outdated Show resolved Hide resolved
crates/scion-proto/src/scmp.rs Outdated Show resolved Hide resolved
@mlegner mlegner enabled auto-merge (rebase) December 22, 2023 12:35
@mlegner mlegner merged commit 2c56fe6 into main Dec 22, 2023
11 checks passed
@mlegner mlegner deleted the feat/scmp-types branch December 22, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request refactor rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for SCMP messages and conversions
2 participants