Skip to content

Commit

Permalink
preview1: Update semantics of seeking on appending files (#7586)
Browse files Browse the repository at this point in the history
This commit updates the semantics of `fd_{seek,tell}` on preview1 to
match native Unix when used with appending files. On Unix `write` claims
to always update the file position pointer to the end of the file, so
this commit implements that instead of the previous logic of ignoring
the position update for appending files. This currently requires an
extra roundtrip via `stat` to figure out the size of the file, but for
now that seems to be the best that can be done.

Closes #7583
  • Loading branch information
alexcrichton authored Nov 27, 2023
1 parent 9da24f3 commit 55ac268
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 7 deletions.
47 changes: 46 additions & 1 deletion crates/test-programs/src/bin/preview1_file_seek_tell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,48 @@ unsafe fn test_file_seek_tell(dir_fd: wasi::Fd) {
wasi::fd_close(file_fd).expect("closing a file");
wasi::path_unlink_file(dir_fd, "file").expect("deleting a file");
}

// Test that when a file is opened with `O_APPEND` that acquiring the current
// position indicates the end of the file.
unsafe fn seek_and_o_append(dir_fd: wasi::Fd) {
let path = "file2";
let file_fd = wasi::path_open(
dir_fd,
0,
path,
wasi::OFLAGS_CREAT,
wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE,
0,
wasi::FDFLAGS_APPEND,
)
.expect("opening a file");
assert!(
file_fd > libc::STDERR_FILENO as wasi::Fd,
"file descriptor range check",
);

let mut offset = wasi::fd_seek(file_fd, 0, wasi::WHENCE_CUR).unwrap();
assert_eq!(offset, 0);
offset = wasi::fd_tell(file_fd).unwrap();
assert_eq!(offset, 0);

let data = &[0u8; 100];
let iov = wasi::Ciovec {
buf: data.as_ptr() as *const _,
buf_len: data.len(),
};
let nwritten = wasi::fd_write(file_fd, &[iov]).unwrap();
assert_eq!(nwritten, 100);

let mut offset = wasi::fd_seek(file_fd, 0, wasi::WHENCE_CUR).unwrap();
assert_eq!(offset, 100);
offset = wasi::fd_tell(file_fd).unwrap();
assert_eq!(offset, 100);

wasi::fd_close(file_fd).unwrap();
wasi::path_unlink_file(dir_fd, path).unwrap();
}

fn main() {
let mut args = env::args();
let prog = args.next().unwrap();
Expand All @@ -99,5 +141,8 @@ fn main() {
};

// Run the tests.
unsafe { test_file_seek_tell(dir_fd) }
unsafe {
test_file_seek_tell(dir_fd);
seek_and_o_append(dir_fd);
}
}
16 changes: 11 additions & 5 deletions crates/wasi-preview1-component-adapter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1293,12 +1293,18 @@ pub unsafe extern "C" fn fd_write(
BlockingMode::Blocking.write(wasi_stream, bytes)?
};

// If this is a file, keep the current-position pointer up to date.
// If this is a file, keep the current-position pointer up
// to date. Note that for files that perform appending
// writes this function will always update the current
// position to the end of the file.
//
// NB: this isn't "atomic" as it doesn't necessarily account
// for concurrent writes, but there's not much that can be
// done about that.
if let StreamType::File(file) = &streams.type_ {
// But don't update if we're in append mode. Strictly speaking,
// we should set the position to the new end of the file, but
// we don't have an API to do that atomically.
if !file.append {
if file.append {
file.position.set(file.fd.stat()?.size);
} else {
file.position.set(file.position.get() + nbytes as u64);
}
}
Expand Down
6 changes: 5 additions & 1 deletion crates/wasi/src/preview2/preview1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1428,6 +1428,7 @@ impl<
position,
}) if t.view.table().get(fd)?.is_file() => {
let fd = fd.borrowed();
let fd2 = fd.borrowed();
let blocking_mode = *blocking_mode;
let position = position.clone();
let append = *append;
Expand All @@ -1452,7 +1453,10 @@ impl<
(stream, pos)
};
let n = blocking_mode.write(self, stream, &buf).await?;
if !append {
if append {
let len = self.stat(fd2).await?;
position.store(len.size, Ordering::Relaxed);
} else {
let pos = pos.checked_add(n as u64).ok_or(types::Errno::Overflow)?;
position.store(pos, Ordering::Relaxed);
}
Expand Down

0 comments on commit 55ac268

Please sign in to comment.