JulianSchmid / etherparse

A rust library for parsing ethernet & ethernet using protocols.
Apache License 2.0
286 stars 54 forks source link

Replace `IpNumber` & `EtherType` with structs #51

Closed JulianSchmid closed 1 year ago

JulianSchmid commented 1 year ago

Task

Rewrite EtherType & IpNumber to be single value structs. E.g.:

struct EtherType(pub u16);

Also add implementations of the From & Into traits for simply converting from u8/u16 into the struct type and from the struct type into u8/u16. Also manually implement the Debug and Display to also display the human readable name if available (e.g. IPv4(0x0800) for EtherType(0x0800)).

Why

Both EtherType and IpNumber are currently implemented as simple enums. E.g.:

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum EtherType {
    Ipv4 = 0x0800,
    Ipv6 = 0x86dd,
    // ...
}

Originally the plan was to use the enums in the headers. But this approach was discarded in early versions of the library as it would limit the values to only the ones present in the enums. Additionally adding an Unknown(u8)value to the enums would disabled the ability to simply cast the values to their underlying type and added unneeded computation when converting from and into the enum type. So the headers were switched back to the underlying types u8 & u16 to support all valid IP & Ethernet headers.

But some time ago I noticed that that the pcap crate had implemented the Linktype as struct with a simple value ( https://docs.rs/pcap/1.0.0/pcap/struct.Linktype.html ). To me this seems the more sensible approach for EtherType & IpNumber as well, as it comes with the following advantages:

0xcrust commented 1 year ago

Hello! I noticed that a major refactor is in the works as per PR #47. Would it be fine for me to work on this now or is is it advisable to wait so that this PR(which comes with a lot of changes) lands.

JulianSchmid commented 1 year ago

Hi @0xcrust

I would wait until I at least have merged it to master. I will try to at least merge a partial state this weekend.

Greets Julian

JulianSchmid commented 1 year ago

Hi @0xcrust ,

I merged https://github.com/JulianSchmid/etherparse/pull/47 to master. There will be additional changes, but no where near to the same extend.

Greets Julian

0xcrust commented 1 year ago

Great! I'd like to go ahead with this then if that's fine

JulianSchmid commented 1 year ago

That would be great 😀