Repository navigation
fix(libSceNet): count only datagram bytes in sceNetIoctl FIONREAD on macOS - #10628
maximilianfv wants to merge 1 commit into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 57333d34904a5f01b8f2b0d480265960cf3ea542. The macOS-only branch preserves stream FIONREAD behavior and uses SO_NREAD for non-stream sockets, matching the platform-specific mismatch reproduced in the PR body and the existing guest assertion for a 5-byte datagram. Linux and Windows code paths are unchanged. The debt text now records the platform behavior and distinguishes the PS4 Pro sceNetGetSockInfo measurement from this sceNetIoctl call.
git diff --check upstream/main...HEAD and python3 tools/check_conventions.py --base upstream/main --head HEAD pass on this exact head. Hosted eligibility, conventions, intake, and labels pass; product test jobs were skipped. The macOS 21-test run and before/after debugger evidence are author-reported; I did not independently run macOS. I found no blocking issue in this focused platform fix.
|
Checked the host side on an M5 Pro, macOS 27.2 (26B5086k), with a small C program on a loopback UDP socket, built both x86_64 (Rosetta 2) and arm64. Both builds print the same numbers:
So on 27.2 too,
AI-assisted: yes |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on the new overlap check at exact head 57333d34904a5f01b8b2d0b480265960cf3ea542: I manually compared the shared docs/dev/debt/libSceNet.md hunk with #10572. git merge-tree --write-tree upstream/pr/10572 upstream/pr/10628 reports a content conflict in that file: both PRs edit the adjacent sceNetUsleep / sceNetIoctl entries in one hunk. The two code changes are distinct, and the conflict is straightforward to resolve by retaining both updated entries, but the PRs still need an agreed merge order and the dependent PR should record Depends on: #N as required by CONTRIBUTING. The current checklist says Depends on: none.
Please coordinate with #10572's author and record the agreed order/dependency. My earlier approval covered the implementation; this review requests changes only for the unresolved documentation merge coordination. The newly posted macOS measurements on #10628 are useful additional host evidence.
|
Thanks. AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on exact head 57333d34904a5f01b8f2b0d480265960cf3ea542. The merge-order blocker is resolved: #10572's author confirms it lands first, and this PR now records Depends on: #10572 and will rebase to retain both debt entries. My prior code review remains applicable: the macOS-only path uses SO_NREAD for non-stream sockets while preserving stream FIONREAD; Linux and Windows are unchanged. The author reports 20 of 21 focused tests pass in both runs; the one guest_sce_net stop is a separate resolver expectation, and the target FIONREAD assertion passes only with this change. Hosted policy checks pass and product builds were skipped. I did not independently run the macOS test suite. No blocker remains.
…macOS On macOS, FIONREAD on a datagram socket counts more than the queued payload, so after one 5-byte datagram sceNetIoctl reported 37. libkernel's sockets already ask a macOS datagram socket for SO_NREAD, the size of the next datagram, instead; do the same for sceNetSocket sockets that are not streams. Co-authored-by: Himansh Raj <67262056+iamthehimansh@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
57333d3 to
3d20b5e
Compare
|
Rebased onto main AI-assisted: yes |
What
On macOS,
FIONREADon a datagram socket counts more than the queued payload, sosceNetIoctl(socket, FIONREAD)on asceNetSocketUDP socket reported 37 after one 5-byte datagram, whereguest_sce_netexpects 5, as Linux reports. On macOS,sceNetIoctlnow asks a non-stream socket forSO_NREAD, the size of the next datagram, and keepsioctl(FIONREAD)for stream sockets. libkernel's own sockets already do this on macOS (QueuedBytesinlibkernel/Socket/src/Udp.cpp, from #7850 by mugurc), anddocs/dev/debt/libkernel.mdrecords it. ThesceNetIoctlentry indocs/dev/debt/libSceNet.mdnow says the same for macOS. Linux and Windows are unchanged.Prior art: iamthehimansh made the same change to
sceNetIoctlin commit b2c0e64 (2026-10-23) of the closed macOS port #8658, so he is credited as co-author on the commit. That commit is not in the PRs #8658 was split into (#10362, #10419). The two differ in small ways: that version sends onlyNET_SOCK_DGRAMsockets toSO_NREADand has a code comment and no debt entry; this one sends every non-stream socket and updates the debt entry. #7201 (marion909) and #7332 (ycarogabriel-lgtm) useSO_NREADonly in libkernel'sioctl, not in libSceNet. #9932 and #9948 (ntamas94) readFIONREADinsceNetGetSockInfo, a different function.What the console reports is a separate question: #9932 (ntamas94) measured on a PS4 Pro that
sceNetGetSockInfo's receive queue length counts each queued datagram's payload plus 16 bytes, as FreeBSD 11.4's receive buffer does. So this change matches neither the console nor plain macOS; it follows libkernel's sockets and the test. The debt entry now says so.Tested
Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2.
mainalone does not build on macOS, so I tested in a tree made of mugurc's macOS series (ci/macos-series-2, 27e9689) merged withmainat 1e20650, plus other macOS fixes that are not yet open PRs and #9231 (7c5d973). Only this change differed between the two runs.ctest -R '^(guest_sce_net|guest_inet_address|guest_net_.*|guest_datagram_.*|net_dontwait_.*|guest_socket_error|guest_udp)$'(21 tests): 20 pass both with and without this change.guest_sce_netaborts in both runs, at a different check each time:GuestSceNet.cpp:845. After one 5-bytesceNetSendtoto itself and a readable poll,sceNetIoctl(socket, 0x4004667f, ...)gives 37 where the test expects 5 (value read with lldb at the abort).GuestSceNet.cpp:1930:sceNetResolverStartNtoa6of"127.0.0.1"must fail with0x804101E1. This is a separate resolver gap.I found both stops with lldb on a
-O0rebuild. The test reaches theFIONREADcheck only with #9231 (Gonic0) applied, because #9231 fixes an earlier stop atGuestSceNet.cpp:175. This change does not need #9231's code; only the test needs it to get that far.Not tested on Linux or Windows; the code there is unchanged.
After #10572 merged I rebased onto
mainfccc63319d. Only the debt-file context changed, the code is byte-identical, and I did not re-run the tests.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code