Skip to content

Conversation

@Samuka007
Copy link
Member

@Samuka007 Samuka007 commented Dec 11, 2025

[ RUN      ] AllInetTests/UdpSocketTest.Getsockname/0
[       OK ] AllInetTests/UdpSocketTest.Getsockname/0 (0 ms)
[ RUN      ] AllInetTests/UdpSocketTest.Getpeername/0
[       OK ] AllInetTests/UdpSocketTest.Getpeername/0 (0 ms)

Note

Implements UDP getsockname/getpeername, refactors endpoint handling, fixes unbound send behavior, and enables gVisor socket/udp tests with blocklists.

  • UDP socket core (kernel/src/net/socket/inet/datagram):
    • Implement UdpSocket::remote_endpoint() and refine local_endpoint() to derive from IpListenEndpoint (defaults to 0.0.0.0:0 when unbound).
    • On try_send, bind ephemeral on first send using destination address and return EDESTADDRREQ when destination is missing.
  • UDP bound state (kernel/src/net/socket/inet/datagram/inner.rs):
    • Remove stored local_endpoint; derive via socket.endpoint().
    • Add BoundUdp::remote_endpoint(); drop local_endpoint() method.
  • Tests:
    • Enable socket_test and udp_socket_test in user/apps/tests/syscall/gvisor/whitelist.txt.
    • Add blocklists for socket_test and extensive cases in udp_socket_test to gate unsupported scenarios.

Written by Cursor Bugbot for commit 726e143. This will update automatically on new commits. Configure here.

Copilot AI review requested due to automatic review settings December 11, 2025 14:54
@github-actions github-actions bot added Bug fix A bug is fixed in this pull request test Unitest/User space test labels Dec 11, 2025
@fslongjin
Copy link
Member

@claude review this PR in Chinese

@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Dec 11, 2025

Claude finished @fslongjin's task in 7m 56s —— View job


代码审查: fix(net): udp getsockname/getpeername

待办事项:

  • 分析PR上下文和变更文件
  • 理解代码变更逻辑
  • 检查代码质量和潜在问题
  • 提供中文审查反馈
  • 更新审查结果

分支: fix-udp-getname
PR: #1460
作者: Samuka007
变更文件: 2个文件 (+22/-12)


审查总结

这个PR修复了UDP socket的getsocknamegetpeername系统调用实现。从测试日志看,相关测试已经通过。

主要变更

1. 移除了冗余的本地端点缓存 (inner.rs)

  • 删除BoundUdp结构体中的local_endpoint: smoltcp::wire::IpEndpoint字段
  • 删除local_endpoint()方法
  • 新增remote_endpoint()方法,正确地从remote字段获取远程端点

2. 实现了正确的端点获取逻辑 (mod.rs)

  • 实现remote_endpoint():调用bound.remote_endpoint(),对未连接的socket返回ENOTCONN
  • 实现local_endpoint():通过bound.endpoint()动态获取监听端点
    • 对未绑定的socket返回0.0.0.0:0(符合POSIX规范)
    • 使用addr.unwrap_or(Ipv4([0, 0, 0, 0].into()))处理可能的None地址

3. 修正错误码

  • EADDRNOTAVAIL改为EDESTADDRREQ,更准确地表示"目标地址必需"的错误

代码质量评估

优点 ✅

  1. 设计改进:移除字段缓存,改为从socket动态获取,避免数据不一致
  2. 错误处理正确
    • remote_endpoint()对未连接socket返回ENOTCONN
    • local_endpoint()对未绑定socket返回0.0.0.0:0
  3. 测试通过:PR描述中的测试输出显示相关测试已通过
  4. 错误码更准确EDESTADDRREQEADDRNOTAVAIL更符合语义

潜在问题 ⚠️

  1. unwrap使用:在mod.rs:265mod.rs:275使用了as_ref().unwrap()

    • 虽然inner在初始化时是Some(...),但理论上可能有None状态
    • 建议:使用as_ref().expect("UDP inner should not be None")提供更清晰的错误信息
  2. IPv6支持待完善:代码注释中标注了"TODO: IPv6 support"

建议改进

  1. 考虑添加#[inline]到频繁调用的小方法
  2. 可以为UdpInner实现Default trait以避免unwrap()的风险

总体评价

这是一个高质量的修复,解决了UDP socket端点获取的核心问题。代码变更简洁、逻辑正确,测试验证充分。建议合并


技术细节:

  • bound.endpoint()返回IpListenEndpoint,包含可选的地址和端口
  • 对未指定地址的情况,使用默认的0.0.0.0地址
  • 错误码EDESTADDRREQ对应EDESTADDRREQ系统错误(目标地址必需)

审查完成

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the UDP socket getsockname and getpeername operations by implementing the remote_endpoint() method (which was previously marked as todo!()) and refactoring the local_endpoint() implementation. The changes remove the redundant cached local_endpoint field from the BoundUdp struct and instead retrieve the endpoint information directly from the underlying smoltcp socket, making the implementation more robust and consistent.

Key changes include:

  • Implementation of remote_endpoint() to return the connected remote address or ENOTCONN error
  • Refactored local_endpoint() to query the socket's endpoint directly instead of using a cached field
  • Changed error code from EADDRNOTAVAIL to EDESTADDRREQ for missing destination address scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
kernel/src/net/socket/inet/datagram/mod.rs Implements remote_endpoint() and updates local_endpoint() to return proper endpoint information, including handling for unbound sockets with unspecified addresses
kernel/src/net/socket/inet/datagram/inner.rs Removes the redundant local_endpoint field from BoundUdp struct and adds a remote_endpoint() method to retrieve the remote address from the lock

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
let mut inner_guard = self.inner.write();
let inner = match inner_guard.take().expect("Udp Inner is None") {
let inner = match inner_guard.take().expect("Udp Inner is None") { // TODO: 此处会为空,需要DEBUG
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The TODO comment indicates that the inner can be empty and needs debugging. However, the code uses expect() which will panic if it's None. If this is a known issue that needs to be fixed, consider using proper error handling (e.g., ok_or(SystemError::EBADF)?) instead of expect() to prevent panics. If this is not expected to be None, the TODO comment should be clarified to explain when this condition can occur.

Suggested change
let inner = match inner_guard.take().expect("Udp Inner is None") { // TODO: 此处会为空,需要DEBUG
let inner = match inner_guard.take().ok_or(SystemError::EBADF)? { // TODO: 此处会为空,需要DEBUG

Copilot uses AI. Check for mistakes.

fn remote_endpoint(&self) -> Result<Endpoint, SystemError> {
todo!()
match self.inner.read().as_ref().unwrap() {
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The use of unwrap() here could panic if the inner Option is None. While the inner field is initialized as Some(UdpInner::Unbound(...)) in new(), it can be set to None via take() in the close() method (line 93). If remote_endpoint() is called after the socket is closed, this will panic. Consider using a more explicit error handling approach, such as ok_or(SystemError::EBADF)? to return an appropriate error when the socket is closed.

Suggested change
match self.inner.read().as_ref().unwrap() {
match self.inner.read().as_ref().ok_or(SystemError::EBADF)? {

Copilot uses AI. Check for mistakes.

fn local_endpoint(&self) -> Result<Endpoint, SystemError> {
use smoltcp::wire::{IpListenEndpoint, IpEndpoint, IpAddress::*};
match self.inner.read().as_ref().unwrap() {
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The use of unwrap() here could panic if the inner Option is None. While the inner field is initialized as Some(UdpInner::Unbound(...)) in new(), it can be set to None via take() in the close() method (line 93). If local_endpoint() is called after the socket is closed, this will panic. Consider using a more explicit error handling approach, such as ok_or(SystemError::EBADF)? to return an appropriate error when the socket is closed.

Suggested change
match self.inner.read().as_ref().unwrap() {
match self.inner.read().as_ref().ok_or(SystemError::EBADF)? {

Copilot uses AI. Check for mistakes.
_ => Err(SystemError::ENOTCONN),
UdpInner::Bound(bound) => {
let IpListenEndpoint { addr, port } = bound.endpoint();
Ok(Endpoint::Ip(IpEndpoint::new(addr.unwrap_or(Ipv4([0, 0, 0, 0].into())), port)))
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The hardcoded IPv4 address [0, 0, 0, 0] creates a magic number. Consider using a named constant like UNSPECIFIED_LOCAL_ENDPOINT_V4 (defined at kernel/src/net/socket/inet/mod.rs:32-33) for better maintainability and consistency with the TCP implementation.

Copilot uses AI. Check for mistakes.
Ok(Endpoint::Ip(IpEndpoint::new(addr.unwrap_or(Ipv4([0, 0, 0, 0].into())), port)))
},
// TODO: IPv6 support
_ => Ok(Endpoint::Ip(IpEndpoint::new(Ipv4([0, 0, 0, 0].into()), 0))),
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The hardcoded IPv4 address [0, 0, 0, 0] and port 0 create magic numbers. Consider using a named constant like UNSPECIFIED_LOCAL_ENDPOINT_V4 (defined at kernel/src/net/socket/inet/mod.rs:32-33) for better maintainability and consistency with the TCP implementation.

Copilot uses AI. Check for mistakes.
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on January 26

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

remote: SpinLock::new(Some(endpoint)),
local_endpoint: endpoint,
})
}
Copy link

Choose a reason for hiding this comment

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

Bug: getsockname returns wrong data after implicit bind

The bind_ephemeral function reserves an ephemeral port via port_manager().bind_ephemeral_port() but never binds the smoltcp socket to that port (unlike the bind function which explicitly calls socket.bind()). The old code stored the endpoint in a local_endpoint field and returned it correctly. The new local_endpoint() implementation relies on socket.endpoint() from the smoltcp socket, which returns incorrect/default data for sockets that went through bind_ephemeral since they were never actually bound. This causes getsockname() to return wrong data after implicit binding via sendto().

Additional Locations (1)

Fix in Cursor Fix in Web

let endpoint = smoltcp::wire::IpEndpoint::new(address, bound_port);
Ok(BoundUdp {
inner,
remote: SpinLock::new(Some(endpoint)),
Copy link

Choose a reason for hiding this comment

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

Bug: bind_ephemeral stores local endpoint in remote field

In bind_ephemeral, the endpoint variable is constructed from the local address and local bound_port, making it the local endpoint. However, this is stored in the remote field. The newly implemented remote_endpoint() method returns this value, so getpeername() would return the local endpoint instead of the remote endpoint for sockets that went through bind_ephemeral. While this existed in the old code, it's now exposed since remote_endpoint() was changed from todo!() to an actual implementation.

Additional Locations (1)

Fix in Cursor Fix in Web

@fslongjin fslongjin merged commit 165fa8e into DragonOS-Community:master Dec 14, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug fix A bug is fixed in this pull request test Unitest/User space test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants