Skip to content

[WasmFS] Catch and report read and write errors in OPFS#17770

Merged
tlively merged 1 commit intomainfrom
wasmfs-read-write-errors
Sep 1, 2022
Merged

[WasmFS] Catch and report read and write errors in OPFS#17770
tlively merged 1 commit intomainfrom
wasmfs-read-write-errors

Conversation

@tlively
Copy link
Member

@tlively tlively commented Aug 31, 2022

Reading and writing with invalid offsets was the most convenient error to test
and produces an EINVAL error. In principle other kinds of errors are also
possible, but for now they produce EINVAL as well.

return accessHandle.read(data, {at: pos});
} catch (e) {
return -1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, weren't we saying that errPtr is the simpler thing, than to overload the return values?

And specifically, overloading it here means we are limited to 2GB, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out using negative return values was already the established pattern in this file, but maybe we should update it everywhere to use error pointers. Notably we can use the return value directly here because this is not an async function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, there is a TODO in opfs_backend.cpp about using an i64 here, which would eliminate the 2GB limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also see #17774, which tries to make the existing error handling more uniform and robust.

Base automatically changed from wasmfs-setsize-errors to main September 1, 2022 00:04
Reading and writing with invalid offsets was the most convenient error to test
and produces an EINVAL error. In principle other kinds of errors are also
possible, but for now they produce EINVAL as well.
@tlively
Copy link
Member Author

tlively commented Sep 1, 2022

@kripken, note that #17774 is stacked on top of this change so it would be easiest to land this first.

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.

2 participants