Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nodefs] Return real values for statvfs via __syscall_statfs64 #22631

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
25 changes: 25 additions & 0 deletions src/library_fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,31 @@ FS.staticInit();
}
return parent.node_ops.mknod(parent, name, mode, dev);
},
statfs(path) {

// NOTE: None of the defaults here are true. We're just returning safe and
// sane values.
var defaults = {
bsize: 4096,
frsize: 4096,
blocks: 1e6,
bfree: 5e5,
bavail: 5e5,
files: FS.nextInode,
// Free inodes can not be larger than total inodes so calculate a reasonable value here
ffree: () => Math.max(0, Math.floor(this.files * 0.9) - this.files),
fsid: 42,
flags: 2,
namelen: 255,
};

var parent = FS.lookupPath(path, {follow: true}).node;
if (typeof parent.node_ops?.statfs !== 'function') {
return defaults;
}

return { ...defaults, ...parent.node_ops.statfs(parent.mount.opts.root) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

var rtn = {
   ... // defaults
}
var parent = FS.lookupPath(path, {follow: true}).node;
if (typeof parent.node_ops?.statfs) {
   Object.assign(rtn, parent.node_ops.statfs(parent.mount.opts.root));
}
return rtn;

Also, so we really need to merge the defaults with the actual results? Are we expecting backends to only paritally fill the out the information?

Copy link
Author

Choose a reason for hiding this comment

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

I've applied the suggested changes.

The idea behind merging the defaults it to ensure that all the required fields are always set. In the case of Node, statfs does not return all of the information:

StatFs {
  type: 1397114950,
  bsize: 4096,
  blocks: 121938943,
  bfree: 61058895,
  bavail: 61058895,
  files: 999,
  ffree: 1000000
} 

We could add the missing fields in the node_ops.statfs but it seemed appropriate to have the FS class ensure a valid response. Im happy to change it though, what do you think?

},
// helpers to create specific types of nodes
create(path, mode) {
mode = mode !== undefined ? mode : 438 /* 0666 */;
Expand Down
7 changes: 7 additions & 0 deletions src/library_nodefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,13 @@ addToLibrary({
var path = NODEFS.realPath(node);
return NODEFS.tryFSOperation(() => fs.readlinkSync(path));
},
statfs(path) {
var stats = NODEFS.tryFSOperation(() => fs.statfsSync(path));
jeroenpf marked this conversation as resolved.
Show resolved Hide resolved
return {
...stats,
frsize: stats.bsize
};
}
},
stream_ops: {
open(stream) {
Expand Down
24 changes: 11 additions & 13 deletions src/library_syscall.js
Original file line number Diff line number Diff line change
Expand Up @@ -802,22 +802,20 @@ var SyscallsLibrary = {
},

__syscall_statfs64: (path, size, buf) => {
path = SYSCALLS.getStr(path);
#if ASSERTIONS
assert(size === {{{ C_STRUCTS.statfs.__size__ }}});
#endif
// NOTE: None of the constants here are true. We're just returning safe and
// sane values.
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bsize, '4096', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_frsize, '4096', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_blocks, '1000000', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bfree, '500000', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bavail, '500000', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_files, 'FS.nextInode', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_ffree, '1000000', 'i32') }}};
{{{ 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));
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bsize, 'defaults.bsize', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_frsize, 'defaults.bsize', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_blocks, 'defaults.blocks', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bfree, 'defaults.bfree', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_bavail, 'defaults.bavail', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_files, 'defaults.files', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_ffree, 'defaults.ffree', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_fsid, 'defaults.fsid', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_flags, 'defaults.flags', 'i32') }}}; // ST_NOSUID
{{{ makeSetValue('buf', C_STRUCTS.statfs.f_namelen, 'defaults.namelen', 'i32') }}};
return 0;
},
__syscall_fstatfs64__deps: ['__syscall_statfs64'],
Expand Down
3 changes: 3 additions & 0 deletions test/core/test_statvfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
#include <stdio.h>
#include <errno.h>
#include <sys/statvfs.h>
#include <sys/types.h>
#include <sys/stat.h>

int main() {
struct statvfs s;

mkdir("/test", S_IRWXU | S_IRWXG | S_IRWXO);
printf("result: %d\n", statvfs("/test", &s));
printf("errno: %d\n", errno);

Expand Down
37 changes: 37 additions & 0 deletions test/fs/test_nodefs_statfs.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#include <assert.h>
#include <stdio.h>
#include <sys/statvfs.h>
#include <emscripten.h>

void test_statfs(const char *path) {
printf("Testing statfs for path: %s\n", path);
struct statvfs st;
int result = statvfs(path, &st);

assert(result == 0 && "statvfs should succeed");



jeroenpf marked this conversation as resolved.
Show resolved Hide resolved
// Basic sanity checks
assert(st.f_bsize > 0 && "Block size should be positive");
assert(st.f_blocks > 0 && "Total blocks should be positive");
assert(st.f_bfree <= st.f_blocks && "Free blocks should not exceed total blocks");
assert(st.f_bavail <= st.f_bfree && "Available blocks should not exceed free blocks");
assert(st.f_files > 0 && "Total inodes should be positive");
assert(st.f_ffree <= st.f_files && "Free inodes should not exceed total inodes");
}

int main() {
// Test the root filesystem (which should be MEMFS by default)
test_statfs("/");
jeroenpf marked this conversation as resolved.
Show resolved Hide resolved

// Mount NODEFS and test it
EM_ASM(
FS.mkdir('/working');
FS.mount(NODEFS, { root: '.' }, '/working');
);
jeroenpf marked this conversation as resolved.
Show resolved Hide resolved

test_statfs("/working");

puts("success");
}
9 changes: 9 additions & 0 deletions test/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -5779,6 +5779,15 @@ def test_fs_nodefs_readdir(self):
self.emcc_args += ['-lnodefs.js']
self.do_runf('fs/test_nodefs_readdir.c', 'success')

@requires_node
jeroenpf marked this conversation as resolved.
Show resolved Hide resolved
def test_fs_nodefs_statfs(self):
# externally setup an existing folder structure: existing/a
if self.get_setting('WASMFS'):
self.set_setting('FORCE_FILESYSTEM')
os.makedirs(os.path.join(self.working_dir, 'existing', 'a'))
jeroenpf marked this conversation as resolved.
Show resolved Hide resolved
self.emcc_args += ['-lnodefs.js']
self.do_runf('fs/test_nodefs_statfs.c', 'success')

@no_windows('no symlink support on windows')
@requires_node
def test_fs_noderawfs_nofollow(self):
Expand Down