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

[ACP] adding todevice call to std::net #432

Closed
devnexen opened this issue Aug 22, 2024 · 4 comments
Closed

[ACP] adding todevice call to std::net #432

devnexen opened this issue Aug 22, 2024 · 4 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@devnexen
Copy link

Proposal

Problem statement

Giving the possibility, for a socket, to be bound to a particular network interface.

Motivating examples or use cases

It can serve for filtering purpose, e.g. external incoming requests for eth1 then separate private requests for eth0 ... or based on particular type of services e.g. http only.

Tracking issue and the PR.

@devnexen devnexen added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Aug 22, 2024
@kennytm
Copy link
Member

kennytm commented Aug 22, 2024

regarding the PR already sent

pub fn todevice(&self) -> io::Result<&CStr>;
pub fn set_todevice(&self, ifrname: &CStr) -> io::Result<()>;
  1. Why the setter takes a &CStr rather than a &str or &OsStr?

  2. As mentioned in the PR the getter cannot just return a reference, it must return an owned type (CString/OsString/String)

  3. The socket option is called SO_BINDTODEVICE, and "todevice" isn't really the specific term here. IMO it makes more sense to just name the functions device()/set_device(), or call the setter bind_to_device(). For reference, in socket2 the functions are named:

    pub fn device(&self) -> io::Result<Option<Vec<u8>>>;
    pub fn bind_device(&self, interface: Option<&[u8]>) -> io::Result<()>;

    (I'm not sure sure why these aren't showing up in docs.rs)

@lolbinarycat
Copy link

seconding the concerns about naming.

todevice is not ideal for several reasons:

  • rust method names are almost always snake_case, this has no word separation, and is harder to read (tode vice? tod ev ice?).
  • the verb to at the start of a method name is usually reserved for methods that perform some kind of conversion
  • the verb to could also impl sending packets to a device, while this would often be used to filter what packets are recieved from a device.

@devnexen
Copy link
Author

Ok for the naming changes. The setter already returned CString since first feedbacks but will change the rest. Thanks

@the8472
Copy link
Member

the8472 commented Sep 3, 2024

Since binding has to happen on an unconnected/unbound socket we first need a way to do this in std, which is a bigger story (possibly requiring an RFC) since it means adopting a chunk of the socket2 crate.

So this ACP has to be postponed until that prerequisite is fulfilled.

@the8472 the8472 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants