icmp redux #89
icmp redux #89
Conversation
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
==========================================
+ Coverage 61.35% 68.31% +6.96%
==========================================
Files 60 60
Lines 4270 4621 +351
==========================================
+ Hits 2620 3157 +537
+ Misses 1650 1464 -186
Continue to review full report at Codecov.
|
.
core/src/packets/icmp/v4/mod.rs
Outdated
| /// | ||
| /// The trait is used for conversion between the generic [ICMPv4] packet | ||
| /// and the more specific messages. Implementors can use this trait to | ||
| /// add custom message types. This trait should not be used directly. Use |
There was a problem hiding this comment.
used or imported directly?
core/src/packets/icmp/v4/mod.rs
Outdated
| /// which also derives the implementation for the [`Packet`] trait. | ||
| /// This trait should be used with `#[derive]`. The struct must manually | ||
| /// implement [`Icmpv4Message`] trait. `#[derive]` will also provide an | ||
| /// implementation of [`Packet`] trait for the struct as well. |
There was a problem hiding this comment.
don't need as well and also. Same goes for icmpv6packet derive info.
| } | ||
|
|
||
| #[capsule::test] | ||
| fn push_and_set_echo_reply() { |
There was a problem hiding this comment.
maybe as a separate, smaller test, you can showcase set_code from within a message type. No biggie, as it's own, but just to exercise the setting on some field like that?
There was a problem hiding this comment.
neither echo request or reply should have non-0 code. so doesn't make sense to include. I've added set_code to v6 time exceeded test cuz that message type can have different codes.
There was a problem hiding this comment.
i meant generally, just picked this for ease. But yeah, as per slack discussion, we should just notate set_code's usefulness only in very specific situations/specs.
core/src/packets/icmp/v4/mod.rs
Outdated
| /// Prepends an ICMPv4 packet to the beginning of the IPv4's payload. | ||
| /// | ||
| /// [`Ipv4::protocol`] is set to [`ProtocolNumbers::Icmpv4`]. | ||
| /// Cannot push an ICMPv4 header without a message body. This function |
There was a problem hiding this comment.
minor: link to types (echorequest/reply) that are message types? (maybe wording can be updated here b/c those messages have "bodies" per se.
| let _ = self.envelope_mut().truncate(IPV6_MIN_MTU); | ||
| self.compute_checksum(); | ||
| pub fn data(&self) -> &[u8] { | ||
| let offset = self.payload_offset() + TimeExceededBody::size_of(); |
There was a problem hiding this comment.
In seeing https://github.com/capsule-rs/capsule/pull/89/files#diff-1925ff5cfe7a1e975f1b9e48f02a21edR92, maybe it'd be nice for this and PacketTooBig to have "fns" for data_offset and data_len?
There was a problem hiding this comment.
those are just private fns anyway. they are used in echo request/reply by both the getter and the setter. here they are only used once.
There was a problem hiding this comment.
yeah its' minor/fine. I was trying to think of how these data fns could be made general, but obviously, only certain types work w/ "data" payloads, not all of them.
| @@ -108,8 +108,7 @@ | |||
| //! [`skeleton`]: https://github.com/capsule-rs/capsule/tree/master/examples/skeleton | |||
| //! [`syn-flood`]: https://github.com/capsule-rs/capsule/tree/master/examples/syn-flood | |||
|
|
|||
| // alias for the test macro | |||
| #[cfg(test)] | |||
| @@ -23,144 +23,93 @@ pub fn gen_icmpv6(input: syn::DeriveInput) -> TokenStream { | |||
| let name = input.ident; | |||
|
|
|||
| let expanded = quote! { | |||
| impl<E: Ipv6Packet> crate::packets::icmp::v6::Icmpv6Packet<E, #name> for Icmpv6<E, #name> { | |||
| impl<E: ::capsule::packets::ip::v6::Ipv6Packet> ::capsule::packets::icmp::v6::Icmpv6Packet for #name<E> { | |||
| @@ -419,11 +434,34 @@ mod tests { | |||
| let packet = Mbuf::from_bytes(&ICMPV4_PACKET).unwrap(); | |||
There was a problem hiding this comment.
q: @drunkirishcoder for setting a good example (of how to add tests), do we want parse tests for each message type specifically, pulling in more byte arrays from one of these places: https://cloudshark.io/articles/how-to-get-sample-captures/? Know it's tedious, but sets a good example for adding more protocols, and you did something good by adding all these push_and_set* tests :).
There was a problem hiding this comment.
I don't think parsing each type is that interesting or error prone. and some types like time exceeded and packet too big require a large payload to be defined, not ideal as byte arrays.
There was a problem hiding this comment.
@drunkirishcoder yeah. that's fine. I guess more so for setting a good example for adding new types in the future (at least for ones more manageable than timeexceeded etc..., especially b/c many sites now have various packet examples. No biggie otherwise.
| /// | ||
| /// Not all code values are applicable to all message types. This setter | ||
| /// does not perform any validation. It's the caller's responsibility to | ||
| /// ensure that the value provided follows the spec. |
Description
Rework the ICMP API for both v4 and v6. This change makes ICMP extendable from outside the
Capsulecrate. Users can implement custom messages and work with them using the same public API as provided messages.Type of change
Checklist