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.
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
initial work for fs api forwarding #49
initial work for fs api forwarding #49
Changes from 3 commits
98b673e
e655e67
a5c3a69
6568a35
237a443
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan to implement those in a future PR? I do see a
syscall.Seek
for the Read endpoint for position.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was mainly a carry over from the original wasm_exec.js write
https://github.com/golang/go/blob/451e4727ec825a7ce6f6e6f82761ff90c33fec83/misc/wasm/wasm_exec.js#L27-L34
The read endpoint seek was implemented due to the coverage tool performing reads with offsets.
i.e. coverage metadata generation was broke without it.
I'll look and see how feasible it is to implement it here also.syscall.Seek
is now also used on theWrite
api.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we can do
make([]string, 0, len(entries))
as a further optimization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to be ENOSYS? Also, we seem to be completely gobbling up the actual
err
. Could we pass that down or log that somewhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENOSYS was the error code previously used in the wasm_exec.js for all unimplemented file system calls.
I think it still makes sense to use it as a generic "not implemented" error with the following rationale:
For each of the different platforms: linux, mac, and windows, the various
syscall.Xxxx
are handled differently, have different error codes, and for windows, are not POSIX at all. It would be quite a task to sort out what the appropriate error code would be based on the current running platform. ENOENT ( effectively file not found ) was the most important one to get right.I'll see about adding logging for the
err
in doError and updating all callers to pass in the error for logging.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks.