Skip to content

Improve wasmfs API doc comments and return types #16267

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 4 commits into from
Feb 15, 2022
Merged

Improve wasmfs API doc comments and return types #16267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7867dc4
Improve wasmfs API doc comments and return types
tlively Feb 12, 2022
35c1228
Add static_assert guarding against conversion
tlively Feb 14, 2022
096d3f8
Fix assertions, add error docs
tlively Feb 14, 2022
f859b10
TODO
tlively Feb 14, 2022
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
10 changes: 7 additions & 3 deletions system/include/emscripten/wasmfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@ backend_t wasmfs_get_backend_by_path(char* path);
// Obtains the backend_t of a specified fd.
backend_t wasmfs_get_backend_by_fd(int fd);

// Creates a new file in the new file system under a specific backend.
uint32_t wasmfs_create_file(char* pathname, mode_t mode, backend_t backend);
// Creates and opens a new file in the new file system under a specific backend.
// Returns the file descriptor for the new file like `open`. Returns a negative
// value on error. TODO: It might be worth returning a more specialized type
// like __wasi_fd_t here.
int wasmfs_create_file(char* pathname, mode_t mode, backend_t backend);

// Creates a new directory in the new file system under a specific backend.
long wasmfs_create_directory(char* path, long mode, backend_t backend);
// Returns 0 on success like `mkdir`, or a negative value on error.
int wasmfs_create_directory(char* path, long mode, backend_t backend);

// Backend creation

Expand Down
8 changes: 6 additions & 2 deletions system/lib/wasmfs/syscalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,9 @@ static __wasi_fd_t doOpen(char* pathname,

// This function is exposed to users and allows users to create a file in a
// specific backend. An fd to an open file is returned.
__wasi_fd_t wasmfs_create_file(char* pathname, mode_t mode, backend_t backend) {
int wasmfs_create_file(char* pathname, mode_t mode, backend_t backend) {
Copy link
Member

@kripken kripken Feb 14, 2022

Choose a reason for hiding this comment

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

isn't it better to have the more explicit type here? if it doesn't match another location we'd get a link error actually, and not a silent cast. But a static assert might be better there I guess?

Copy link
Member Author

@tlively tlively Feb 14, 2022

Choose a reason for hiding this comment

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

The problem is that __wasi_fd_t is not meant to be a user-facing type, AFAICT. We certainly don't expose it via our other system libraries, at least.

Copy link
Member

@kripken kripken Feb 14, 2022

Choose a reason for hiding this comment

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

Thinking about this some more, I'm not sure what is best. Musl syscalls don't expose __wasi_fd_t, which is I think why you feel it should not be exposed. But the public WasmFS API could. Just like the WASI API does, for example.

I think it would be better to do so. It is the proper type, and more specific than int, and could be changed in the future easily (unlike int, which grepping for will not be useful).

OTOH, perhaps we could define a new type wasmfs_fd_t and use that in the WasmFS public API, so we don't depend on WASI here.

Overall I don't feel strongly, but I do think plain int is the least good option. I'd be fine with a TODO to investigate this later though.

Copy link
Member Author

@tlively tlively Feb 14, 2022

Choose a reason for hiding this comment

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

My primary motivation is to get the API to be as similar as possible to open, which also returns an int. I agree that using int has downsides that a more specialized type wouldn't have, though. I'll keep int for now and add a TODO, as you suggest. (I think that's what you had in mind, or did you mean switching back to __wasi_fd_t with a TODO?)

static_assert(std::is_same_v<decltype(doOpen(0, 0, 0, 0)), unsigned int>,
"unexpected conversion from result of doOpen to int");
return doOpen(pathname, O_CREAT, mode, backend);
}

Expand Down Expand Up @@ -480,7 +482,9 @@ static long doMkdir(char* path, long mode, backend_t backend = NullBackend) {

// This function is exposed to users and allows users to specify a particular
// backend that a directory should be created within.
long wasmfs_create_directory(char* path, long mode, backend_t backend) {
int wasmfs_create_directory(char* path, long mode, backend_t backend) {
static_assert(std::is_same_v<decltype(doMkdir(0, 0, 0)), long>,
"unexpected conversion from result of doMkdir to int");
return doMkdir(path, mode, backend);
}

Expand Down