return RecvError::Truncated when calling recv_slice with a small buffer

[thvdveld]

When calling recv_slice/peek_slice in the udp, icmp and raw sockets, data is lost when the provided buffer is smaller than the payload to be copied (not the case for peek_slice because it is not dequeued).

With this commit, an RecvError::Truncated error is returned. In case of UDP, the endpoint is also returned in the error. In case of ICMP, the IP address is also returned in the error.

I implemented it the way @whitequark proposes it in #330. Data is still lost, but at least the caller knows that the data was truncated to the size of the provided buffer. As @whitequark says, it is preferred to call recv instead of recv_slice as there would be no truncation. If this is expected behaviour for recv_slice, we could close #330.

[codecov]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (a226d5b) 79.60% compared to head (1386417) 79.55%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/socket/raw.rs	44.44%	5 Missing ⚠️
src/socket/udp.rs	44.44%	5 Missing ⚠️
src/socket/icmp.rs	70.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #859      +/-   ##
==========================================
- Coverage   79.60%   79.55%   -0.06%     
==========================================
  Files          78       78              
  Lines       27896    27917      +21     
==========================================
+ Hits        22206    22208       +2     
- Misses       5690     5709      +19     

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

[whitequark]

I'm not sure I like the design where there are two different ways of receiving metadata from the socket. This certainly takes "convenience" out of "convenience method".

I am not entirely sure what to do about this, but I would personally be happy to have Error::Truncated mean "there was a packet in the socket that was too big, it is lost, and you can move on to the next packet if you are this sure you only care about smaller ones". Anyone else should be using recv, probably.

[thvdveld]

I was not sure about that part either. I'll remove the endpoint from the Error::Truncated.

[thvdveld]

here was a packet in the socket that was too big, it is lost

Do you mean that we also don't really need to copy part of the packet in the provided buffer and we should drop the full packet?

[whitequark]

Do you mean that we also don't really need to copy part of the packet in the provided buffer and we should drop the full packet?

Yep. These are convenience methods for the case where you're pretty certain you do not care at all about the bigger packets. If you sometimes want to process only the first part, or you want to execute some complex error handling code, they're not for you.

A note to that end in the docs may be useful.

[thvdveld]

I made the changes and added a note in the documentation of the functions.

Note that now the behaviour of these functions is different: no data is actually copied when the buffer is too small, whereas previously, data was still copied into the smaller buffer.

[whitequark]

Note that now the behaviour of these functions is different: no data is actually copied when the buffer is too small, whereas previously, data was still copied into the smaller buffer.

This is a breaking change regardless of how much data is copied though, isn't it?

[thvdveld]

I think only because of the addition of Truncated to the enums.

[thvdveld]

Do you think we can close #330 now?

[whitequark]

I think only because of the addition of Truncated to the enums.

I mean that returning a different error, with or without payload, in case an overlong packet is detected, is a breaking change. Whether or not the buffer is filled out with data, at that point, doesn't make it any more breaking.

[whitequark]

Do you think we can close #330 now?

Yeah.