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

Conversation

jeroenpf
Copy link

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.

@sbc100 sbc100 changed the title Return real values for statvfs via __syscall_statfs64 [nodefs] Return real values for statvfs via __syscall_statfs64 Sep 26, 2024
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this?

I wonder if we can/should make this working under NODERAWFS too? Or can that be a followup? I think maybe it would need to happen at the same time?

src/library_nodefs.js Show resolved Hide resolved
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?

test/fs/test_nodefs_statfs.c Outdated Show resolved Hide resolved
test/fs/test_nodefs_statfs.c Outdated Show resolved Hide resolved
test/test_core.py Outdated Show resolved Hide resolved
test/test_core.py Show resolved Hide resolved
src/library_fs.js Outdated Show resolved Hide resolved
@sbc100
Copy link
Collaborator

sbc100 commented Sep 26, 2024

Thanks for working on this?

I wonder if we can/should make this working under NODERAWFS too? Or can that be a followup? I think maybe it would need to happen at the same time?

Actually I think the NODERAWFS version can be a followup as it should just keep using the fake/default values up until that point.

@jeroenpf
Copy link
Author

Thanks for working on this?
I wonder if we can/should make this working under NODERAWFS too? Or can that be a followup? I think maybe it would need to happen at the same time?

Actually I think the NODERAWFS version can be a followup as it should just keep using the fake/default values up until that point.

I agree, all filesystems, besides NODEFS, should continue using the hardcoded values so we can implement the real values for the others in follow-ups.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 30, 2024

Looks like a test is failing:

======================================================================
FAIL [0.000s]: test_unistd_fstatfs (test_other.other)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
AssertionError: Unexpected difference:
  File "/usr/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/root/project/test/common.py", line 771, in resulting_test
    return func(self, *args)
  File "/root/project/test/common.py", line 384, in metafunc
    f(self, *args, **kwargs)
  File "/root/project/test/test_other.py", line 13502, in test_unistd_fstatfs
    self.do_run_in_out_file_test('unistd/fstatfs.c')
  File "/root/project/test/common.py", line 1777, in do_run_in_out_file_test
    output = self._build_and_run(srcfile, expected, **kwargs)
  File "/root/project/test/common.py", line 1815, in _build_and_run
    js_output = self.run_js(js_file, engine, args,
  File "/root/project/test/common.py", line 1410, in run_js
    self.fail('JS subprocess failed (%s): %s (expected=%s).  Output:\n%s' % (error.cmd, error.returncode, assert_returncode, ret))
  File "/usr/lib/python3.8/unittest/case.py", line 753, in fail
    raise self.failureException(msg)
AssertionError: JS subprocess failed (/root/emsdk/node/18.20.3_64bit/bin/node --stack-trace-limit=50 --trace-uncaught /tmp/emtest_22wft709/emscripten_test_other_pxlnnu_b/fstatfs.js): 1 (expected=0).  Output:
/tmp/emtest_22wft709/emscripten_test_other_pxlnnu_b/fstatfs.js:2596
        if (parent.node_ops.statfs) {
                   ^

TypeError: Cannot read properties of null (reading 'node_ops')

test/test_core.py Show resolved Hide resolved
@bgrgicak
Copy link

@sbc100 I've taken over this task from @jeroenpf to wrap the work.

I had to start a new PR to push my fix. When you have time could you please take a look at it?

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.

5 participants