Skip to content

fix(net): truncate TCP_NODELAY readback - #9931

Merged
oneandonlydean merged 6 commits into
boykopovar:mainfrom
Queuse:codex/net-nodelay-short-readback
Oct 25, 2026
Merged

oneandonlydean merged 6 commits into
boykopovar:mainfrom
Queuse:codex/net-nodelay-short-readback

Conversation

@Queuse

@Queuse Queuse commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

What

Allow zero-length and short TCP_NODELAY output buffers and copy only the supplied bytes. Keep FreeBSD's TF_NODELAY bit-valued result and existing validation for other TCP options, following tcp_ctloutput.

Tested

Windows 11 x86-64, WinLibs MinGW-w64 GCC 15.2.0-r7, Ninja, Release. Head 7ad69a9f19 includes main 63a863d3b0.

  • Rebuilt affected libraries and focused targets; guest_net_nodelay_readback: 1/1 pass, with no skips.
  • Public declaration audit, conventions and whitespace checks pass. The fixture uses canonical public headers.
  • Consulted main 75227c7cdc leaves the focused library/fixture sources and test registrations unchanged after the tested base; the trial merge is clean and retains the intervening debt corrections. Intervening shader changes are outside these focused runtime tests.
  • No fresh Linux/macOS runtime, full hosted product build, title or console run.

Integration order: #9523 → #9542 → #9515 → #9578; #9525 → #9543 → #9930; #9931 follows #9542. #9930/#9931 can land independently after their prerequisites and before #9932/#9948. #8686 and the canonical-header migration #10706 are already in main; retain accepted-socket protocol inheritance, connection state, all option paths, short-buffer rules, tests and debt entries. The #5836 peer/broadcast order proposal remains unconfirmed.

Overlap snapshot 63a863d3b0: confirmed intersections with #9523, #9525, #9542, #9932, #10097. 7 shared-file comparisons are incomplete because those branches conflict with the base: #5836, #7201, #9494, #9727, #9733, #9948, #10502. Manual inspection finds peer/broadcast paths, platform sends, argument validation, IPv6 conversion, socket records and diagnostic display. No duplicate option implementation was found in those shared hunks. Preserve fields, declarations, registrations and debt entries when integrating. These checks do not establish a combined runtime pass with the conflicting branches.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: feat(libSceNet): read reuse and broadcast flags #9542 (merge order; includes feat(libSceNet): support SO_KEEPALIVE #9523)
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10

Copy link
Copy Markdown
Contributor

I checked #9930’s new SO_PROTOCOL getter alongside this PR. It is distinct behavior, but both patch sceNetGetsockopt in Export.cpp and add tests/debt entries to the same libSceNet build/docs. The edits appear composable: #9930 adds protocol state and its getter branch, while this PR changes the TCP_NODELAY output-length path. Please coordinate the landing order or state explicitly that either can land first and ensure the rebase retains both branches, CTest registrations and debt entries. This branch is based on 8aa44cf3; current main is f57ceaad after four intervening commits, so refresh before final validation.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 9932 docs/dev/debt/libSceNet.md L44-45
PR 10097 docs/dev/debt/libSceNet.md L43-44

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.

Pull request Shared files not compared
PR 5836 core/libs/prx/libSceNet/CMakeLists.txt
core/libs/prx/libSceNet/Export.cpp
docs/dev/debt/libSceNet.md
PR 7201 core/libs/prx/libSceNet/Export.cpp
PR 9727 core/libs/prx/libSceNet/Export.cpp
docs/dev/debt/libSceNet.md
PR 9733 core/libs/prx/libSceNet/Export.cpp
docs/dev/debt/libSceNet.md
PR 9948 core/libs/prx/libSceNet/Export.cpp
docs/dev/debt/libSceNet.md
PR 10502 core/libs/prx/libSceNet/Export.cpp
docs/dev/debt/libSceNet.md

@MarcZX10

Copy link
Copy Markdown
Contributor

One additional overlap surfaced: #9932 adds FIONREAD receive-queue reporting and edits libSceNet/Export.cpp and docs/dev/debt/libSceNet.md (plus the shared GuestSceNet.cpp test). Its fill_sock_info change appears independent of this PR’s TCP_NODELAY readback path, but please coordinate the order with #9932 and ensure a rebase retains both changes and their tests/debt entries. I also noted this in #9932.

@Queuse

Queuse commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@MarcZX10 I checked the two heads together (c079a98a41 and fea0994737): the three-way merge is clean, so #9930 and #9931 can land in either order. I've recorded that in both descriptions. The rebase needs to keep both getter branches, test registrations and debt entries. Their existing prerequisites stay in place: #9543 for #9930 and #9542 for #9931.

Both also merge cleanly with main bd1d3994ce. The runtime results are still tied to the heads and base named in Tested; I haven't run the combined code. I'll refresh and rerun the focused test before final validation.

@MarcZX10

Copy link
Copy Markdown
Contributor

I completed a diff/code pass on the TCP_NODELAY readback path. Short reads copy only the supplied bytes, preserve surrounding sentinels, and keep the existing socket/level validation ordering; the tests cover native option changes and neighboring error cases. I found no code defect in this diff. git diff --check upstream/main...refs/review/9931 and the current-main conventions check both pass.

The branch is 2 commits ahead/48 behind current main caa089fd735d0a5d615279e0f567cb638ad65690 (merge base 1fd6f9bb588f66dfc5f1ed3bda9cb9dda434a27b). No intervening main commit touches its changed paths, but please sync before merge and rerun guest_net_nodelay_readback on the final head. Please also finish merge-order coordination with #9930 and #9932; their distinct changes share the implementation/debt files. The author-reported Windows focused test is not a hosted product result.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head fea09947372fbb0907c2e549f52a522257008219. The short-output handling copies only the caller-supplied bytes and preserves the existing option validation; focused tests cover short/oversized lengths, sentinels, option changes, IPv4/IPv6 and invalid handles. The author’s combined Windows and WSL2 runs report 8/8 focused network tests including this path; current-main diff-check and conventions pass, and no current-main commit since the merge base touches the PR’s changed paths. I did not run the C++ test locally. The documented dependency on open #9542 (which includes #9523) remains a merge-order requirement, and the PR description correctly says to refresh and rerun on the final integrated head before merge. No independent defect found.

@Queuse
Queuse marked this pull request as ready for review October 24, 2026 21:58
@oneandonlydean oneandonlydean self-assigned this Oct 25, 2026
@oneandonlydean
oneandonlydean merged commit ee856eb into boykopovar:main Oct 25, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants