Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

feat(preview1): implement core I/O functionality#175

Closed
rvolosatovs wants to merge 27 commits intobytecodealliance:mainfrom
rvolosatovs:feat/preview1-io
Closed

feat(preview1): implement core I/O functionality#175
rvolosatovs wants to merge 27 commits intobytecodealliance:mainfrom
rvolosatovs:feat/preview1-io

Conversation

@rvolosatovs
Copy link
Copy Markdown
Member

@rvolosatovs rvolosatovs commented May 22, 2023 •
edited
Loading

This includes, but is not limited to:

  • fd_advise
  • fd_close
  • fd_fdstat_get
  • fd_filestat_get
  • fd_filestat_set_size
  • fd_filestat_set_times
  • fd_prestat_dir_name
  • fd_prestat_get
  • fd_pread
  • fd_pwrite
  • fd_read
  • fd_renumber
  • fd_seek
  • fd_write
  • path_open
  • path_symlink
  • path_unlink_file
  • path_create_directory
  • path_remove_directory

Note, that there are a few differences with the adapter, most notably (there may be more, I am following the existing preview1 implementation in Wasmtime):

  • fd_pwrite uses stream-based I/O respecting the blocking flag, rather than relying on filesystem::write as is done in the existing adapter
  • fd_write and fd_pwrite return an error even when no data is to be written to invalid FDs, just list the "legacy" Wasmtime preview1 implementation, unlike the existing adapter (which just returns Ok(0) in that case). fd_read exhibits same behavior with empty buffers
  • fd_pwrite returns SPIPE errno on writes to stdout/stderr and BADF on writes to stdin, just like the "legacy" Wasmtime preview1 implementation, unlike the existing adapter
  • fd_filestat_set_times does not require a directory or a regular file, but rather works on any FD, including stdio, just like legacy Wasmtime preview1

@rvolosatovs
refactor(wasi): impl Debug on Table
d328987 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): instrument calls
8932d2d 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement core I/O functionality
3f36714 
This includes, but is not limited to:

- `fd_close`
- `fd_fdstat_get`
- `fd_prestat_dir_name`
- `fd_prestat_get`
- `fd_pwrite`
- `fd_renumber`
- `fd_write`
- `path_open`

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
test(preview1): do not expect panic on unsupported systems
917153d 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_symlink
471452b 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_unlink_file
2eb545d 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_create_directory
da2d0ec 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/preview1-io branch 2 times, most recently from 40bfdc1 to 1731ed0 Compare May 23, 2023 10:41
@rvolosatovs
feat(preview1): implement path_remove_directory
1731ed0 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_filestat_get
38e2ae3 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_seek
b0c82d1 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_filestat_set_size
1e78b9c 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_advise
392ab86 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_filestat_set_times
42e068f 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_allocate
0cb1132 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_datasync
441fca1 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_tell
38c9439 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_sync
c70b4ac 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_{,p}read
4f5b662 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_fdstat_set_flags
6daac38 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
refactor(preview1): rename get -> get_descriptor
99cf3e3 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement fd_fdstat_set_rights
5b759c0 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_filestat_get
cba9e66 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_filestat_set_times
398ab08 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_link
272f45c 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
refactor(preview1): simplify FD accessor error handling
36a7c2c 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/preview1-io branch 2 times, most recently from f99c71c to 58b0869 Compare May 23, 2023 16:23
@rvolosatovs
feat(preview1): implement path_rename
4bd0c95 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs
feat(preview1): implement path_readlink
58b0869 
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@pchickey
Copy link
Copy Markdown
Collaborator

pchickey commented May 23, 2023

This is really excellent work, I am excited to see this.

While you were on vacation, I moved all of the code in this repository over to the wasmtime repository (except sockets, which arent relevant here). Can you please change this PR to apply to the wasmtime_wasi::preview2::preview1 module in wasmtime, and the tests are now in wasmtime/crates/test-programs/tests/wasi-preview1-host-in-preview2.rs.

Thank you for finding all of these corner cases where the behavior diverged from the legacy implementation! I think that, rather than take the legacy as the spec, we should consider these all and figure out what the behavior really should be, and make all implementations comform. For each of these corner cases, can you please make a PR that asserts the desired behavior in with a new test in wasmtime/crates/test-programs/wasi-tests/src/bin/, and then we get every implementation aligned:

fd_pwrite uses stream-based I/O respecting the blocking flag, rather than relying on filesystem::write as is done in the existing adapter

I agree with this change. Lets upstream it in wasmtime/crates/wasi-preview1-component-adapter as well.

fd_write and fd_pwrite return an error even when no data is to be written to invalid FDs, just list the "legacy" Wasmtime preview1 implementation, unlike the existing adapter (which just returns Ok(0) in that case). fd_read exhibits same behavior with empty buffers

Also agreed. Lets upstream it.

fd_pwrite returns SPIPE errno on writes to stdout/stderr and BADF on writes to stdin, just like the "legacy" Wasmtime preview1 implementation, unlike the existing adapter

Lets do whatever behavior posix does for these cases.

fd_filestat_set_times does not require a directory or a regular file, but rather works on any FD, including stdio, just like legacy Wasmtime preview1

Same, lets align with posix here.

@pchickey
Copy link
Copy Markdown
Collaborator

pchickey commented May 23, 2023

Also, don't worry about leaving this granular commit history when you move things over to the wasmtime repo.

@rvolosatovs rvolosatovs mentioned this pull request May 23, 2023
Merged
@rvolosatovs
Copy link
Copy Markdown
Member Author

rvolosatovs commented May 23, 2023

Moved to bytecodealliance/wasmtime#6440

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rvolosatovs @pchickey