Skip to content

Conversation

@aadilshabier
Copy link
Contributor

No description provided.

- Implement checksum calculation for IPv4 header.
- Replace TOS header field with the new DSCP and ECN fields as directed
  in RFC 2474 and RFC 3168.

Signed-off-by: Mohammad Aadil Shabier <[email protected]>
Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

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

Mostly looks okay - I have not looked at the ipv4 checksum algorithm. Just assuming it's correct. Probably a good idea to add some test case for that.

Few other small changes.

This will be used when sculpting to select an EtherType based on the
upcoming layer.

Signed-off-by: Mohammad Aadil Shabier <[email protected]>
@aadilshabier aadilshabier changed the title Implement IPv4 checksum and use inverse maps for ethernet scultping Implement IPv4 checksum and use inverse maps for ethernet sculpting Jul 25, 2024
@aadilshabier aadilshabier marked this pull request as ready for review July 29, 2024 14:26
@gabhijit
Copy link
Collaborator

@aadilshabier: Sorry - This somehow got missed! Will take a look at it sometime soon (this week).

Copy link
Collaborator

@gabhijit gabhijit left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks: but are useful to get it right, so that we can merge it.

// Register outselves with Ethernet layer
pub(crate) fn register_defaults() -> Result<(), Error> {
ethernet::register_ethertype(ETHERTYPE_ARP, ARP::creator)
let name = Some(ARP::default().name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use short_name (short_name is convenient and usually all lowercase)

ETHERTYPES_MAP.get_or_init(|| RwLock::new(HashMap::new()))
}

/// A Map maintaining String -> EtherType of L3 Layers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more description for the map as a documentation. If we simply read this documentation it is not clear why this map exists and how it is to be used. For example something like this.

/// This map will be used during sculpting a packet. For example for IPv4 you the entry
/// in the map will look like `ip -> 0x0800`

}

/// A Map maintaining String -> EtherType of L3 Layers.
pub fn get_inv_ethertypes_map() -> &'static RwLock<HashMap<String, EtherType>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be this function is gated by cfg(packet_sculpting) ? Because this map and API is not used outside sculpting.


if let Some(name) = name {
let mut inv_map = get_inv_ethertypes_map().write().unwrap();
if inv_map.contains_key(name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if inv_map.contains_key(&name.to_lowercase())

let mut inv_map = get_inv_ethertypes_map().write().unwrap();
if inv_map.contains_key(name) {
return Err(Error::RegisterError(format!(
"Cannot find EtherType for : {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error should be Already Exists and not cannot find. Also Better to let the user know what value exists in the inv_ map. So may be get and check for Some value?


register_ethertype(crate::types::ETHERTYPE_IP, IPv4::creator)?;

let name = Some(IPv4::default().name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

short_name as above in ARP.


get_next_headers_map();
register_ethertype(crate::types::ETHERTYPE_IP6, IPv6::creator)?;
let name = Some(IPv6::default().name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

short_name

pub(crate) fn register_defaults() -> Result<(), Error> {
ethernet::register_ethertype(ETHERTYPE_MPLS_UNICAST, MPLS::creator)?;
ethernet::register_ethertype(ETHERTYPE_MPLS_MULTICAST, MPLS::creator)
let name = Some(MPLS::default().name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

short_name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants