-
Notifications
You must be signed in to change notification settings - Fork 13
WASI: Implement fd_sync
#180
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
Conversation
Sources/WASI/Platform/File.swift
Outdated
@@ -24,6 +24,15 @@ extension FdWASIFile { | |||
) | |||
} | |||
|
|||
func sync() throws { | |||
guard accessMode.contains(.write) else { |
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.
I'd like check if other WASI implementations require write access mode for fd_sync just in case.
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.
I, too, wondered what should I do here, but I read the following documentation, so I put this.
If the argument is a read-only file descriptor, this function fails with EBADF on some platforms: AIX 7.2, Cygwin 2.9.
https://www.gnu.org/software/gnulib/manual/html_node/fsync.html
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.
Write access mode was probably not required.
- wasmtime
- https://github.com/bytecodealliance/wasmtime/blob/5eef20efd479b0a05bb18bb343b207cd23463584/crates/wasi/src/preview1.rs#L1874
- https://github.com/bytecodealliance/wasmtime/blob/5eef20efd479b0a05bb18bb343b207cd23463584/crates/wasi/src/host/filesystem.rs#L347
- https://github.com/rust-lang/rust/blob/f1bc669636023c8643602431791c7f26e5a6edef/library/std/src/fs.rs#L592
- https://github.com/rust-lang/rust/blob/f1bc669636023c8643602431791c7f26e5a6edef/library/std/src/sys/fs/unix.rs#L1204
- wasmer
- WAMR
- https://github.com/bytecodealliance/wasm-micro-runtime/blob/4a177416702da053761fbb33282b9acef910c974/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c#L621
- https://github.com/bytecodealliance/wasm-micro-runtime/blob/4a177416702da053761fbb33282b9acef910c974/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c#L1120
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.
Instead, I think we should add .sync
to FileAccessMode
and check if the file descriptor has it instead of .write
.
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.
My idea is like this. What do you think about it?
diff --git a/Sources/WASI/FileSystem.swift b/Sources/WASI/FileSystem.swift
index 587f5da..65371c3 100644
--- a/Sources/WASI/FileSystem.swift
+++ b/Sources/WASI/FileSystem.swift
@@ -4,6 +4,7 @@ struct FileAccessMode: OptionSet {
let rawValue: UInt32
static let read = FileAccessMode(rawValue: 1)
static let write = FileAccessMode(rawValue: 1 << 1)
+ static let sync = FileAccessMode(rawValue: 1 << 2) // Is this right?
}
protocol WASIEntry {
diff --git a/Sources/WASI/Platform/File.swift b/Sources/WASI/Platform/File.swift
index 6a3d697..a28b044 100644
--- a/Sources/WASI/Platform/File.swift
+++ b/Sources/WASI/Platform/File.swift
@@ -17,6 +17,9 @@ extension FdWASIFile {
if accessMode.contains(.write) {
fsRightsBase.insert(.FD_WRITE)
}
+ if accessMode.contains(.sync) {
+ fsRightsBase.insert(.FD_SYNC)
+ }
return try WASIAbi.FdStat(
fsFileType: self.fileType(),
fsFlags: self.status(),
@@ -25,8 +28,8 @@ extension FdWASIFile {
}
func sync() throws {
- guard accessMode.contains(.write) else {
- throw WASIAbi.Errno.EBADF
+ guard accessMode.contains(.sync) else {
+ throw WASIAbi.Errno.EACCES
}
try WASIAbi.Errno.translatingPlatformErrno {
try fd.sync()
diff --git a/Sources/WASI/WASI.swift b/Sources/WASI/WASI.swift
index cbffe02..c80b22a 100644
--- a/Sources/WASI/WASI.swift
+++ b/Sources/WASI/WASI.swift
@@ -1780,6 +1780,9 @@ public class WASIBridgeToHost: WASI {
if fsRightsBase.contains(.FD_WRITE) {
accessMode.insert(.write)
}
+ if fsRightsBase.contains(.FD_SYNC) {
+ accessMode.insert(.sync)
+ }
let hostFd = try dirEntry.openFile(
symlinkFollow: dirFlags.contains(.SYMLINK_FOLLOW),
path: path, oflags: oflags, accessMode: accessMode,
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.
The rights
concept has been removed from the wasip2, and most of WASI implementations don't respect it, so I don't think we need to care about it. Also, FD_SYNC
is something to allow performing fd_sync
for the fd but not something like O_SYNC
, which opens the fd with "sync mode".
Thus, I think just removing the write check is enough here, thank you for your detailed investigation!
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.
Thank you. I agree with you. I just pushed the commit.
fd_sync
except for Windowsfd_sync
fixes #178