Skip to content

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

Merged
merged 4 commits into from
Mar 27, 2025
Merged

WASI: Implement fd_sync #180

merged 4 commits into from
Mar 27, 2025

Conversation

kkebo
Copy link
Contributor

@kkebo kkebo commented Mar 25, 2025

fixes #178

@@ -24,6 +24,15 @@ extension FdWASIFile {
)
}

func sync() throws {
guard accessMode.contains(.write) else {
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@kkebo kkebo Mar 26, 2025

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.

refs: https://github.com/wasmerio/wasmer/blob/aa2024fc684d9c205dff7df1adf12fe4676b1eda/lib/wasix/src/syscalls/wasi/fd_sync.rs#L20

Copy link
Contributor Author

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,

Copy link
Member

@kateinoigakukun kateinoigakukun Mar 27, 2025

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!

Copy link
Contributor Author

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.

34149ae

@kkebo kkebo changed the title WASI: Implement fd_sync except for Windows WASI: Implement fd_sync Mar 26, 2025
@kkebo kkebo marked this pull request as ready for review March 26, 2025 16:46
@kateinoigakukun kateinoigakukun merged commit f380094 into swiftwasm:main Mar 27, 2025
14 checks passed
@kkebo kkebo deleted the wasi-fd-sync branch March 27, 2025 17:12
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.

String.write(toFile:atomically:encoding:) fails even with atomically: false only on WasmKit
2 participants