Repository navigation
Conversation
|
Windows data point for Reading the change: |
|
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
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The socket ioctl path sets and clears the same descriptor flag reported by F_GETFD, and the added checks cover both request forms and null/non-null arguments.
What I ran
Reviewed head 4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c against PR base f0c0b2d19a6ab6bfb11de584bfc38895816ccaf5; GitHub reports it mergeable and clean. The stack preview used current target bff0289787c98570491c8f175179c8a5651c8017; #13695 and #11219 preview individually cleanly, while their combined preview conflicts only in the debt file. Environment: Linux 6.1, Python 3.11.2, GCC 12.2.0. No build or compile was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13695
git diff --check f0c0b2d19a6ab6bfb11de584bfc38895816ccaf5...4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c
python3 tools/check_conventions.py --base f0c0b2d19a6ab6bfb11de584bfc38895816ccaf5 --head 4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c --title 'fix(libkernel): set and clear close-on-exec with FIOCLEX and FIONCLEX on a socket' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13695-4b3b93f0ec29/pr-body.md
python3 tools/pr_stack_check.py 13695 --repo boykopovar/AnyPS5
python3 tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13695-4b3b93f0ec29/head-4b3b93f0ec29-a051f1fd --json core/libs/tests/GuestKernelFcntl.cppResults
| Check | Result |
|---|---|
| Diff whitespace | Passed. |
| Conventions and PR template | Passed with one existing-style debt-entry length warning (1,672 characters). |
| Test registration | Confirmed guest_kernel_fcntl is registered in core/libs/prx/libkernel/CMakeLists.txt. The heuristic's possible-unregistered warning is a false positive. |
| Optional audit helpers | The isolated precheck worktree lacks the workspace-local ABI and test-quality helper scripts; the ABI audit is inapplicable (no exports changed), and I ran the test-quality helper directly. |
| Dependency and mergeability | PR is clean against its base. #11219 remains open; a pairwise preview auto-merges the code and tests but conflicts in docs/dev/debt/libkernel.md. The declared chain also includes #9713, which currently conflicts with the target. |
| Hosted checks | Conventions, Windows, eligibility, intake, labels, library-portability, and report passed; Linux and specialized jobs were skipped. |
| Test execution | The author reports the focused test and 44 related tests passed on Windows. I did not run those C++ tests locally. |
Relevant raw output
$ git diff --check f0c0b2d19a6ab6bfb11de584bfc38895816ccaf5...4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c
(no output)
docs/dev/debt/libkernel.md:304: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 1672 characters
1 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all
4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c:core/libs/prx/libkernel/CMakeLists.txt:2202: add_test_executable(guest_kernel_fcntl_tests ${LIBS_INCLUDE_DIR}/tests/GuestKernelFcntl.cpp)
4b3b93f0ec292d978f4bbb038c4f2ad59aa3275c:core/libs/prx/libkernel/CMakeLists.txt:2206: add_test(NAME guest_kernel_fcntl COMMAND guest_kernel_fcntl_tests)
{
"findings": [
{
"path": "core/libs/tests/GuestKernelFcntl.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}
$ gh pr checks 13695 --repo boykopovar/AnyPS5
conventions pass 14s https://github.com/boykopovar/AnyPS5/actions/runs/41151024848/job/124387302691
labels pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41151024778/job/124387302627
windows pass 4m52s https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387647452
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387649077
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387649508
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387648561
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387649013
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387648508
eligibility pass 4s https://github.com/boykopovar/AnyPS5/actions/runs/41151024853/job/124387301978
intake pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41151024843/job/124387302986
library-portability pass 1m45s https://github.com/boykopovar/AnyPS5/actions/runs/41151024935/job/124387301277
report pass 35s https://github.com/boykopovar/AnyPS5/actions/runs/41151024867/job/124387300949
Negative control
| Assertion | Change to code under test | Failed? |
|---|---|---|
New FIOCLEX/FIONCLEX socket assertions |
Author reports testing them with the base Udp.cpp implementation |
Author reports yes; I did not reproduce this control locally. |
Findings
Blocking: None found.
Non-blocking: The #11219 dependency has not landed. Its declared order should be retained; resolve the debt-file conflict when that dependency is integrated.
Sources
- FreeBSD 11.4
kern_ioctl, pinned source handlesFIOCLEX/FIONCLEXbefore dispatching to the descriptor operation. - Reviewed socket
ioctlimplementation and added socket assertions.
Not verified
I did not build or run C++ tests locally, test Linux behavior, or verify behavior on PS5 hardware. Linux and specialized hosted jobs were skipped. The Windows test results above are author/CI evidence, not local runs.
To get this to a good state
No code change is needed for this review. Keep the declared dependency order and reconcile the debt-file wording when #11219 lands; I can recheck the integrated head then.
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Linux run of this head (4b3b93f0ec), merged onto main 6475267b45 (local only). Ubuntu 26.04 LTS (kernel 7.0), Intel Core i7-8700, GCC 15.2.0, Ninja, Release, BUILD_TESTING=ON.
guest_kernel_fcntlpasses 3 of 3;public_headerspasses;ctest -R "fcntl|socket|net_|ioctl|guest_poll"50/50.- Negative control: same tree with
Udp.cppback tomain(test kept):guest_kernel_fcntlaborts 3 of 3.
Together with #13738, which also changes Udp.cpp, on main 2364892547: clean merge, full build passes, guest_kernel_fcntl and guest_poll pass 3 of 3, the socket/poll/fcntl subset 52/52 and public_headers pass.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
What
On a guest socket,
ioctl(fd, FIOCLEX)andioctl(fd, FIONCLEX)failed: with a null argument, the usual call for these_IOrequests, they returnedEFAULT, and with a pointerENOTTY. The socket path ofioctlchecks the argument and then accepts onlyFIONBIOandFIONREAD. On files and pipes,File::Ioctlalready handles both requests.FreeBSD 11.4's
kern_ioctlhandlesFIOCLEXandFIONCLEXfor every descriptor before calling the file'sfo_ioctl. It sets or clearsUF_EXCLOSEand returns 0, sosoo_ioctlnever sees them, andF_GETFDthen reports the flag.The socket path of
ioctl_nid_postfixnow handles both requests after the descriptor lookup and before the argument check. They insert into or erase fromcloseOnExecDescriptors, the set that the socketfcntlF_GETFDandF_SETFDalready read and write. The request is compared on its low 32 bits, asFile::Ioctldoes. The debt entry now lists the two requests withFIONBIOandFIONREAD.libSceNet sockets are not changed. Their
fcntlanswers onlyF_GETFLandF_SETFL, so they have no close-on-exec flag to set yet.Tested
Windows 11, WinLibs MinGW-w64 GCC 15.2.0, CMake + Ninja, Release,
-DBUILD_TESTING=ON, onmain7ceda13.guest_kernel_fcntlnow checks, on anAF_INETdatagram socket, thatFIOCLEXreturns 0 andF_GETFDreports 1, and thatFIONCLEXreturns 0 andF_GETFDreports 0. Each request is tried with a null argument and with a pointer.main'sUdp.cpp, the new checks fail (exit 0xc0000409 from the failedRequire). With this PR they pass.fcntl|ioctl|socket|recvmsg|sendfile|^guest_udp$|^guest_poll$pass.python tools/check_conventions.py --base origin/mainreports no errors. Its one warning is the length of theioctldebt entry, which was 1650 characters onmainand is 1672 here.Overlap (
tools/pr_overlap.py): it reports no conflict among the PRs it could compare. Of the rest, and the PRs that change the same function:ioctlrequests and answers unknown INET socket requests as measured on a PS5. It edits the sameioctldebt entry, so the two conflict there; the code merges cleanly. It keeps groupfrequests other thanFIONBIOandFIONREADon sockets unchanged, so it does not cover these two. Proposed order: fix(libkernel): reject empty ioctl requests and answer INET socket requests as measured on a PS5 #11219 first, then this PR rebases the debt sentence onto its wording, asked on fix(libkernel): reject empty ioctl requests and answer INET socket requests as measured on a PS5 #11219.if (!argument)check right after the lines added here, to let a void request through with a null argument. It still answers anything butFIONBIOandFIONREADwithENOTTY, so it does not implement these requests. It already conflicts withmain; whichever merges second keeps both changes.FIONBIObranch further down the same function, not these lines.Udp.cppor other entries oflibkernel.md.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)