Skip to content

test(libkernel): remove a leftover kernel_fcntl_probe.bin before guest_kernel_fcntl opens it - #9425

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
cadenasynyster:test/fcntl-remove-stale-probe
Oct 25, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
cadenasynyster:test/fcntl-remove-stale-probe

Conversation

@cadenasynyster

Copy link
Copy Markdown
Contributor

What

FileFlagsRoundTrip in guest_kernel_fcntl opens kernel_fcntl_probe.bin with O_CREAT but without O_TRUNC, and removes it only at the end, after every check has passed. A run that aborts earlier leaves the file behind. Every later run then starts from that file and fails at the FIONREAD check (readable == 3 after writing abcde and seeking to 2), because the file is longer than five bytes, until someone deletes it by hand. The test now removes the file before it opens it.

I hit this while repeating the negative control of #9256: that run aborts after writing abcde and ab, leaving 7 bytes, and main's guest_kernel_fcntl then failed until the file was deleted.

Tested

Windows 11 10.0.26300, WinLibs MinGW-w64 GCC 15.2.0 r7, CMake + Ninja, Release, -DBUILD_TESTING=ON, on main 0c6a5bd.

  • With a 7-byte kernel_fcntl_probe.bin (abcdeab) placed in the test's working directory, main's guest_kernel_fcntl fails (exit 0xc0000409; a temporary line probe, not in this PR, showed the failing check at line 241).
  • With this change and the same file placed again, the test passes 3 of 3, and the file is gone afterwards.
  • python tools/check_conventions.py --base origin/main passes.
  • Not tested: Linux and macOS (the same open without O_TRUNC applies there; std::remove is already used at the end of this function), the full CTest run, and a title. tools/pr_overlap.py was not run (it needs gh). fix(libkernel): change O_APPEND on a Windows file through fcntl #9256 edits the end of the same function, away from this line.

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: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@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.

The pre-run cleanup is a focused fix for the stale-file failure you reproduced, and the Windows negative-control/repeat runs are useful. Two repository follow-ups remain:

  1. The overlap report could not compare this test file with open #6044. I checked #6044's changed-file list: it moves core/libs/tests/GuestKernelFcntl.cpp to core/libs/tests/files/GuestKernelFcntl.cpp. This change edits the old path, so the landing order determines whether the cleanup line survives. Please coordinate with #6044's author and declare the agreed order on Depends on, then refresh the overlap check. #9256 also touches this test file at the end of the same test, as your description notes; preserve both changes when coordinating.

  2. This branch compares 417 commits behind current main, while the reported Windows test used 0c6a5bd59. Please rebase onto current main and rerun guest_kernel_fcntl there; no Linux, macOS, or current-base result is reported.

Hosted eligibility, conventions, intake, and labels pass; full Linux and Windows product jobs are skipped. I did not run the test locally because there is no matching checkout/build in the workspace.

…t_kernel_fcntl opens it

FileFlagsRoundTrip opens kernel_fcntl_probe.bin with O_CREAT but without O_TRUNC and removes it only when every check passes. A run that aborts leaves the file behind, and every later run then fails at the FIONREAD check because the file is longer than the five bytes the test writes. The test now removes the file before opening it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GtXavMkgCTCdmRpweviHcQ
@cadenasynyster
cadenasynyster force-pushed the test/fcntl-remove-stale-probe branch from 6c5d8fc to 84fc359 Compare October 22, 2026 19:10
@cadenasynyster

Copy link
Copy Markdown
Contributor Author

Rebased on d6e7ab4 (the branch was 417 commits behind). guest_kernel_fcntl passes on the rebased branch (one run), and the conventions check passes.

Order with #6044, which moves GuestKernelFcntl.cpp to files/: I will propose on #6044 that this PR lands first and #6044 rebases over it, and I will record any other order in Depends on once its author answers. #9256 edits the end of the same test; this change is at the start of FileFlagsRoundTrip, so the two are separate hunks.

Not run: Linux and macOS, repeated runs on the rebased branch, and the full ctest.

AI-assisted: yes

@cadenasynyster

Copy link
Copy Markdown
Contributor Author

Thanks. Rebased on d6e7ab4 (see my earlier comment). On #6044: it is open, conflicts with main and moves GuestKernelFcntl.cpp to core/libs/tests/files/, so I am not making this PR depend on it. Either order works: if #6044 lands first, this one-line cleanup follows the file in a rebase, and if this lands first, #6044's rename carries it. #9256 edits the end of the same test and this change is at the start of FileFlagsRoundTrip, so they are separate hunks. Depends on stays none.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed 84fc3594b8edd99c9468850381f1ec8df8e95f62 against author merge base d6e7ab42dfe191818721c6c5ad9aee4070b434aa.

Approve. No actionable findings in this single fixture-cleanup line.

The added remove targets the same fixed probe file created and already removed at the end of FileFlagsRoundTrip. It runs before the first open, so an interrupted earlier run cannot leave an existing file influencing creation-time flags. Ignoring a missing file is appropriate; the existing open/assertions still fail if cleanup or creation cannot proceed. Reviewed surrounding open/close ownership and later append checks.

Validation: exact head and baseline GuestKernelFcntl.cpp passed GCC C++23 syntax compilation. Conventions, whitespace and registration passed. This review covers the cleanup change; no linked guest fcntl runtime or hosted native pass is claimed (hosted native jobs were skipped).

@adriadam10 adriadam10 self-assigned this Oct 25, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 25, 2026 13:09

Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit 0119103 into boykopovar:main Oct 25, 2026
9 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked that the added std::remove runs before the first O_CREAT open (O_TRUNC is not set), so a stale probe file from an aborted run can no longer change FIONREAD; it matches the existing cleanup at the end of the test. The ordering concern with #6044 no longer applies, since #6044 is closed; merged.

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.

4 participants