fix: match name with the std's & socket2's set_nonblocking#33
fix: match name with the std's & socket2's set_nonblocking#33kristof-mattei wants to merge 1 commit intorust-netlink:mainfrom
set_nonblocking#33Conversation
Summary of ChangesHello @kristof-mattei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the socket non-blocking API by renaming the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors set_non_blocking to set_nonblocking for consistency with the standard library and socket2 crate. The renaming is applied consistently. However, I've identified several instances where set_nonblocking is called redundantly, as the async wrappers used (async_io::Async and tokio::io::unix::AsyncFd) already configure the socket for non-blocking I/O. Some of these redundant calls could also lead to panics due to the use of .unwrap(). I've provided suggestions to remove these calls for cleaner and safer code. Additionally, I've made a minor suggestion to improve variable naming in the set_nonblocking function to avoid shadowing, which enhances code clarity.
| unsafe fn from_raw_fd(fd: RawFd) -> Self { | ||
| let socket = Socket::from_raw_fd(fd); | ||
| socket.set_non_blocking(true).unwrap(); | ||
| socket.set_nonblocking(true).unwrap(); |
There was a problem hiding this comment.
The call to socket.set_nonblocking(true) is redundant because async_io::Async::new() already sets the file descriptor to non-blocking mode. Removing this line also eliminates a potential panic from the use of .unwrap().
| unsafe fn from_raw_fd(fd: RawFd) -> Self { | ||
| let socket = Socket::from_raw_fd(fd); | ||
| socket.set_non_blocking(true).unwrap(); | ||
| socket.set_nonblocking(true).unwrap(); |
There was a problem hiding this comment.
The call to socket.set_nonblocking(true) is redundant because tokio::io::unix::AsyncFd::new() already sets the file descriptor to non-blocking mode. Removing this line also eliminates a potential panic from the use of .unwrap().
| let mut nonblocking = nonblocking as libc::c_int; | ||
| let res = unsafe { | ||
| libc::ioctl(self.as_raw_fd(), libc::FIONBIO, &mut non_blocking) | ||
| libc::ioctl(self.as_raw_fd(), libc::FIONBIO, &mut nonblocking) |
There was a problem hiding this comment.
To improve clarity and avoid shadowing the nonblocking parameter, consider using a different name for this variable, for example mode. This aligns with general Rust API guidelines that discourage shadowing except in specific cases.
| let mut nonblocking = nonblocking as libc::c_int; | |
| let res = unsafe { | |
| libc::ioctl(self.as_raw_fd(), libc::FIONBIO, &mut non_blocking) | |
| libc::ioctl(self.as_raw_fd(), libc::FIONBIO, &mut nonblocking) | |
| let mut mode = nonblocking as libc::c_int; | |
| let res = unsafe { | |
| libc::ioctl(self.as_raw_fd(), libc::FIONBIO, &mut mode) |
| fn new(protocol: isize) -> io::Result<Self> { | ||
| let socket = Socket::new(protocol)?; | ||
| socket.set_non_blocking(true)?; | ||
| socket.set_nonblocking(true)?; |
There was a problem hiding this comment.
This call to socket.set_nonblocking(true) is redundant. The AsyncFd::new(socket)? call on the next line already sets the socket to non-blocking mode, so this line can be safely removed.
kristof-mattei
force-pushed
the
nonblocking
branch
from
0a5ad02 to
6cc785e
Compare
1 participant
No description provided.