Skip to content

Conversation

@onestay
Copy link
Contributor

@onestay onestay commented Sep 21, 2022

This pull request adds support for the DCCP protocol.
This included adding support for the DCCP Protocol and Type as well as adding a bunch of socket options. Not all socket options as listed on https://www.kernel.org/doc/html/latest/networking/dccp.html are implemented. DCCP_SOCKOPT_QPOLICY_ID is not added because I could not find the constants DCCPQ_POLICY_PRIO, DCCPQ_POLICY_SIMPLE in the libc crate.

DCCP_SOCKOPT_CCID_RX_INFO and DCCP_SOCKOPT_CCID_TX_INFO are also not added since they are CCID3 exclusive.

Setting different RX and TX CCIDs is also not supported. This is, in theory, allowed, however in practice very rarely used and not well understood.

I also wrote a simple test, testing most socket options, as well as creating a server and client and sending a message between the two.

@onestay onestay changed the title Adding DCCP support [WIP] Adding DCCP support Sep 21, 2022
@onestay onestay marked this pull request as draft September 21, 2022 14:38
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Initial review comments.

Note that you don't need to add all options in one go. It's perfectly fine to add them in follow up prs (or not at all ;) )

src/sys/unix.rs Outdated
&mut len,
))?;
let buf = &buf[..len as usize - 1];
Ok(unsafe { &*(buf as *const [_] as *const [u8]) }.into())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do better than allocating 10 bytes? Maybe a small vec like type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quiet sure what to do here. I've changed the buf to length 5 instead of 10. The code of this function was basically copied from the device function.

socket2/src/sys/unix.rs

Lines 1488 to 1507 in 4c43681

pub fn device(&self) -> io::Result<Option<Vec<u8>>> {
// TODO: replace with `MaybeUninit::uninit_array` once stable.
let mut buf: [MaybeUninit<u8>; libc::IFNAMSIZ] =
unsafe { MaybeUninit::uninit().assume_init() };
let mut len = buf.len() as libc::socklen_t;
syscall!(getsockopt(
self.as_raw(),
libc::SOL_SOCKET,
libc::SO_BINDTODEVICE,
buf.as_mut_ptr().cast(),
&mut len,
))?;
if len == 0 {
Ok(None)
} else {
let buf = &buf[..len as usize - 1];
// TODO: use `MaybeUninit::slice_assume_init_ref` once stable.
Ok(Some(unsafe { &*(buf as *const [_] as *const [u8]) }.into()))
}
}

@onestay onestay changed the title [WIP] Adding DCCP support Adding DCCP support Sep 22, 2022
@onestay onestay marked this pull request as ready for review September 22, 2022 16:38
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Can you follow our code conventions? That is

  • Start setters with set_, e.g. set_dccp_service.
  • Setters are before getters in the code.
  • Add the socket option used to the function docs and maybe add a quick 1 sentence description. It's ok if this comes from the original docs.
  • In the getter functions refer to the setter docs.

For example see

socket2/src/sys/unix.rs

Lines 1906 to 1968 in 4c43681

/// Set the value of the `TCP_USER_TIMEOUT` option on this socket.
///
/// If set, this specifies the maximum amount of time that transmitted data may remain
/// unacknowledged or buffered data may remain untransmitted before TCP will forcibly close the
/// corresponding connection.
///
/// Setting `timeout` to `None` or a zero duration causes the system default timeouts to
/// be used. If `timeout` in milliseconds is larger than `c_uint::MAX`, the timeout is clamped
/// to `c_uint::MAX`. For example, when `c_uint` is a 32-bit value, this limits the timeout to
/// approximately 49.71 days.
#[cfg(all(
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
#[cfg_attr(
docsrs,
doc(cfg(all(
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
)))
)]
pub fn set_tcp_user_timeout(&self, timeout: Option<Duration>) -> io::Result<()> {
let timeout = timeout.map_or(0, |to| {
min(to.as_millis(), libc::c_uint::MAX as u128) as libc::c_uint
});
unsafe {
setsockopt(
self.as_raw(),
libc::IPPROTO_TCP,
libc::TCP_USER_TIMEOUT,
timeout,
)
}
}
/// Get the value of the `TCP_USER_TIMEOUT` option on this socket.
///
/// For more information about this option, see [`set_tcp_user_timeout`].
///
/// [`set_tcp_user_timeout`]: Socket::set_tcp_user_timeout
#[cfg(all(
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
))]
#[cfg_attr(
docsrs,
doc(cfg(all(
feature = "all",
any(target_os = "android", target_os = "fuchsia", target_os = "linux")
)))
)]
pub fn tcp_user_timeout(&self) -> io::Result<Option<Duration>> {
unsafe {
getsockopt::<libc::c_uint>(self.as_raw(), libc::IPPROTO_TCP, libc::TCP_USER_TIMEOUT)
.map(|millis| {
if millis == 0 {
None
} else {
Some(Duration::from_millis(millis as u64))
}
})
}
}

Since DCCP is not enabled in all of the kernels of major linux distros
it is probably better to ignore the test by default
@Thomasdezeeuw
Copy link
Collaborator

Sorry @onestay I missed this one. I took a quick glance and it looks pretty good. Can you rebase this on master and then I'll do a final review pass.

@Thomasdezeeuw
Copy link
Collaborator

Thanks @onestay, this was merged as a6d6587.

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