Repository navigation
Conversation
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact PR head 4080a15f229892ce756c1e53670eee4d8795cff7 against current main and the overlap notes. The change reuses the directory-handle path only for Windows read-mode freopen calls onto directories; write/update modes still fail, fopen retains its existing path, and the standard-stream limitation is recorded as debt. The regression checks directory fstat, EISDIR plus the error indicator on read, reopening the stream onto a regular file afterward, re mode, and update-mode failure.
The author reports the focused Windows tests on this head and a negative control against main; the required hosted checks pass. Linux product tests were not run by me, and the hosted Linux/Windows full-build jobs are skipped under repository policy. The #10251 overlap is documentation/measurement, while #9763 concerns adjacent closed-stream handling. I found no blocking issue.
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 11d6efa and FreeBSD 11.4 lib/libc/stdio/freopen.c:
- This still merges cleanly with main.
FileStream::Reopenon main already takes a(path, mode)open callback and already forgets the libkernel directory descriptor (std::exchange(directory, false)), so reusingOpenDirectoryHandlethere fits well, and the split fromOpenDirectoryStreamkeeps fopen's behaviour unchanged. - Checking
is_directorybeforeOpenNativegives the same result as fopen's "EACCES, then directory" order, since_wfopennever opens a directory on Windows. - Optional: the fopen loop checks on Windows that
openreuses the directory's descriptor number afterfclose. Doing the same afterfreopen(listed, "rb")and after the finalfclosewould cover the forget path inReopentoo. - Nit: the "fopen: libkernel has not registered…" message is now reached from freopen as well.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@harisibrar1 Thanks, I did both in 1e32d23.
|
harisibrar1
left a comment
There was a problem hiding this comment.
Re-checked head 1e32d23 against main 6cd36e6:
- After
freopen(listed, "rb"), the test now checks thatsceKernelFstatreports a regular file (GuestFilesystem.cpp:695). After the final reopen onto the directory andfclose, it checks on Windows thatopenreuses the same descriptor (:697-706). Together these cover the forget path inReopen. - The error message now starts with
OpenDirectoryHandle:(FileAndHeap.cpp:331), so it's right for both fopen and freopen. - It still merges cleanly into main. Thanks for the extra coverage, LGTM.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
What
On Windows,
freopenof a stream onto a directory in a read mode (r,rb,re) failed withEACCES, which is the gap #8550 left open. FreeBSD 11freopenopens the path with_open(freopen.c), andvn_open_vnoderefuses a directory only forFWRITEorO_TRUNC. #10251 measured on a PS5 (firmware 13.60) thatfreopenonto a directory in a read mode succeeds there too.freopennow takes the same pathfopentakes since #8550. For a read mode and a path that is a directory, it reopens the stream throughOpenDirectoryHandle, which opens a libkernel directory descriptor and wraps it in a hostFILE. The stream is then marked as a directory stream.OpenDirectoryHandleis the first half of the oldOpenDirectoryStream, split out so thatFileStream::Reopencan call it the way it callsOpenNative.fopenkeeps callingOpenDirectoryStream, which still uses it.The stream behaves as a directory stream from
fopendoes: a read fails withEISDIRand sets the error indicator,fstaton itsfilenoreports a directory, andfclose, or a secondfreopen, removes the descriptor from libkernel's table. A write or update mode keeps failing withEISDIR.stdin,stdoutandstderrkeep their host descriptor numbers 0 to 2 acrossfreopen, so they cannot hold a libkernel directory descriptor.freopenof one of them onto a directory in a read mode now throws instead of failing withEACCES. The debt entry says so, and its old sentence aboutEACCESis removed.Test:
guest_filesystemopens a file withfopenand reopens it onto a directory withr.fstatmust report a directory, andfreadmust fail withEISDIRand set the error indicator. It then reopens the stream onto the file withrb.fstatof its descriptor must report a regular file, so the directory registration is gone, and the stream reads the file's data. It reopens the stream onto the directory withreand closes it. On Windows,openmust then return the same descriptor number, as thefopenloop already checks. A reopen onto a directory withr+must fail withEISDIR.Tested
Windows 11, WinLibs MinGW-w64 GCC 15.2.0 r7, CMake + Ninja, Release,
-DBUILD_TESTING=ON, onmaina23f8fc.main's libc, the newguest_filesystemchecks fail (exit 0xc0000409 from the failedRequire). With this PR it passes.guest_filesystem,file_streams,guest_freopen_host_standard,guest_fopen_long_host_path,guest_directory*andguest_file_lock_*pass.guest_filesystem,file_streamsandguest_freopen_host_standardpass again on head 1e32d23, after the review follow-up.python tools/check_conventions.py --base origin/mainreports no errors.freopenalready opened a directory and the new checks run unchanged. No title or homebrew run.Overlap (
tools/pr_overlap.py):EACCESsentence this PR removes. Whichever merges second needs a one-line rebase of the entry.freopenof a closed stream next to this one, and changes the closed-stream checks at the top offreopenandFileStream::Reopen, not the lines changed here. A small rebase of the debt file at most.FileAndHeap.cpp, and test: organize library tests and label CTest suites #6044, fix(libkernel): support lstat on long Windows paths #7202, feat(libkernel): enumerate archived app directories #8520 and fix(libkernel): report lookup errors when creating through a trailing slash #10319 touchGuestFilesystem.cpp. None of them changesfreopenor directory streams.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)