Skip to content

Conversation

thvdveld
Copy link
Contributor

@thvdveld thvdveld commented Oct 7, 2025

This allows to bind the ICMP socket to a TCP port as well, in order to receive ICMP errors associated to TCP packets sent from that port.

Using the ICMP socket, the TCP socket could be reset when receiving an IMCP error related to that socket (#1071).

This allows to bind the ICMP socket to a TCP port as well, in order to
receive ICMP errors associated to TCP packets sent from that port.

Signed-off-by: Thibaut Vandervelden <[email protected]>
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

❌ Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.36%. Comparing base (cfbfeae) to head (a0cf072).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/socket/icmp.rs 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
+ Coverage   80.34%   80.36%   +0.01%     
==========================================
  Files          81       81              
  Lines       24335    24486     +151     
==========================================
+ Hits        19552    19678     +126     
- Misses       4783     4808      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 7, 2025

I think it'd be better to make smoltcp itself handle these ICMP messages and reset the socket. Possibly behind a Cargo feature.

Requiring the user to set up the ICMP socket themselves is quite burdensome:

  • It makes the user set up the socket, bind it, manage its state, poll it for packets, then parse the packet themselves and act accordingly. It's quite a bit of code that will be essentially the same for all projects that want this.
  • you need one ICMP socket for each TCP socket you have, and each ICMP socket needs enough buffer space to hold one packet. It's quite a bit of wasted RAM.

@thvdveld
Copy link
Contributor Author

thvdveld commented Oct 7, 2025

I was thinking about doing it internally in smoltcp, but just quickly added this now such that if someone wants to do it using the ICMP socket, which we already allow for UDP, then they can do so with this PR.

When doing it internally, we shouldn't do it with a feature flag. It would result in a conflict when two crates depend on smoltcp, one using the feature flag and one not using the feature flag, and the latter actually depending on smoltcp not to handle the error and reset the socket.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 7, 2025

the latter actually depending on smoltcp not to handle the error and reset the socket.

This seems a bit unrealistic? in what use case would you want it to actively not handle the ICMP errors in a way that breaks if you do?

anyway, I was thinking of the Cargo feature mostly to avoid increased code size when you don't need it. We can make a Cargo feature and a flag you set in each socket indivudually.

@thvdveld
Copy link
Contributor Author

thvdveld commented Oct 7, 2025

A custom protocol might handle errors differently. Using a flag in the socket itself solves this. In any case, I totally agree we should do it internally.

I think this PR already provides some way of handling ICMP errors for TCP, not ideal, but allowing to do the same as for UDP. Doing it internally could be a follow up PR.

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.

2 participants