[nodefs] Return real values for statvfs via __syscall_statfs64 #22631 #22932
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Carlos Garcia <[email protected]>
|
I tested the failing test locally by running |
Yup that test is just flaky, sadly. |
sbc100
reviewed
Nov 15, 2024
src/library_syscall.js
Outdated
| {{{ makeSetValue('buf', C_STRUCTS.statfs.f_fsid, '42', 'i32') }}}; | ||
| {{{ makeSetValue('buf', C_STRUCTS.statfs.f_flags, '2', 'i32') }}}; // ST_NOSUID | ||
| {{{ makeSetValue('buf', C_STRUCTS.statfs.f_namelen, '255', 'i32') }}}; | ||
| var defaults = FS.statfs(SYSCALLS.getStr(path)); |
There was a problem hiding this comment.
Can you rename this? Perhaps stats instead of `defaults? (Since they might be real values now).
There was a problem hiding this comment.
Stats sound better. I made the change in cf570be.
sbc100
approved these changes
Nov 15, 2024
sbc100
approved these changes
Nov 18, 2024
|
Is this ready to land? |
brandonpayton
added a commit
to WordPress/wordpress-playground
that referenced
this pull request
Mar 7, 2025
Note: This work is continued from a PR started under the WordPress GH org. There are a couple of reasons for this upgrade: 1. I am having trouble rebuilding php-wasm on my current system. References to older ubuntu images and emscripten versions [has given me trouble](#2038 (comment)). 2. We want [this Emscripten fix](emscripten-core/emscripten#22932) from @jeroenpf and @bgrgicak which was first included in Emscripten 3.1.73. This PR: - Bumps the ubuntu docker image version due to build errors related to missing image. Referencing a newer ubuntu image seemed to fix the "missing image" errors. - Bumps Emscripten version - Adjusts Playground-specific patches to Emscripten output since the latest breaks that patching. - CI (once we can run GH actions on A8C's GHE instance) --------- Co-authored-by: Brandon Payton <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was originally implemented by @jeroenpf in #22631.
I'm adding a fix for a broken test and had to do it in a new branch because I can't push to the original branch.
See #22607
statfs syscall functions are returning hardcoded default values which can in some cases lead to unexpected results.
This PR introduces a way for __syscall_statfs64 to return real values if the underlying filesystem supports this. For now, I've only implemented this for the NODEFS filesystem but we can expand this to other filesystems when it makes sense.
Additionally, in the previous defaults, ffree could be larger than files which is incorrect, this has also been addressed.
Ive added a test test/fs/test_nodefs_statfs.c which can be executed by running:
test/runner test_fs_nodefs_statfs
As I am a new contributor I'm happy to get feedback and make improvements where needed.