Thoughts on how to handle Arp parsing?

[robs-zeynet]

Hi @JulianSchmid - thanks again for the help in getting ICMP{v4,v6} landed. Now I'm volunteering to write Arp support as well, but I'd prefer to do it in a way that you're more likely to accept (and ideally is less work for you).

Good news, there's already an Arp enum.

In theory I could add some additional parsing code at https://github.com/JulianSchmid/etherparse/blob/master/etherparse/src/packet_decoder.rs#L255 and just extend InternetSlice and InternetHeader to add the Arp variant. It would be a breaking change for a lot of people (including my code!) but probably the best way to do it.

https://github.com/JulianSchmid/etherparse/blob/master/etherparse/src/internet/internet_slice.rs#L7

Should I write this up or do you have any other concerns that would need to be addressed at the design phase?

[JulianSchmid]

Hi @robs-zeynet ,

I am torn on how to integrate ARP.

Originally I also thought of just adding it to InternetSlice as it makes sense from a "it is identified by an ether_type number and bellow VLAN & co". But after reworking the slicing and also including the payloads together with the layers I am less sure about that. It kind of makes it harder to get the ip payload as methods "payload()" then have to return a Option<&IpPayloadSlice<'a>> instead of &IpPayloadSlice<'a>. From a user perspective it might be simpler to have a field to check for IP and a separate field to check for ARP and other protocols that get identified by "ether_type".

So currently I think a new field should be added to SlicedPacket & PacketDecoder for ARP and potentially other protocols on that layer. It could also double to grant easy access to unknown EtherType protocols. But it is nothing I am 100% sure about.

Also I would advise you to work based on #71 (lax branch https://github.com/JulianSchmid/etherparse/tree/lax ) as it changes quiet a lot. I will try to merge a partial state this week to master.

Greets
Julian

[JulianSchmid]

Man I am really going back and forth. Now I am thinking we should just add a new enum that wraps ARP and IP and should still put it into the same field. E.g.:

enum IpAlike {
    Ip(IpSlice),
    Arp(ArpSlice),
}

But I cannot think for a good name for the enum. If you have any ideas I am all ears.

[robs-zeynet]

So a couple of thoughts:

1. For naming, I'd suggest we follow the OSI model (https://en.wikipedia.org/wiki/OSI_model) which we already are for 'link' and 'transport' but not for this - that would say call it 'network' or 'network_layer' and be explicit about modeling the OSI

2. I thought about both of your suggestions (separate field 'arp' or nested in some new field, e.g., 'IpAlike') before submitting and IMHO I think keeping it all together in one enum still makes more sense. When thinking about API design, I always think about how the users will have to interact with them and a single flat enum seems to make matches easier:

Example #1: with a different top-level field (e.g., 'arp'), it makes the packet matching more complicated. Let's take your helloworld code from the README.md and see what changes would be needed:

// if we went with the 'arp is separate field
match SlicedPacket::from_ethernet(&packet) {
    Err(value) => println!("Err {:?}", value),
    Ok(value) => {
        println!("link: {:?}", value.link);
        println!("vlan: {:?}", value.vlan);
        // println!("ip: {:?}", value.ip);  // old code
        if value.arp.is_some() {    // can't be both Arp and IP so need to pick one
              println!("arp: {:?}", value.arp);
        } else if value.ip.is_some() {
              println!("ip: {:?}", value.ip);
        }
        println!("transport: {:?}", value.transport);
    }
}

or alternately, if we went with sub-enums, the hello-world code wouldn't change but matching would:

match SlicedPacket::from_ethernet(&packet)?.ip_alike {  // field name TBD
    Arp(a) => handle_arp_pkt(a),
    Ip(ip) => match ip {                         // I find this nested match structure unwieldly 
         Ipv4(ip4) => handle_v4(ip4),
         Ipv6(ip6) => handle_v6(ip6),
    }
}

Where I think everything in one flat, renamed structure reads/works best:

match SlicedPacket::from_ethernet(&packet)?.network_layer {  // field name TBD
    Arp(a) => handle_arp_pkt(a),
    Ipv4(ip4) => handle_v4(ip4),
    Ipv6(ip6) => handle_v6(ip6),
}

Thoughts?

FYI: I've written the code for my own library already so hopefully it's mostly a matter of porting it into etherparse (which I'm happy to do - great project).

Thanks for the discussion!

[robs-zeynet]

Separately, lots of thanks for working on #71 the lax support. I've actually internally forked etherparse until that lands because all of my code requires lax parsing (e.g., parsing embedded packets in side ICMP TTL Exceeded pkts).. Thank you!

[JulianSchmid]

I like the the idea of calling it "network" or "net" and I also come around to doing it in one enum (again).

The one thing I still wanted was a quick way to get to the IP payload but that could be done by:

using etherparse::NetSlice::*;

match SlicedPacket::from_ethernet(&packet)?.net.map(|v| v.ip_payload()).flatten() {
    Some(ip_payload) => {},
    None => {},
}

So go ahead, I would ask you to

• Rename SlicedPacket::ip to SlicedPacket::net (see next comment)
• Rename IpSlice (previously InternetSlice) to NetSlice (see next comment)
• Adding arp to the NetSlice,``LaxNetSlice & NetHeaders

[JulianSchmid]

Ok slight change of plan. After having another look I would like to keep IpSlice and IpHeaders around, because they are really useful while parsing. But I added the new enums NetSlice LaxNetSlice & NetHeaders to the lax branch and switched PacketHeaders & SlicedPacket to use those in their net fields (previously ip).

I just pushed the changes to the lax branch so you can start working on ARP parsing if you want to.

Also the lax branch is now back to a completely building state and the tests are all passing. But there is still stuff missing.

[JulianSchmid]

Hi @robs-zeynet ,

I just released https://github.com/JulianSchmid/etherparse/releases/tag/v0.14.0 with lax parsing and ip renamed to net. I will not touch that part of the code for a while. If you still are up for implementing ARP now would be a good moment.

Greets & 1000 Thanks
Julian

[JulianSchmid]

ARP Support was added with #103 and released with https://github.com/JulianSchmid/etherparse/releases/tag/v0.17.0 .