Repository navigation
fix(libkernel): drop the kqueue and knotes of a descriptor that dup2 replaces with a socket - #11445
MaxLikesCode wants to merge 1 commit into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed head 01679f8dbe3856b1f70d8dc0d8ffd10453c00788 against current main 9656d366be903e570594e0726f9bd86f2c32ddb0. The replacement notification is correctly after successful descriptor duplication and is safe for unregistered descriptors: EqueueDescriptorClosed removes a kqueue at that descriptor number and removes descriptor notes from all owning queues. The two regression cases cover replacing a kqueue with a socket and replacing a pipe watched by another kqueue. The latest main commit after the reported bf77a9e9cb base does not touch the changed files. Local tools/check_conventions.py --base upstream/main --head upstream/pr-11445 and git diff --check upstream/main...upstream/pr-11445 pass.
The PR reports the focused guest_kqueue red/green test and full Windows suite after rebasing; I did not independently run that Windows suite. Hosted eligibility, conventions, intake, labels and report checks pass. Build and product jobs were skipped by path policy. I found no blocking issue.
|
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.
|
|
Independent component qualification of exact head
This qualifies these two component suites, not the full suite, a retail title, macOS or PS5 console behavior. The Linux negative control is a baseline-library substitution, not an unmodified-main full build. I had independently reached the same implementation locally, but found this PR in the final overlap check and am contributing the qualification here rather than opening a duplicate. AI-assisted: yes. |
|
Thanks @B-Howell for the Windows and Linux runs and the baseline-library control on Linux. I added them to the Tested section. AI-assisted: yes (Claude Code) |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 01679f8d against 9656d366. No actionable findings.
GuestSockets::DuplicateTo notifies event queues only after replacement succeeds and after releasing the socket lock. That clears a kqueue registered at the target and removes notes from every owning queue. The regression covers replacing a pending timer kqueue with a socket and replacing a pipe watched by another kqueue, checking EBADF, no event, and ENOENT.
Validation: The exact-head Windows Release build succeeded; focused CTest passed 3 tests, 0 skipped, 0 failed. A pinned current-main control passed the same two descriptor tests. The author also reports a baseline failure with the previous Udp.cpp; I did not independently rerun that negative control. Local conventions and git diff --check passed. Hosted intake, eligibility, labels, conventions, and report checks passed; hosted product jobs were skipped by path policy.
This is local Windows x64 validation. I did not test Linux, macOS, console firmware, or a title.
|
Windows run of this head
AI-assisted: yes (Claude Code) |
|
Thanks @babyhuey for running CI's Windows selection on this head. I added it to the Tested section. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on branch currency: this PR is still based on 9656d366be, while upstream main is 801d5b9bc1. The exact changed paths (Udp.cpp and GuestKqueue.cpp) are unchanged on current main, and the submitted implementation/tests still look correct; however, the repository asks branches to stay up to date before merge. Please rebase onto current main and rerun the focused guest_kqueue and conventions checks. This supersedes the earlier approval until the current-main tree is checked.
|
macOS run of head Tree:
Same pattern as your Windows run and B-Howell's Linux control. AI-assisted: yes |
01679f8 to
56c1a45
Compare
|
@MarcZX10 rebased onto Thanks @Gonic0 for the macOS run with the AI-assisted: yes (Claude Code) |
|
Thanks for rebasing this PR onto The branch is still behind current I did not run a local build or test for this PR in this workspace. |
…replaces with a socket dup2 from a guest socket notified the event queues only when the replaced descriptor was itself a guest socket. A kqueue replaced this way stayed registered, so kevent on the number that now names the socket still returned the old queue's events, and knotes registered on a replaced pipe or file stayed attached. FreeBSD 11.4's kern_dup closes the replaced file through closefp, whose knote_fdclose drops every knote on the descriptor, as the other dup2 paths here already do. Fixes boykopovar#11429 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
56c1a45 to
dcdf9f3
Compare
|
@MarcZX10 rebased onto AI-assisted: yes (Claude Code) |
What
dup2from a guest socket now drops the kqueue and the knotes of the descriptor it replaces, as the pipe and file paths ofdup2already do.GuestSockets::DuplicateTo, which the socket branch ofdup2_nid_postfix(and sofcntlF_DUP2FD) uses, calledEqueueDescriptorClosedonly when the replaced descriptor was itself a guest socket. Two consequences, as #11429 reports:keventon the number that now names the socket still returned the old queue's events;FreeBSD 11.4's
kern_dupcloses the replaced file throughclosefp, whoseknote_fdclosedrops every knote on that descriptor;keventon a descriptor that is not a kqueue then fails withEBADF(kqueue_acquire), andEV_DELETEof a dropped knote withENOENT(kqueue_register).DuplicateTonow notifies the event queues after every successful replacement. Merged #3764 already givesdup2onto a kqueue these semantics for the other sources.Tested
Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, full Release build with
BUILD_TESTING=ON.guest_kqueuegainsSocketReplacesDescriptor:keventon that number must fail withEBADF;EV_DELETEof that knote must fail withENOENT.Udp.cppfrommain,guest_kqueueaborts 3 of 3. A local diagnostic build showed both cases failing:keventon the replaced kqueue returned 1 with the old timer event (ident 1,EVFILT_TIMER, the originaludata), andEV_DELETEof the replaced pipe's knote returned 0. With the change it passes 3 of 3.main035ac96 with this change: 2472 of 2475 pass. The 3 failures fail the same way onmainon this machine:guest_wide_formatting(fix(libc): convert the Windows time zone name from the ANSI code page to UTF-8 for tzname, %Z and tm_zone #10981, German code page, fixed by fix(libc): convert Windows time zone names to UTF-8 #10987),agc_host_import_cut_lifetime(agc_host_import_cut_lifetime fails on Windows since the merge of #9159 #10746) andagc_driver_depth_plane_partial_store(AGC: imported depth-plane partial-store test fails refresh-proof assertion on Windows (NVIDIA and AMD) #10664, fixed by fix(agc): collect upload generation after host import #10696).mainbf77a9e: the full suite passes 2473 of 2476 with the same 3 failures, andguest_kqueuepasses 3 of 3.python tools/check_conventions.py --base origin/mainpasses.python tools/pr_overlap.py --branch fix/dup2-socket-closes-kqueuecould not compare 12 conflicting PRs; I checked their diffs by hand. feat(libkernel): report open, read and close notes on watched files #7121 also adds adup2case toGuestKqueue.cpp(close notes on a replaced vnode), in another function; none of them changeDuplicateToor this test.guest_kqueueandguest_socket_descriptorspass 2 of 2 on Windows (MSYS2 UCRT64 GCC 16.2, Release) and on Linux (WSL Ubuntu, GCC 11, Release). On Linux, the same test executables with onlylibkernel.prxbuilt frombf77a9e9cb'sUdp.cppmakeguest_kqueueabort, whileguest_socket_descriptorsstill passes.main27ad724 with the test selection of CI's skipped Windows job (Windows 11, RTX 3080, WinLibs GCC 15.2, Release; comment): 1422 of the 2122 selected tests pass and none fail; the other 700 are skipped, as onmainthere.main801d5b9 with the open macOS build PRs (comment):guest_kqueuepasses 3 of 3 on that base and 3 of 3 with this change, and aborts 3 of 3 with this change's test but onlyUdp.cpprestored from the base.main2ee8e97 at @MarcZX10's request (head 56c1a45, was 01679f8); the change is unchanged.guest_kqueuepasses 3 of 3 andpython tools/check_conventions.py --base origin/mainpasses. The full suite on the same change rebased ontomainf2f759a, four commits earlier and none of them in libkernel, passes 2666 of 2667: onlyguest_wide_formattingfails, the same way as onmainhere (fix(libc): convert the Windows time zone name from the ANSI code page to UTF-8 for tzname, %Z and tm_zone #10981, fixed by fix(libc): convert Windows time zone names to UTF-8 #10987).main5454cd2 at @MarcZX10's request (head dcdf9f3, was 56c1a45), aftermainchangedUdp.cppin other functions; the change is unchanged. Full Release build:guest_kqueuepasses 3 of 3, and the full suite passes 3016 of 3018. The 2 failures fail the same way onmainon this machine:guest_wide_formatting(fix(libc): convert the Windows time zone name from the ANSI code page to UTF-8 for tzname, %Z and tm_zone #10981, German code page, fixed by fix(libc): convert Windows time zone names to UTF-8 #10987) andguest_freebsd_clocks(Tests: guest_freebsd_clocks aborts where the host zone is not UTC #12736, the host zone here is not UTC, fixed by test(libkernel): pin the zone before the kernel timezone check #12747).python tools/check_conventions.py --base origin/mainpasses.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