sys::socket listen's Backlog wrapper type addition. #2276

devnexen · 2023-12-28T20:18:36Z

changing the sys::socket::listen backlog type from usize to a i32 wrapper, offering known sane values, from -1, SOMAXCONN to
511.

close gh-2264

SteveLauC

Thanks for this PR! I have left some comments. BTW, I would appreciate it if we can have doc comments for these new public types.

A changelog is also needed.

SteveLauC · 2023-12-29T11:10:05Z

src/sys/socket/mod.rs

+impl Backlog {
+    /// Sets the listen queue size to system `SOMAXCONN` value
+    #[cfg(target_os = "redox")]
+    pub const DEFAULT: Self = Self(128);


Redox has SOMAXCONN defined, we should use it:

$ pwd /home/steve/Documents/workspace/libcs/libc/src/unix/redox $ rg "SOMAXCONN" mod.rs 816:pub const SOMAXCONN: ::c_int = 128;

I got an error on CI we might just need to update libc version again (I m the one adding the constant in this crate for redox).

SteveLauC · 2023-12-29T11:15:32Z

src/sys/socket/mod.rs

+    #[cfg(target_os = "redox")]
+    pub const DEFAULT: Self = Self(128);
+    #[cfg(not(target_os = "redox"))]
+    pub const DEFAULT: Self = Self(libc::SOMAXCONN);


Why are we calling this value DEFAULT? But not MAX or simply SOMAXCONN

It is mostly what people use we can call it differently sure.

SteveLauC · 2023-12-29T11:23:18Z

src/sys/socket/mod.rs

+    /// Reasonable upper limit, while in theory we might be able
+    /// to set it to a higher boundary, comes with a risk
+    /// of resource exhaustion in return
+    pub const MAXVALUE: Self = Self(511);


Is it really necessary to expose a MAXVALUE that is specified by us rather than the system?

We might refine it a bit (e.g. linux/glibc which has SOMAXCONN to 4096, for this case we might go up to 65535).

IMHO, we should not put a restriction that the OS does not have, so I kinda tend to remove this and use SOMAXCONN as the upper bound

SteveLauC · 2023-12-29T11:24:50Z

src/sys/socket/mod.rs

+    fn try_from(backlog: i64) -> std::result::Result<Self, Self::Error> {
+        match backlog {
+            ..=-2 => Err(BacklogTryFromError::TooNegative),
+            -1..=511 => Ok(Self(i32::try_from(backlog).map_err(|_| BacklogTryFromError::TooPositive)?)),


If a backlog of value -1 is only valid on Linux and FreeBSD, why are we allowing it for other OSes here?

sure. still a draft :) just wanted first impressions.

SteveLauC · 2023-12-30T05:52:55Z

src/sys/socket/mod.rs

+impl From<u16> for Backlog {
+    fn from(backlog: u16) -> Self {
+        Self(i32::from(backlog))
+    }
+}
+
+impl From<u8> for Backlog {
+    fn from(backlog: u8) -> Self {
+        Self(i32::from(backlog))
+    }
+}


Just realized that we need to check the value even the value is in range of i32, so what about defining a new(val: i32) function and remove all the helper functions (TryFrom/From) for other types:

/// The `backlog` argument of [`listen()`]. pub struct Backlog(i32); impl Backlog { /// Sets the listen queue size to system `SOMAXCONN` value pub const MAXCONN: Backlog = Backlog(libc::SOMAXCONN); /// Sets the listen queue size to -1 for system supporting it #[cfg(any(target_os = "linux", target_os = "freebsd"))] pub const MAXALLOWABLE: Backlog = Backlog(-1); /// Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid. pub fn new(val: i32) -> Result<Self> { cfg_if! { if #[cfg(any(target_os = "linux", target_os = "freebsd"))] { const MIN: i32 = -1; } else { const MIN: i32 = 0; } } if !(MIN..libc::SOMAXCONN).contains(&val) { return Err(Errno::EINVAL); } Ok(Self(val)) } }

If we want to be generic, we can also do new<I: Into<i32>>(val: I).

SteveLauC · 2023-12-30T08:20:28Z

src/sys/socket/mod.rs

+    pub const MAXALLOWABLE: Self = Self(-1);
+
+    // Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.
+    pub fn new<I: Into<i32> + PartialOrd<I> + From<i32>>(val: I) -> Result<Self> {


If I is Into<i32>, why not just cast it to i32 and do the comparison:

let val = val.into();

And, with this generic function, the other From/TryFrom impls are no longer needed, they are making things overly complicated.

SteveLauC · 2023-12-30T09:03:05Z

src/sys/socket/mod.rs

+    #[cfg(any(target_os = "linux", target_os = "freebsd"))]
+    pub const MAXALLOWABLE: Self = Self(-1);
+
+    // Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.


Please use a doc comment:

Suggested change

// Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.

/// Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.

SteveLauC · 2023-12-30T09:04:21Z

src/sys/socket/mod.rs

 /// Listen for connections on a socket
 ///
 /// [Further reading](https://pubs.opengroup.org/onlinepubs/9699919799/functions/listen.html)
-pub fn listen<F: AsFd>(sock: &F, backlog: usize) -> Result<()> {
+pub fn listen<F: AsFd, B: Into<Backlog>>(sock: &F, backlog: B) -> Result<()> {


With the current impl, we can just use the Backlog type here:

Suggested change

pub fn listen<F: AsFd, B: Into<Backlog>>(sock: &F, backlog: B) -> Result<()> {

pub fn listen<F: AsFd>(sock: &F, backlog: Backlog) -> Result<()> {

SteveLauC · 2023-12-30T09:05:22Z

src/sys/socket/mod.rs

    let fd = sock.as_fd().as_raw_fd();
-    let res = unsafe { libc::listen(fd, backlog as c_int) };
+    let res = unsafe { libc::listen(fd, i32::from(backlog.into())) };


Backlog is Into<i32>, so we can simply do this:

Suggested change

let res = unsafe { libc::listen(fd, i32::from(backlog.into())) };

let res = unsafe { libc::listen(fd, backlog.into()) };

SteveLauC · 2023-12-30T09:07:01Z

test/sys/test_socket.rs

+    assert!(Backlog::new(5012).is_err());
+    assert!(Backlog::new(65535).is_err());


For these 2 assertions, why not use the libc::SOMAXCONN, like this:

assert!(Backlog::new(libc::SOMAXCONN + 1).is_err());

changing the sys::socket::listen backlog type from `usize` to a `i32` wrapper, offering known sane values, from -1/0, to SOMAXCONN. close nix-rustgh-2264

SteveLauC

Thanks!

devnexen force-pushed the negative_backlog branch 8 times, most recently from 3e2735c to 6158e15 Compare December 29, 2023 07:32

SteveLauC requested changes Dec 29, 2023

View reviewed changes

devnexen force-pushed the negative_backlog branch 3 times, most recently from c34b1d7 to b8630c3 Compare December 29, 2023 15:05

devnexen marked this pull request as ready for review December 29, 2023 15:06

SteveLauC reviewed Dec 30, 2023

View reviewed changes

devnexen force-pushed the negative_backlog branch 2 times, most recently from 5064960 to 792b568 Compare December 30, 2023 07:27

SteveLauC reviewed Dec 30, 2023

View reviewed changes

devnexen force-pushed the negative_backlog branch from 792b568 to fbb37d1 Compare December 30, 2023 08:34

SteveLauC requested changes Dec 30, 2023

View reviewed changes

sys::socket listen's Backlog wrapper type addition.

d8e7cbe

changing the sys::socket::listen backlog type from `usize` to a `i32` wrapper, offering known sane values, from -1/0, to SOMAXCONN. close nix-rustgh-2264

devnexen force-pushed the negative_backlog branch from fbb37d1 to d8e7cbe Compare December 30, 2023 09:26

SteveLauC approved these changes Dec 30, 2023

View reviewed changes

SteveLauC added this pull request to the merge queue Dec 30, 2023

Merged via the queue into nix-rust:master with commit d2445bf Dec 30, 2023
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys::socket listen's Backlog wrapper type addition. #2276

sys::socket listen's Backlog wrapper type addition. #2276

devnexen commented Dec 28, 2023

SteveLauC left a comment

SteveLauC Dec 29, 2023

devnexen Dec 29, 2023

SteveLauC Dec 29, 2023

devnexen Dec 29, 2023

SteveLauC Dec 29, 2023

devnexen Dec 29, 2023

SteveLauC Dec 29, 2023

SteveLauC Dec 29, 2023

devnexen Dec 29, 2023

SteveLauC Dec 30, 2023 •

edited

Loading

SteveLauC Dec 30, 2023

SteveLauC Dec 30, 2023

SteveLauC Dec 30, 2023

SteveLauC Dec 30, 2023

SteveLauC Dec 30, 2023

SteveLauC left a comment

	// Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.
	/// Create a `Backlog`, an `EINVAL` will be returned if `val` is invalid.

	pub fn listen<F: AsFd, B: Into<Backlog>>(sock: &F, backlog: B) -> Result<()> {
	pub fn listen<F: AsFd>(sock: &F, backlog: Backlog) -> Result<()> {

	let res = unsafe { libc::listen(fd, i32::from(backlog.into())) };
	let res = unsafe { libc::listen(fd, backlog.into()) };

		assert!(Backlog::new(5012).is_err());
		assert!(Backlog::new(65535).is_err());

sys::socket listen's Backlog wrapper type addition. #2276

sys::socket listen's Backlog wrapper type addition. #2276

Conversation

devnexen commented Dec 28, 2023

SteveLauC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC left a comment

Choose a reason for hiding this comment

SteveLauC Dec 30, 2023 •

edited

Loading