Skip to content

Remove second argument to ErrnoError. NFC #21136

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Remove second argument to ErrnoError. NFC #21136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/library_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ FS.staticInit();` +
lookupNode(parent, name) {
var errCode = FS.mayLookup(parent);
if (errCode) {
throw new FS.ErrnoError(errCode, parent);
throw new FS.ErrnoError(errCode);
}
var hash = FS.hashName(parent.id, name);
#if CASE_INSENSITIVE_FS
Expand Down Expand Up @@ -316,6 +316,7 @@ FS.staticInit();` +
return 0;
},
mayLookup(dir) {
if (!FS.isDir(dir.mode)) return {{{ cDefs.ENOTDIR }}};
var errCode = FS.nodePermissions(dir, 'x');
if (errCode) return errCode;
if (!dir.node_ops.lookup) return {{{ cDefs.EACCES }}};
Expand Down Expand Up @@ -1408,15 +1409,14 @@ FS.staticInit();` +
},
ensureErrnoError() {
if (FS.ErrnoError) return;
FS.ErrnoError = /** @this{Object} */ function ErrnoError(errno, node) {
FS.ErrnoError = /** @this{Object} */ function ErrnoError(errno) {
// We set the `name` property to be able to identify `FS.ErrnoError`
// - the `name` is a standard ECMA-262 property of error objects. Kind of good to have it anyway.
// - when using PROXYFS, an error can come from an underlying FS
// as different FS objects have their own FS.ErrnoError each,
// the test `err instanceof FS.ErrnoError` won't detect an error coming from another filesystem, causing bugs.
// we'll use the reliable test `err.name == "ErrnoError"` instead
this.name = 'ErrnoError';
this.node = node;
this.setErrno = /** @this{Object} */ function(errno) {
this.errno = errno;
#if ASSERTIONS
Expand Down
10 changes: 1 addition & 9 deletions src/library_syscall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,7 @@ var SyscallsLibrary = {
},

doStat(func, path, buf) {
try {
var stat = func(path);
} catch (e) {
if (e && e.node && PATH.normalize(path) !== PATH.normalize(FS.getPath(e.node))) {
// an error occurred while trying to look up the path; we should just report ENOTDIR
return -{{{ cDefs.ENOTDIR }}};
}
throw e;
}
var stat = func(path);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a significant functional change? Even with the change to mayLookup I can't figure out why this is NFC. Can you please elaborate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC this change will not be observable to the callers of stat. We have tests that cover this and all the tests pass so my claim is that this is just an internal refactor.

AFAICT try/catch here was in place in order to rewrite the incorrect errno returned my mayLookup (EACCES) to the correct one (ENOTDIR). My change to mayLookup fixes the errno the it returns and therefore obviates the need for this additional try/catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For context this cleanup is part of a sequence of changes to cleanup and remove the errno usage in our JS library. This is just the first step and it seems like right more to me.

Copy link
Member

Choose a reason for hiding this comment

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

We do have stat tests, but the interesting logic e.node && PATH.normalize(path) !== PATH.normalize(FS.getPath(e.node) might not be tested. It was added here without a test:

6bac5f5

for which I take full responsibility 😄 But, yeah, I understand your explanation for mayLookup which sounds like it might handle this, and I verified other.test_stat_fail_alongtheway fails with only part of the patch, so looks like that's covered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly yes. The test_stat_fail_alongtheway is the test that failed on the initial version of this PR. I used it to figure out what was going on here and find this fix.

{{{ makeSetValue('buf', C_STRUCTS.stat.st_dev, 'stat.dev', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.stat.st_mode, 'stat.mode', 'i32') }}};
{{{ makeSetValue('buf', C_STRUCTS.stat.st_nlink, 'stat.nlink', SIZE_TYPE) }}};
Expand Down
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors1.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
9936
9932
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors1.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24683
24693
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors2.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
9921
9915
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors2.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24651
24661
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
11015
11010
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
28573
28583
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except_wasm.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
9903
9899
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except_wasm.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24576
24586
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_mangle.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
11021
11016
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_mangle.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
28573
28583
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_noexcept.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
9936
9932
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_noexcept.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24683
24693
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_files_js_fs.gzsize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7874
7871
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_files_js_fs.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19705
19715