Conversation
9e77550 to
a8e6c77
Compare
There was a problem hiding this comment.
Pull request overview
Fixes WASIX symlink behavior so absolute symlink targets resolve from the virtual root and symlinks created by one command become visible to later commands (by persisting them in the writable backing FS, with an in-memory fallback).
Changes:
- Fix absolute-target symlink reopen/follow behavior to resolve from
VIRTUAL_ROOT_FD. - Add symlink node +
readlink/create_symlinksupport tovirtual-fsmemfs and expose it viaTmpFileSystem. - Extend WASIX symlink tests to cover nested symlink opens/reads.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wasix/symlink-open-read-write/main.c | Adds a nested symlink case to validate open/read through a symlink in a subdir. |
| lib/wasix/src/syscalls/wasix/path_open2.rs | Adjusts symlink reopen logic to treat absolute targets as rooted at VIRTUAL_ROOT_FD. |
| lib/wasix/src/syscalls/wasi/path_symlink.rs | Persists symlinks into the backing FS when possible; falls back to an internal registry. |
| lib/wasix/src/fs/mod.rs | Adds ephemeral symlink registry + normalization and adjusts inode path resolution to consult it and to resolve absolute targets from the virtual root. |
| lib/virtual-fs/src/tmp_fs.rs | Exposes create_symlink() on TmpFileSystem. |
| lib/virtual-fs/src/mem_fs/mod.rs | Introduces a Node::Symlink variant and plumbing for inode/name/metadata. |
| lib/virtual-fs/src/mem_fs/filesystem.rs | Implements memfs create_symlink() and readlink() support; includes symlink nodes in parent lookups/debug. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lib/wasix/src/syscalls/wasi/path_symlink.rs:133
path_symlink_internalcreates and inserts the new symlink inode into the in-memory inode tree before attempting to persist the symlink into the backing filesystem. Ifcreate_symlinklater fails with an error (anything other thanUnsupported), the syscall returns an error but the symlink entry has already been created in-memory, leaving the VFS in an inconsistent state. Consider persisting to the backing FS first (and only then creating/inserting the inode), or add explicit rollback logic (remove the dir entry + inode) on failure.
let new_inode =
state
.fs
.create_inode_with_default_stat(inodes, kind, false, entry_name.clone().into());
5b57d03 to
06594bb
Compare
|
Can we review the PR together with #6161 - seems to me there might be some interaction (read: the FS is can of worms). |
…, and mutation flows This consolidates and finalizes the symlink fixes so runtime-created and backing-FS symlinks follow the same representation and resolve consistently across WASIX filesystem code paths. Implemented changes: - Fixed absolute symlink target resolution in both core path traversal and `path_open2` symlink-reopen flow by resolving absolute targets from `VIRTUAL_ROOT_FD`. - Standardized runtime-created symlink representation to match backing-FS-discovered symlinks: `(base_po_dir, path_to_symlink_within_preopen, relative_path_raw_target)`. - Removed depth-based `..` rewriting in `path_symlink`; now stores raw `old_path` symlink targets. - Added `WasiFs::path_into_pre_open_and_relative_path_owned` helper for owned-path conversion. Backing FS + mem-fs integration: - Added symlink-node support in mem-fs: - new `Node::Symlink` variant, - symlink target persistence, - `readlink()` support, - `create_symlink(source, target)` support. - Exposed mem-fs symlink creation via `TmpFileSystem::create_symlink()`. - Updated mem-fs redirect behavior for symlink creation: - delegate to redirected mem-fs when possible, - return `FsError::Unsupported` otherwise. - Updated `path_symlink` to persist symlinks in backing FS when supported (sandbox / overlay primary). - Fallback to ephemeral symlink registration is now only used on `FsError::Unsupported`; other backing-FS errors are propagated. Ephemeral symlink behavior: - Kept ephemeral symlink registry for filesystems without native symlink creation. - Hardened key normalization: - prevent `..` from escaping above `/`, - ignore `Component::Prefix(_)` for virtual key normalization. - Ephemeral symlinks are resolved transiently and not inserted into directory-entry caches. - Added `WasiFs` helpers for ephemeral lifecycle: - `unregister_ephemeral_symlink`, - `move_ephemeral_symlink`. Mutation-flow consistency: - `path_rename` now handles symlink inodes coherently: - attempts backing-FS rename for real symlinks, - allows `Errno::Noent` only for known ephemeral symlinks, - updates stored symlink location (`base_po_dir`, `path_to_symlink`) after rename, - moves ephemeral registry entries for renamed ephemeral symlinks. - `path_unlink_file` now handles symlink deletion coherently: - removes real symlinks from backing FS, - unregisters ephemeral symlink entries when applicable, - preserves removal error behavior for real failures. Tests and validation: - Extended `tests/wasix/symlink-open-read-write/main.c` coverage with nested symlink open/read checks. - Fixed test setup robustness by allowing `EEXIST` from `mkdir`. - Verified with: - `cargo test -p wasmer-wasix test_relative_path_to_absolute --lib` - `cargo test --release --features cranelift --locked --test compilers path_symlink::cranelift::cranelift` - `cargo test --release --features cranelift --locked --test compilers path_rename::cranelift::cranelift` (selection builds; cases are ignored in this suite)
06594bb to
6aa2582
Compare
zebreus
left a comment
There was a problem hiding this comment.
It seems to me that this PR puts a lot of effort into working around the missing symlink(target, linkname) function in the FileSystem trait.
It seems to me like most of this behaviour should be implemented in the virtual fs. Having another layer of emulated "ephemeral" symlinks above that makes the already complex and error-prone system even worse. Can you move that to a FileSystem implementation?
I don't have to live with the consequences
fix(wasix-fs): align symlink semantics across resolution, persistence, and mutation flows