-
Notifications
You must be signed in to change notification settings - Fork 0
7782: Support as many abstract datagram unix domain socket operations as possible #75
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,8 @@ use crate::util::check_socket_for_blocking; | |||||||||||||||||
| use std::fmt; | ||||||||||||||||||
| use std::io; | ||||||||||||||||||
| use std::net::Shutdown; | ||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||
| use std::os::unix::ffi::OsStrExt; | ||||||||||||||||||
| use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, IntoRawFd, RawFd}; | ||||||||||||||||||
| use std::os::unix::net; | ||||||||||||||||||
| use std::path::Path; | ||||||||||||||||||
|
|
@@ -396,7 +398,8 @@ impl UnixDatagram { | |||||||||||||||||
| where | ||||||||||||||||||
| P: AsRef<Path>, | ||||||||||||||||||
| { | ||||||||||||||||||
| let socket = mio::net::UnixDatagram::bind(path)?; | ||||||||||||||||||
| let addr = SocketAddr::from_path(path)?.0; | ||||||||||||||||||
| let socket = mio::net::UnixDatagram::bind_addr(&addr)?; | ||||||||||||||||||
| UnixDatagram::new(socket) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -571,6 +574,11 @@ impl UnixDatagram { | |||||||||||||||||
| /// The `send` method may be used to send data to the specified address. | ||||||||||||||||||
| /// `recv` and `recv_from` will only receive data from that address. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Limitations | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This method currently does not allow specifying Linux abstract | ||||||||||||||||||
| /// namespace paths. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Examples | ||||||||||||||||||
| /// ``` | ||||||||||||||||||
| /// # use std::error::Error; | ||||||||||||||||||
|
|
@@ -605,6 +613,16 @@ impl UnixDatagram { | |||||||||||||||||
| /// # } | ||||||||||||||||||
| /// ``` | ||||||||||||||||||
| pub fn connect<P: AsRef<Path>>(&self, path: P) -> io::Result<()> { | ||||||||||||||||||
| //FIXME: Convert this to `mio::net::UnixDatagram::connect_addr` once | ||||||||||||||||||
| // that is supported by MIO | ||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||
| if path.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||||||||||||||||||
| return Err(io::Error::new( | ||||||||||||||||||
| io::ErrorKind::Other, | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency and to provide a more descriptive error, it would be better to use
Suggested change
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue. |
||||||||||||||||||
| "UnixDatagram::connect(): abstract paths currently unsupported", | ||||||||||||||||||
| )); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| self.io.connect(path) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -694,6 +712,11 @@ impl UnixDatagram { | |||||||||||||||||
|
|
||||||||||||||||||
| /// Tries to send a datagram to the peer without waiting. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Limitations | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This method currently does not allow specifying Linux abstract | ||||||||||||||||||
| /// namespace paths. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Examples | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// ```no_run | ||||||||||||||||||
|
|
@@ -733,6 +756,16 @@ impl UnixDatagram { | |||||||||||||||||
| where | ||||||||||||||||||
| P: AsRef<Path>, | ||||||||||||||||||
| { | ||||||||||||||||||
| //FIXME: Convert this to `mio::net::UnixDatagram::send_to_addr` once | ||||||||||||||||||
| // that is supported by MIO | ||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||||||||||||||||||
| return Err(io::Error::new( | ||||||||||||||||||
| io::ErrorKind::Other, | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue. |
||||||||||||||||||
| "UnixDatagram::try_send_to(): abstract paths currently unsupported", | ||||||||||||||||||
| )); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| self.io | ||||||||||||||||||
| .registration() | ||||||||||||||||||
| .try_io(Interest::WRITABLE, || self.io.send_to(buf, target)) | ||||||||||||||||||
|
|
@@ -1066,6 +1099,11 @@ impl UnixDatagram { | |||||||||||||||||
| /// [`tokio::select!`](crate::select) statement and some other branch | ||||||||||||||||||
| /// completes first, then it is guaranteed that the message was not sent. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Limitations | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This method currently does not allow specifying Linux abstract | ||||||||||||||||||
| /// namespace paths. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Examples | ||||||||||||||||||
| /// ``` | ||||||||||||||||||
| /// # use std::error::Error; | ||||||||||||||||||
|
|
@@ -1102,6 +1140,16 @@ impl UnixDatagram { | |||||||||||||||||
| where | ||||||||||||||||||
| P: AsRef<Path>, | ||||||||||||||||||
| { | ||||||||||||||||||
| //FIXME: Convert this to `mio::net::UnixDatagram::send_to_addr` once | ||||||||||||||||||
| // that is supported by MIO | ||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||||||||||||||||||
| return Err(io::Error::new( | ||||||||||||||||||
| io::ErrorKind::Other, | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue. |
||||||||||||||||||
| "UnixDatagram::send_to(): abstract paths currently unsupported", | ||||||||||||||||||
| )); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| self.io | ||||||||||||||||||
| .registration() | ||||||||||||||||||
| .async_io(Interest::WRITABLE, || self.io.send_to(buf, target.as_ref())) | ||||||||||||||||||
|
|
@@ -1205,6 +1253,11 @@ impl UnixDatagram { | |||||||||||||||||
| /// `Waker` from the `Context` passed to the most recent call will be scheduled to | ||||||||||||||||||
| /// receive a wakeup. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Limitations | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This method currently does not allow specifying Linux abstract | ||||||||||||||||||
| /// namespace paths. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// # Return value | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// The function returns: | ||||||||||||||||||
|
|
@@ -1225,6 +1278,16 @@ impl UnixDatagram { | |||||||||||||||||
| where | ||||||||||||||||||
| P: AsRef<Path>, | ||||||||||||||||||
| { | ||||||||||||||||||
| //FIXME: Convert this to `mio::net::UnixDatagram::send_to_addr` once | ||||||||||||||||||
| // that is supported by MIO | ||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||
| if target.as_ref().as_os_str().as_bytes().starts_with(b"\0") { | ||||||||||||||||||
| return Poll::Ready(Err(io::Error::new( | ||||||||||||||||||
| io::ErrorKind::Other, | ||||||||||||||||||
| "UnixDatagram::send_to(): abstract paths currently unsupported", | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The error message is copy/pasted from another method and the method name is still mentioning the source of the copy. The method name should be updated to "poll_send_to()" to avoid confusion in the developer debugging an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error message references wrong method name in poll_send_toLow Severity The error message in
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! The error message is copy/pasted from another method and the method name is still mentioning the source of the copy. The method name should be updated to "poll_send_to()" to avoid confusion in the developer debugging an issue. |
||||||||||||||||||
| ))); | ||||||||||||||||||
|
Comment on lines
+1285
to
+1288
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two small issues here:
Suggested change
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Since some UDP related methods do not support abstract paths it would be better to return an InvalidInput kind of error to better tell the caller what is the issue. |
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| self.io | ||||||||||||||||||
| .registration() | ||||||||||||||||||
| .poll_write_io(cx, || self.io.send_to(buf, target.as_ref())) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,12 @@ | ||
| use std::fmt; | ||
| use std::io; | ||
| #[cfg(target_os = "android")] | ||
| use std::os::android::net::SocketAddrExt; | ||
| #[cfg(target_os = "linux")] | ||
| use std::os::linux::net::SocketAddrExt; | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| use std::os::unix::ffi::OsStrExt; | ||
| use std::os::unix::net::SocketAddr as StdSocketAddr; | ||
| use std::path::Path; | ||
|
|
||
| /// An address associated with a Tokio Unix socket. | ||
|
|
@@ -7,9 +15,33 @@ use std::path::Path; | |
| /// can convert to and from the standard library `SocketAddr` type using the | ||
| /// [`From`] trait. | ||
| #[derive(Clone)] | ||
| pub struct SocketAddr(pub(super) std::os::unix::net::SocketAddr); | ||
| pub struct SocketAddr(pub(super) StdSocketAddr); | ||
|
|
||
| impl SocketAddr { | ||
| /// Creates an address from a path | ||
| /// | ||
| /// On Linux, allows the path to start with a leading `\0` to declare | ||
| /// a socket path within the abstract namespace. | ||
| pub fn from_path<P>(path: P) -> io::Result<Self> | ||
| where | ||
| P: AsRef<Path>, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The method is available for both Linux and Android (Android is based on Linux but still there are differences), so the documentation should be more explicit that Android is also supported. |
||
| { | ||
| // For now, we handle abstract socket paths on linux here. | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| let addr = { | ||
| let os_str_bytes = path.as_ref().as_os_str().as_bytes(); | ||
| if os_str_bytes.starts_with(b"\0") { | ||
| StdSocketAddr::from_abstract_name(&os_str_bytes[1..])? | ||
| } else { | ||
| StdSocketAddr::from_pathname(path)? | ||
| } | ||
| }; | ||
| #[cfg(not(any(target_os = "linux", target_os = "android")))] | ||
| let addr = StdSocketAddr::from_pathname(path)?; | ||
|
|
||
| Ok(SocketAddr(addr)) | ||
| } | ||
|
|
||
| /// Returns `true` if the address is unnamed. | ||
| /// | ||
| /// Documentation reflected in [`SocketAddr`]. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,10 @@ use tokio::try_join; | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| use std::future::poll_fn; | ||||||||||||||||||||||
| use std::io; | ||||||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||||||
| use std::os::linux::net::SocketAddrExt; | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrong platform import for Android in test fileMedium Severity The import
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android. |
||||||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Severity: high 🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android. |
||||||||||||||||||||||
| use std::os::unix::net as std_net; | ||||||||||||||||||||||
|
Comment on lines
+11
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes. On Android targets the abstract Unix-socket extension trait is exposed as Practical pattern: #[cfg(target_os = "linux")]
use std::os::linux::net::SocketAddrExt;
#[cfg(target_os = "android")]
use std::os::android::net::SocketAddrExt;[1] (doc.rust-lang.org) [2] (doc.rust-lang.org) Citations:
Split the import by platform for Android compatibility. Lines 11-14 compile for both Linux and Android, but 🔧 Proposed fix-#[cfg(any(target_os = "linux", target_os = "android"))]
-use std::os::linux::net::SocketAddrExt;
+#[cfg(target_os = "android")]
+use std::os::android::net::SocketAddrExt;
+#[cfg(target_os = "linux")]
+use std::os::linux::net::SocketAddrExt;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The IT test should use one import for Linux and a different one for Android. Since the IT tests are executed only on Linux in the Github Actions CI environment there is no issue at the moment because it is always Linux and the imported file matches the OS. A problem would occur when someone tries to run the test on Android. |
||||||||||||||||||||||
| use std::sync::Arc; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async fn echo_server(socket: UnixDatagram) -> io::Result<()> { | ||||||||||||||||||||||
|
|
@@ -71,6 +75,50 @@ async fn echo_from() -> io::Result<()> { | |||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #[allow(clippy::bool_assert_comparison)] | ||||||||||||||||||||||
| #[tokio::test] | ||||||||||||||||||||||
| #[cfg_attr(miri, ignore)] // No `socket` on miri. | ||||||||||||||||||||||
| async fn recv_abstract() -> io::Result<()> { | ||||||||||||||||||||||
| // Generate 128-bit random string prefixed by `\0` as socket path | ||||||||||||||||||||||
| let abstract_path = format!( | ||||||||||||||||||||||
| "\0{:016x}{:016x}", | ||||||||||||||||||||||
| rand::random::<u64>(), | ||||||||||||||||||||||
| rand::random::<u64>() | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // On non-Linux platforms, abstract socket paths are rejected | ||||||||||||||||||||||
| #[cfg(not(any(target_os = "linux", target_os = "android")))] | ||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||
| UnixDatagram::bind(&abstract_path).err().map(|e| e.kind()), | ||||||||||||||||||||||
| Some(io::ErrorKind::InvalidInput) | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Remaining test is Linux-only | ||||||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| // Create socket in abstract namespace | ||||||||||||||||||||||
| let recv_socket = UnixDatagram::bind(&abstract_path)?; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Send message from stdlib | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| //FIXME: Use `tokio` native here once passing abstract socket paths to | ||||||||||||||||||||||
| // `send_to`/`connect` is implemented | ||||||||||||||||||||||
| let send_socket = std_net::UnixDatagram::unbound()?; | ||||||||||||||||||||||
| send_socket.send_to_addr( | ||||||||||||||||||||||
| b"READY=1\n", | ||||||||||||||||||||||
| &std_net::SocketAddr::from_abstract_name(&abstract_path[1..])?, | ||||||||||||||||||||||
| )?; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Receive and validate message | ||||||||||||||||||||||
| let mut buf = vec![0u8; 32]; | ||||||||||||||||||||||
| let (len, addr) = recv_socket.recv_from(&mut buf).await?; | ||||||||||||||||||||||
| assert_eq!(&buf[0..len], b"READY=1\n"); | ||||||||||||||||||||||
| assert_eq!(addr.is_unnamed(), true); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Even though we use sync non-blocking io we still need a reactor. | ||||||||||||||||||||||
| #[tokio::test] | ||||||||||||||||||||||
| #[cfg_attr(miri, ignore)] // No SOCK_DGRAM for `socketpair` in miri. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limitation docs should mention Android too.
The runtime checks for abstract paths are gated with
cfg(any(target_os = "linux", target_os = "android"))at Lines 618-624, 761-767, 1145-1151, and 1283-1289, so the current "Linux abstract namespace paths" wording is incorrect on Android across all four methods.Also applies to: 715-718, 1102-1105, 1256-1259
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The method is available for both Linux and Android (Android is based on Linux but still there are differences), so the documentation should be more explicit that Android is also supported.