JulianSchmid / etherparse

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

More specific error types #16

Closed not-my-profile closed 1 year ago

not-my-profile commented 2 years ago

Hey, thanks for your library :) I want to use the manual slicing methods but noticed that they all return the ReadError enum, which contains variants that cannot actually occur.

For example with Ethernet2HeaderSlice.from_slice the only error variant that can actually occur is UnexpectedEndOfSlice.

Would you accept a PR changing ReadError to something like the following?

pub enum ReadError {
    IoError(Error),
    UnexpectedEndOfSlice(UnexpectedEndOfSliceError),
    DoubleVlan(DoubleVlanError),
    IpUnsupportedVersion(u8),
    Ipv4(Ipv4Error),
    Ipv6(Ipv6Error),
    IpAuthenticationHeader(IpAuthenticationHeaderError),
    Tcp(TcpError),
}
JulianSchmid commented 2 years ago

Hi @not-my-profile,

That is a good idea. We could even add a From implementation to enable auto conversion to read error to be backwards compatible.

Greets Julian

JulianSchmid commented 2 years ago

Would you accept a PR changing ReadError to something like the following?

Yes of course. But I can not guarantee you how fast I can merge it. Last time one week escalated into 2 years, but I don't think this will happen with this :) .

Greets Julian

not-my-profile commented 2 years ago

Thanks, for the quick answer :) Could you format your code with cargo fmt beforehand?

JulianSchmid commented 2 years ago

I decided agains using rust fmt in this library as it made the testcases where raw bytes are used harder to read (e.g. https://github.com/JulianSchmid/etherparse/blob/36f8f7f202772df468346f7aa671657371e87241/tests/transport/tcp.rs#L1473 ).

not-my-profile commented 2 years ago

Ah yeah ... adding #[rustfmt::skip] everywhere is cumbersome and #![rustfmt::skip] doesn't work on stable yet.

JulianSchmid commented 2 years ago

Hi, I had a closer look at the error types and I will probably restructure them quite a bit in the next release. Specifically also splitting them up based on the operation: read, from_slice, write, to_slice. At the same time I will try to split the error types so that an operation only returns an error type with values it can actually trigger.

That should hopefully also resolve your request, but will allow me to separate std::io::Error from the rest. std::io::Error has always been a bit of a thorn in my side, as it does not implement Eq or PartialEq (which makes writing tests a bit more verbose). Additionally std::io::Error is not available when building with no_std, which I really would like to support.

JulianSchmid commented 1 year ago

Finalized by https://github.com/JulianSchmid/etherparse/pull/64