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

IGMP support #53

Closed
wants to merge 3 commits into from
Closed

IGMP support #53

wants to merge 3 commits into from

Conversation

podhrmic
Copy link
Contributor

related to #51

I am seeking review early as recommended so I can easily fix and move things around.

This daily snapshot contains:

  1. adds optional feature protocol-igmp so the multicast is optional
  2. starts a minimal implementation of IGMPv2 protocol (v3 should be a future expansion)

As mentioned, this is not a full PR as things are not working yet, but I am looking for feedback/suggestions!

Note: not functional, more like a daily snapshot of work. There is more to come.
@@ -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")]
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong doc comment.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Sorry, something went wrong.

src/phy/mod.rs Outdated
@@ -179,6 +179,7 @@ pub struct ChecksumCapabilities {
pub udpv4: Checksum,
pub tcpv4: Checksum,
pub icmpv4: Checksum,
pub igmp: Checksum,
Copy link
Contributor

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.

Sorry, something went wrong.

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong doc comment.

Copy link
Contributor Author

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].
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Without get_.

Copy link
Contributor Author

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] {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@podhrmic
Copy link
Contributor Author

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:

  1. test sending/receiving IGMP packets over Ethernet iface
  2. add application logic for subscribing/leaving groups

@whitequark let me know if you prefer daily snapshots or rather larger chunks of code.

@podhrmic
Copy link
Contributor Author

Closing for now, will reopen with more complete feature set.

@podhrmic podhrmic closed this Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants