-
Notifications
You must be signed in to change notification settings - Fork 447
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
IGMP support #53
IGMP support #53
Conversation
Note: not functional, more like a daily snapshot of work. There is more to come.
src/iface/ethernet.rs
Outdated
@@ -345,6 +349,10 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> { | |||
IpProtocol::Icmp => | |||
self.process_icmpv4(ipv4_repr, ip_payload), | |||
|
|||
#[cfg(feature = "protocol-igmp")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, fix indentation. We use spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
use wire::Ipv4Address; | ||
|
||
enum_with_unknown! { | ||
/// Internet protocol control message type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong doc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
@@ -0,0 +1,262 @@ | |||
/// Internet Group Management Protocol v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the file should be called igmpv2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
|
||
enum_with_unknown! { | ||
/// Internet protocol control message type. | ||
pub doc enum Igmpv2MessageType(u8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and when the file is called igmpv2
you can remove the obnoxious prefixes. See how the other protocols do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed to look like for ICMP packets
src/phy/mod.rs
Outdated
@@ -179,6 +179,7 @@ pub struct ChecksumCapabilities { | |||
pub udpv4: Checksum, | |||
pub tcpv4: Checksum, | |||
pub icmpv4: Checksum, | |||
pub igmp: Checksum, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any NIC that does IGMP checksum offload. Even ICMP was a surprise to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
|
||
|
||
impl<T: AsRef<[u8]>> Packet<T> { | ||
/// Imbue a raw octet buffer with ICMPv4 packet structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong doc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
/// Ensure that no accessor method will panic if called. | ||
/// Returns `Err(Error::Truncated)` if the buffer is too short. | ||
/// | ||
/// The result of this check is invalidated by calling [set_header_len]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header length is fixed, so there's nothing to invalidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
|
||
/// Return the header length. | ||
/// The result depends on the value of the message type field. | ||
pub fn header_len(&self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
|
||
/// Return the Max reponse time | ||
#[inline] | ||
pub fn get_max_resp_time(&self) -> u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without get_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/wire/igmp.rs
Outdated
impl<'a, T: AsRef<[u8]> + ?Sized> Packet<&'a T> { | ||
/// Return a pointer to the type-specific data. | ||
#[inline] | ||
pub fn data(&self) -> &'a [u8] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No data in IGMP packets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
OK, adding another daily snapshot. Added parse/emit functions and simple tests to verify that parsing/creating IGMP packets works. The next step is to:
@whitequark let me know if you prefer daily snapshots or rather larger chunks of code. |
Closing for now, will reopen with more complete feature set. |
related to #51
I am seeking review early as recommended so I can easily fix and move things around.
This daily snapshot contains:
protocol-igmp
so the multicast is optionalAs mentioned, this is not a full PR as things are not working yet, but I am looking for feedback/suggestions!