nuttxpr commented on PR #14337: URL: https://github.com/apache/nuttx/pull/14337#issuecomment-2414597405
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Let's analyze this PR against the NuttX requirements: **Summary:** * **Why?** Fixes two bugs in socketcan: incorrect default network device selection and broken cmsg data appending for timestamps. * **What?** Modifies network device selection logic and socketcan message handling ( `netdev_findbyaddr`, `can_sendmsg.c`, `psock_send_eventhandler`, `devif_poll`). * **How?** * Prevents non-IP devices from being the default for sending IP data. * Ensures cmsg timestamp data is correctly included in socketcan messages by adjusting `d_sndlen` and `d_len` during copying. * **Issues:** No linked issues, which would be ideal for traceability. **Impact:** * **New Feature?** NO * **Existing Feature Changed?** YES, socketcan functionality is corrected. * **User Impact?** YES. Users relying on socketcan will see corrected behavior for timestamped messages. This could be a breaking change if applications relied on the previous, buggy behavior. * **Build Impact?** NO. No build system changes are indicated. * **Hardware Impact?** NO. Changes target logic, not specific hardware. * **Documentation Impact?** Potentially YES. If the previous buggy behavior was unintended, documentation might need clarification. * **Security Impact?** Unclear. Bug fixes can have security implications, but the description doesn't provide enough detail to assess. * **Compatibility Impact?** Potentially YES. Fixing the cmsg data handling could break applications expecting the incorrect format. Backward compatibility concerns should be addressed. **Testing:** * **Host:** Insufficient. Only mentions "imx9 platform" - needs OS, compiler details for reproducibility. * **Target:** "imx9 platform" with ethernet and CAN interfaces. More details on the specific board and configurations used would be beneficial. * **Logs:** Missing. "Testing logs before/after" sections are empty, making it impossible to verify the fix's effectiveness. **Conclusion:** This PR addresses important socketcan issues but falls short of the NuttX requirements in several ways: * **Missing Issue References:** Link related issues for better tracking. * **Incomplete Testing Information:** Provide detailed host and target environments and include actual testing logs demonstrating the fix. * **Unclear Impact Assessment:** Elaborate on potential security and compatibility implications of the changes. Addressing these points will significantly improve the PR's quality and make it easier for maintainers to review and merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
