Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The new readback branch and tests are focused, the Windows test results are reported, and the current eligibility, conventions, intake, labels and report checks pass; product jobs are skipped. I found no defect in the option mapping or short-buffer handling.
Please complete the overlap ordering before merge. #9525 also changes sceNetGetsockopt in Export.cpp and the libSceNet test-registration area; a direct git merge-tree of the two heads is clean, and their option cases appear separable, but the repository overlap rule still calls for recording which PR lands first. Since both PRs are yours, please add #9525 to the overlap/order note and to Depends on if #9542 should follow it, or record the agreed order if #9542 lands first. Keep the already-declared #9523 prerequisite for the getter-length hunk.
MarcZX10
left a comment
There was a problem hiding this comment.
Correction to my preceding review: the live branch overlap report lists #9523 at the sceNetGetsockopt length-validation lines, which this PR already declares, but does not list #9525. A direct merge-tree of #9542 and #9525 is clean; I withdraw the requested ordering change for #9525.
I found a separate unresolved overlap with #6044: it moves the existing libSceNet tests to tests/networking and rewrites their CMake registrations to add_library_test(networking) in the same libSceNet/CMakeLists.txt test block where this PR adds a test. A pairwise merge also conflicts in that CMake file. Please coordinate the landing order and record Depends on: #6044 if this PR should follow the reorganization, or note the agreed order and carry the new test registration/path through whichever lands second.
MarcZX10
left a comment
There was a problem hiding this comment.
The focused option mapping is consistent with the existing setters and FreeBSD flag-bit behavior; the reported Windows test covers enabled/disabled values, native readback, short buffers and invalid arguments. Current eligibility, conventions, intake, labels and report checks pass; product jobs are skipped.
The branch-mode overlap report identifies #9523 at the getter length-validation lines, and this PR correctly declares Depends on: #9523. The other unresolved manual overlap is #6044: its current diff moves the libSceNet tests to tests/networking and rewrites registrations in the same libSceNet/CMakeLists.txt test block where this PR adds guest_net_boolean_readback. Pairwise merge-tree reports a conflict in that CMake file. Please coordinate the order and record the dependency if this PR should follow #6044, then ensure the test source path and registration use the final organization.
|
@MarcZX10 thanks for the correction. I'll keep #9523 as the prerequisite; there's no need to add #9525. I confirmed the CMake conflict with #6044. That branch also conflicts in the same file against main alone, so this is a test-organization conflict rather than a dependency in the socket behavior. @TaymKal would landing #9523 and this PR before #6044 work for you? The new |
|
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.
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.
|
|
I refreshed this head against the current queue: diff-check and conventions pass, and it merges cleanly with current |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 3ca07d0 and FreeBSD 11.4 sys/kern/uipc_socket.c (sogetopt/sooptcopyout):
- The readback looks right to me: enabled options return their flag bit, as
so_options & optnamedoes. The short-length exemption is limited to these twoSOL_SOCKEToptions, so every other option still getsEINVALbelow 4 bytes and reports*optlen = 4as before. A three-way merge with current main applies cleanly. - Small debt suggestion: on a Windows host, the guest's
SO_REUSEADDRis passed to Winsock'sSO_REUSEADDR(setter, Export.cpp around line 1511 on main). On Winsock that lets a second socket bind an address and port another socket is actively using, while FreeBSD only lets the flag reuse a port held in TIME_WAIT and needsSO_REUSEPORTon both sockets for a second unicast bind. The setter on main already does this, but since this PR adds the debt entry for these options, a sentence there would record the gap. Maybe also note thatSO_REUSEPORT(0x200) still fails withEOPNOTSUPP.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@harisibrar1 thanks, added the Winsock SO_REUSEADDR binding difference and the unsupported SO_REUSEPORT result to the debt entry in @MarcZX10 this branch now includes main Update: the focused network targets build with MSYS2 UCRT64 GCC 16.2.0. Runtime validation is blocked by this host's missing AVX2/FMA/BMI/LZCNT/MOVBE features. The boolean-readback test and unchanged guest_sce_net fixture both stop in main's unchanged CPU startup guard before assertions. A collaborator needs to run the Build workflow on a compatible runner for these refreshed heads. No new runtime pass is claimed. |
What
Read
SO_REUSEADDRandSO_BROADCASTfrom the native socket insceNetGetsockopt, returning the FreeBSD flag bits (4and32) and truncating output to the supplied length. Semantics follow FreeBSD 11.4 sogetopt/sooptcopyout; console results remain unverified in library debt.Merge after #9523 because both edit the getter length-validation hunk.
Tested
Windows 11 x86-64, WinLibs MinGW-w64 GCC 15.2.0-r7, Ninja, Release. Head
28371daefdincludes main2fade7ef4e.guest_net_boolean_readback: 1/1 pass, with no skips.Integration order remains #9523 → #9542 → #9515 → #9578 and #9525 → #9543 → #9930 → #8686; #9931 follows #9542. #9930/#9931 may land independently after their prerequisites, before the agreed #9932/#9948 updates. Preserve every option path, short-buffer rule, test and debt entry.
#6044 was closed without merging its test relocation, so that former coordination item is retired. The existing #5836 order proposal remains unconfirmed; its peer-transport and broadcast-alias changes must retain current buffer validation and option state when integrated. No author agreement is assumed. #10097 changes neighboring SO_ERROR debt and lists #9523 first; retain both debt entries during integration.
Earlier combined Windows/WSL2 network results remain tied to tree c9c1293; they are separate from this head's fresh Windows run.
Fresh overlap snapshot
30be9b428b: existing getter/debt intersections remain covered by the ordering above. 7 shared-file comparisons are incomplete because those branches conflict with the base: #5836, #7201, #9727, #9733, #9948, #10502, #10706. Manual inspection identifies export-header refactoring (#10706), diagnostic display (#10502), argument validation (#9727), send/platform paths (#9733/#7201), socket records (#9948), and the already pending peer/broadcast changes (#5836). No duplicate option branch was found in the inspected shared hunks. Retain the private socket fields, canonical declarations, test registrations and debt entries when integrating. These source checks do not establish author agreement or a combined runtime pass.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)