Skip to content

[WasmFS] Implement fdatasync #16675

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

Merged
merged 2 commits into from
Apr 12, 2022
Merged

[WasmFS] Implement fdatasync #16675

merged 2 commits into from
Apr 12, 2022

Conversation

tlively
Copy link
Member

@tlively tlively commented Apr 8, 2022

As an alias for __wasi_fd_sync (i.e. fsync). This flushes data more
conservatively than necessary, but it's not worth maintaining a separate code
path to avoid flushing file metadata right now.

Resolves #16672.

As an alias for `__wasi_fd_sync` (i.e. `fsync`). This flushes data more
conservatively than necessary, but it's not worth maintaining a separate code
path to avoid flushing file metadata right now.
@tlively tlively requested a review from kripken April 8, 2022 00:05
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm % testing

Is the new test because the existing test that covers this (unistd/misc.c) also tests other stuff we don't pass? Or because we want more fine-grained testing?

The existing test also checks for an error so if we want a new test we should add that here too.

If this test is temporary please add a TODO to remove it. If it's meant to be permanent then let's remove the relevant lines from the existing test?

@kripken
Copy link
Member

kripken commented Apr 8, 2022

Oh, the new test is not enough to replace the old because the new one is wasmfs-only?

@tlively
Copy link
Member Author

tlively commented Apr 8, 2022

I'll add a TODO to remove this test in favor of unistd/misc.c and make it WasmFS-only since there's no reason to run this test on the classic FS.

@tlively tlively enabled auto-merge (squash) April 12, 2022 00:10
@tlively tlively disabled auto-merge April 12, 2022 02:06
@tlively
Copy link
Member Author

tlively commented Apr 12, 2022

Merging given previous LGTM.

@tlively tlively merged commit 3f94742 into main Apr 12, 2022
@tlively tlively deleted the wasmfs-sync branch April 12, 2022 02:07
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.

WasmFS: fdatasync
2 participants