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

Expose the packet parsing and crafting infrastructure as a separate crate #811

Open
HKalbasi opened this issue Jul 5, 2023 · 5 comments

Comments

@HKalbasi
Copy link
Contributor

HKalbasi commented Jul 5, 2023

Rust ecosystem lacks a mature packet parsing and crafting library similar to libtins in C++, the best I found is the packet crate which lacks some basic functionality like ARP packets (if a good library existed, I guess this crate would probably use it).

This crate has a great in house implementation of such functionality, which is zero copy, handles corner cases, is well maintained, ... . Would you accept a PR to exposing it as a separate crate, e.g. smolpackets? Currently most (all?) of that functionality is private, which is reasonable since it is not the intended job of the smoltcp crate. The new crate can still live in this repository, and I would expect no stability guarantees on it (it can even be published with the version of the smoltcp crate or in some auto release fashion) but it will still probably increase the maintenance cost a little.

@thvdveld
Copy link
Contributor

thvdveld commented Jul 6, 2023

This idea came up more than once (matrix room of smoltcp). So I was working on this as well! I have a branch that is kind of ready. I'll create a pull request this afternoon.

@thvdveld
Copy link
Contributor

thvdveld commented Jul 6, 2023

I discussed this a bit in the matrix room. The thing is that it would be difficult to maintain it. People might want to merge exotic protocols, however, these protocols might not fit the use case of smoltcp and there is not really someone that wants to commit to maintain it.

Then there is a second problem: smoltcp has a lot of feature flags. With these feature flags, we can keep the memory footprint low, since I think most of the people use it for embedded. If we have two crates, then smolwire could enable proto-ipv6 and proto-ipv4, and smoltcp might only enable proto-ipv6, which will cause compile errors.

What is your use case? Couldn't you just use smoltcp::wire directly?

@HKalbasi
Copy link
Contributor Author

HKalbasi commented Jul 6, 2023

So it seems what I need is basically the smoltcp::wire. I thought I need smoltcp::iface::ip_packet::IpPayload and friends but it looks like those are thin internal wrappers over smoltcp::wire.

I still think separate crate makes sense and #812 is in the right direction. About the feature flags problem, from what I see, the only problem would be feature gated enum variants which disappear in pattern matching. Since smoltcp depends on smolpackets it can enable the corresponding features of it so feature flags of smoltcp will always be a subset of feature flags of smolpackets, and since those enums are marked (or should be marked as) non exhaustive, there would be no compile time error since there should always be a _ pattern there. Even if someone hits some compile error due mismatching in feature flags of these crates, they can easily solve the problem by depending directly on smoltcp and adding the missing feature flags.

The maintaining concern is valid, though you can always reject anything that you see undesirable for the smoltcp goals, and exotic old protocols are stable so there wouldn't be that much of maintaining costs (exotic new protocols should be unconditionally rejected by this logic). I personally don't need any exotic protocol and I think smoltcp::wire already satisfies my needs.

@Dirbaio
Copy link
Member

Dirbaio commented Jul 6, 2023

there would be no compile time error since there should always be a _ pattern there

there's not always a _, for example here

match address {
#[cfg(feature = "proto-ipv4")]
IpAddress::Ipv4(address) => self.is_broadcast_v4(*address),
#[cfg(feature = "proto-ipv6")]
IpAddress::Ipv6(_) => false,
}

forcing adding _ to everywhere is a bit ugly

@HKalbasi
Copy link
Contributor Author

HKalbasi commented Jul 6, 2023

Cargo documentation recommends that enabling a feature flag should be a semver compatible change and adding variant to an exhaustive enum is a breaking change so smoltcp is already committing cargo crime and cargo police might catch us :) (that is, in a theoretical scenario, some library can depend on smoltcp with feature proto-ipv4 and breaks if the root crate enables the other feature) and the situation will be worse if we switch to the multi crate setup. I see three solutions:

  1. Make IpAddress and similar enums #[non_exhaustive]: Ugly as you said, but makes the cargo police happy and is minimal change relative to the current version.
  2. Make IpAddress variants always enable, or use core::net::IpAddr: Special care needed in this case to not create unnecessary bloat, but I believe it is possible. Enum variants on their own don't generate any assembly, functions of them do, and we should have some care to use Ipv4Addr when possible (IpAddr is unconditionally 17 bytes but Ipv4Addr is 4 bytes). It also won't save us from become forcing to add _ match arms in some cases.
  3. Keep things as is. Most users will use one of the smoltcp or smolpackets crates and the root crate always can fix such compile errors, so it wouldn't be end of the world.

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

No branches or pull requests

3 participants