Skip to content

Interface ip_addrs field from slice to heapless::Vec #719

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

Merged
merged 5 commits into from
Dec 22, 2022
Merged

Interface ip_addrs field from slice to heapless::Vec #719

merged 5 commits into from
Dec 22, 2022

Conversation

davidedellagiustina
Copy link
Contributor

As stated in the title.
One cargo test is still failing, needs to be looked into.

Moreover, for now the heapless::Vec used is fixed-size.

@MabezDev
Copy link
Contributor

Is there any particular reason you need/want this? What's wrong with the ManagedSlice method?

@Dirbaio
Copy link
Member

Dirbaio commented Dec 20, 2022

It's to fix #458 and some cases of #466.

The problem with ManagedSlice is that in the borrowed case it is fixed length, so you can't dynamically add/remove IPs. If you want to have "no IP" (for example for when DHCP is not up yet) you have to change it to 0.0.0.0 instead. This causes smoltcp to transmit garbage packets with source IP 0.0.0.0, because it treats it like any other IP in the egress code.

The alternative would be to change egress to treat 0.0.0.0 as "not a real IP", but that's an ungly workaround. The root issue is that the IP list should be variable length even in nostd.

(This probably applies to other ManagedSlice's in Interface as well, these'd be better off as Vecs. For example the routes and neighbor cache. We'll change them in future PRs)

@davidedellagiustina
Copy link
Contributor Author

Exactly what @Dirbaio said. I'm still trying to understand how to fix the failing test though.

@MabezDev
Copy link
Contributor

Thanks! I'm pretty sure smoltcp & managed predate heapless so It'll be nice to eventually use heapless instead and phase out managed. I'm sure it would help for #655.

@davidedellagiustina
Copy link
Contributor Author

davidedellagiustina commented Dec 20, 2022

Before forgetting, there is also an additional point that I briefly discussed with @Dirbaio in private: for now, I used a hardcoded constant as the maximum size of the ip_addrs vector, but this might restrict some use cases. We agreed on the fact that using a generic parameter would pollute the API in an undesirable way, and probably the best way would be adding some cargo features to allow for different (though predefined) sizes for such vector. But any further idea is welcome.

@davidedellagiustina davidedellagiustina marked this pull request as ready for review December 20, 2022 14:55
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Yes, we can work on making the max count customizable in the future.

@Dirbaio
Copy link
Member

Dirbaio commented Dec 21, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 22, 2022

Build succeeded:

  • clippy
  • fmt
  • fuzz
  • tests

@bors bors bot merged commit 3455acb into smoltcp-rs:master Dec 22, 2022
@Dirbaio Dirbaio mentioned this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants