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

#487 Add ways to set Path Discovery MTU for Linux on a Socket. #515

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

darnuria
Copy link

@darnuria darnuria commented Jun 9, 2024

Hello first contribution here! It may need some careful review since MTU management is a bit complicated and not so portable.

Add ways to set Path Discovery MTU for Linux on a Socket. (it may not work on freebsd EDIT: CI confirmed it! but may work on Windows subsystem and winsock: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options.

This contribution use setsocketops to configure Dont Fragment (DF) bit and OS
behavior related to PMTU.

It introduce PathMtuDiscoveringMode enum to manage it rust-way and 4 functions
respectively for ipv4/v6 and set/get.

Omit and Interface exist but are not widely documented.

Initially It started by preparing a PR for the patch purposed here: #487

Context: Why it can be needed in a rust library? Some protocols such as DNS in UDP or traceroute needs to manage the MTU on a socket level.

Exemples:

This contribution use setsocketops to configure Dont Fragment (DF) bit and OS
behavior related to PMTU.

It introduce PathMtuDiscoveringMode enum to manage it rust-way and 4 functions
respectively for ipv4/v6 and set/get.

Omit and Interface exist but are not widely documented.
src/sys/unix.rs Outdated
#[cfg_attr(docsrs, doc(cfg(all(feature = "all", target_os = "linux"))))]
#[repr(C)]
#[derive(Debug)]
pub enum PathMtuDiscoveringMode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this enum.

Maybe it's better as a type alias with constants for the options? This way we don't have to do the conversion and we don't have to deal with the case where the kernel returns a value we don't support yet.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I agree, I may do a patch to the linux man page since some are not explicitly documented outside of the in.h/in6.h

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it's better as a type alias with constants for the options? This way we don't have to do the conversion and we don't have to deal with the case where the kernel returns a value we don't support yet.

I reused the pattern used for Type and Protocol code tell me if wasn't the best move. :)

src/sys/unix.rs Outdated Show resolved Hide resolved
@darnuria
Copy link
Author

Hello, thanks for the review, will do the changes today.

darnuria and others added 4 commits June 19, 2024 14:28
Co-authored-by: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
I reused the pattern used elsewhere to have handful short hands documented,
while yet permiting futur constant not exposed in the lib.

Also expose MtuDiscovering Mode types..

#[cfg(all(feature = "all", target_os = "linux"))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "all", target_os = "linux"))))]
impl PathMtuDiscoveringModeV6 {
Copy link
Author

Choose a reason for hiding this comment

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

It involve some code duplication alas but it's code that... never change except for adding new mode since API don't change for Linux.

@darnuria darnuria marked this pull request as ready for review June 19, 2024 19:21
@Thomasdezeeuw
Copy link
Collaborator

Sorry @darnuria I didn't have time to review this last weekend and I don't think I'll have time the next two weeks either, so it's going to take a little while.

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

Successfully merging this pull request may close these issues.

2 participants