Skip to content
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

Refactor and combine all FileType structs in yanix #945

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Feb 15, 2020

This commit does a bit of everything: refactors bits here and there,
fixes a bug discovered in another #701, and combines all structs that
we used in yanix and wasi-common crates to represent file types
on *nix into one struct, yanix::file::FileType.

Up until now, in yanix, we've had two separate structs used to
represent file types on the host: yanix::dir::FileType and
yanix::file::SFlags (well, not quite, but that was its main use).
They both were used in different context (the former when parsing
dirent struct, and the latter when parsing stat struct), they
were C-compatible (as far as their representation goes), and as it
turns out, they shared possible enumeration values. This commit
combines them both into an idiomatic Rust enum with the caveat that
it is now not C-compatible, however, I couldn't find a single use
where that would actually matter, and even if it does in the future,
we can simply add appropriate impl methods.

The combine yanix::file::FileType struct can be constructed in two
ways: 1) either from stat.st_mode value (and while we're here,
now it's done correctly according to POSIX which fixes the bug mentioned
in VFS impl PR #701), or 2) from dirent.d_type value. Also, since we now
have one struct for representing both contexts, this cleans up nicely
a lot of duplicated code in host module.

cc @iximeow Hopefully, this fixes the bug you mentioned in #701!

This commit does a bit of everything: refactors bits here and there,
fixes a bug discovered in another bytecodealliance#701, and combines all structs that
we used in `yanix` and `wasi-common` crates to represent file types
on *nix into one struct, `yanix::file::FileType`.

Up until now, in `yanix`, we've had two separate structs used to
represent file types on the host: `yanix::dir::FileType` and
`yanix::file::SFlags` (well, not quite, but that was its main use).
They both were used in different context (the former when parsing
`dirent` struct, and the latter when parsing `stat` struct), they
were C-compatible (as far as their representation goes), and as it
turns out, they shared possible enumeration values. This commit
combines them both into an idiomatic Rust enum with the caveat that
it is now *not* C-compatible, however, I couldn't find a single use
where that would actually matter, and even if it does in the future,
we can simply add appropriate impl methods.

The combine `yanix::file::FileType` struct can be constructed in two
ways: 1) either from `stat.st_mode` value (and while we're here,
now it's done correctly according to POSIX which fixes the bug mentioned
in VFS impl PR bytecodealliance#701), or 2) from `dirent.d_type` value. Also, since we now
have one struct for representing both contexts, this cleans up nicely
a lot of duplicated code in `host` module.
@kubkon kubkon requested a review from sunfishcode February 15, 2020 21:23
@kubkon kubkon mentioned this pull request Feb 15, 2020
@kubkon
Copy link
Member Author

kubkon commented Feb 24, 2020

I'm gonna go ahead and merge this to not thwart progress in #701, but if anything is amiss, please feel free to reopen, revert, or file an issue.

@kubkon kubkon merged commit 4fe397e into bytecodealliance:master Feb 24, 2020
@kubkon kubkon deleted the yanix-filetype-cleanup branch February 24, 2020 14:18
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.

1 participant