Skip to content

Commit

Permalink
[WasmFS] Allow move to return specific error codes (#17785)
Browse files Browse the repository at this point in the history
It already supported returning errors via a `bool`, but it is better to be able
to return precise errors. Also catch and return errors in the OPFS backend,
which did not previously do any error handling for moves.

Tested manually via an error injected into the OPFS backend since I don't know a
way to get the `move` method on the FileSystemFileHandle to reliably fail in a
way that wouldn't cause the syscall implementation to bail out earlier.
  • Loading branch information
tlively committed Sep 2, 2022
1 parent 44fd8b1 commit 3ce1f72
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 31 deletions.
10 changes: 7 additions & 3 deletions src/library_wasmfs_opfs.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,16 @@ mergeInto(LibraryManager.library, {

_wasmfs_opfs_move__deps: ['$wasmfsOPFSFileHandles',
'$wasmfsOPFSDirectoryHandles'],
_wasmfs_opfs_move: async function(ctx, fileID, newDirID, namePtr) {
_wasmfs_opfs_move: async function(ctx, fileID, newDirID, namePtr, errPtr) {
let name = UTF8ToString(namePtr);
let fileHandle = wasmfsOPFSFileHandles.get(fileID);
let newDirHandle = wasmfsOPFSDirectoryHandles.get(newDirID);
// TODO: error handling
await fileHandle.move(newDirHandle, name);
try {
await fileHandle.move(newDirHandle, name);
} catch {
let err = -{{{ cDefine('EIO') }}};
{{{ makeSetValue('errPtr', 0, 'err', 'i32') }}};
}
_emscripten_proxy_finish(ctx);
},

Expand Down
6 changes: 3 additions & 3 deletions system/lib/wasmfs/backends/memory_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ std::vector<Directory::Entry> MemoryDirectory::getEntries() {
return result;
}

bool MemoryDirectory::insertMove(const std::string& name,
std::shared_ptr<File> file) {
int MemoryDirectory::insertMove(const std::string& name,
std::shared_ptr<File> file) {
auto& oldEntries =
std::static_pointer_cast<MemoryDirectory>(file->locked().getParent())
->entries;
Expand All @@ -76,7 +76,7 @@ bool MemoryDirectory::insertMove(const std::string& name,
}
removeChild(name);
insertChild(name, file);
return true;
return 0;
}

std::string MemoryDirectory::getName(std::shared_ptr<File> file) {
Expand Down
3 changes: 1 addition & 2 deletions system/lib/wasmfs/backends/node_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,7 @@ class NodeDirectory : public Directory {
abort();
}

bool insertMove(const std::string& name,
std::shared_ptr<File> file) override {
int insertMove(const std::string& name, std::shared_ptr<File> file) override {
// TODO
abort();
}
Expand Down
12 changes: 6 additions & 6 deletions system/lib/wasmfs/backends/opfs_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ void _wasmfs_opfs_insert_directory(em_proxying_ctx* ctx,
void _wasmfs_opfs_move(em_proxying_ctx* ctx,
int file_id,
int new_dir_id,
const char* name);
const char* name,
int* err);

void _wasmfs_opfs_remove_child(em_proxying_ctx* ctx,
int dir_id,
Expand Down Expand Up @@ -401,14 +402,13 @@ class OPFSDirectory : public Directory {
return nullptr;
}

bool insertMove(const std::string& name,
std::shared_ptr<File> file) override {
int insertMove(const std::string& name, std::shared_ptr<File> file) override {
auto old_file = std::static_pointer_cast<OPFSFile>(file);
int err = 0;
proxy([&](auto ctx) {
_wasmfs_opfs_move(ctx.ctx, old_file->fileID, dirID, name.c_str());
_wasmfs_opfs_move(ctx.ctx, old_file->fileID, dirID, name.c_str(), &err);
});
// TODO: Handle errors.
return true;
return err;
}

bool removeChild(const std::string& name) override {
Expand Down
12 changes: 6 additions & 6 deletions system/lib/wasmfs/file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,11 @@ Directory::Handle::insertSymlink(const std::string& name,
// TODO: consider moving this to be `Backend::move` to avoid asymmetry between
// the source and destination directories and/or taking `Directory::Handle`
// arguments to prove that the directories have already been locked.
bool Directory::Handle::insertMove(const std::string& name,
std::shared_ptr<File> file) {
int Directory::Handle::insertMove(const std::string& name,
std::shared_ptr<File> file) {
// Cannot insert into an unlinked directory.
if (!getParent()) {
return false;
return -EPERM;
}

// Look up the file in its old parent's cache.
Expand All @@ -161,8 +161,8 @@ bool Directory::Handle::insertMove(const std::string& name,
// involving the backend.

// Attempt the move.
if (!getDir()->insertMove(name, file)) {
return false;
if (auto err = getDir()->insertMove(name, file)) {
return err;
}

if (oldIt != oldCache.end()) {
Expand All @@ -189,7 +189,7 @@ bool Directory::Handle::insertMove(const std::string& name,
oldParent->locked().setMTime(now);
setMTime(now);

return true;
return 0;
}

bool Directory::Handle::removeChild(const std::string& name) {
Expand Down
17 changes: 9 additions & 8 deletions system/lib/wasmfs/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,10 @@ class Directory : public File {
// Move the file represented by `file` from its current directory to this
// directory with the new `name`, possibly overwriting another file that
// already exists with that name. The old directory may be the same as this
// directory. On success, return `true`. Otherwise return `false` without
// changing any underlying state.
virtual bool insertMove(const std::string& name,
std::shared_ptr<File> file) = 0;
// directory. On success return 0 and otherwise return a negative error code
// without changing any underlying state.
virtual int insertMove(const std::string& name,
std::shared_ptr<File> file) = 0;

// Remove the file with the given name, returning `true` on success or if the
// child has already been removed or returning `false` if the child cannot be
Expand Down Expand Up @@ -371,10 +371,11 @@ class Directory::Handle : public File::Handle {
// Move the file represented by `file` from its current directory to this
// directory with the new `name`, possibly overwriting another file that
// already exists with that name. The old directory may be the same as this
// directory. On success, return `true`. Otherwise return `false` without
// changing any underlying state. This should only be called from renameat
// with the locks on the old and new parents already held.
bool insertMove(const std::string& name, std::shared_ptr<File> file);
// directory. On success return 0 and otherwise return a negative error code
// without changing any underlying state. This should only be called from
// renameat with the locks on the old and new parents already held.
[[nodiscard]] int insertMove(const std::string& name,
std::shared_ptr<File> file);

// Remove the file with the given name, returning `true` on success or if the
// vhild has already been removed or returning `false` if the child cannot be
Expand Down
2 changes: 1 addition & 1 deletion system/lib/wasmfs/memory_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class MemoryDirectory : public Directory {
return child;
}

bool insertMove(const std::string& name, std::shared_ptr<File> file) override;
int insertMove(const std::string& name, std::shared_ptr<File> file) override;

size_t getNumEntries() override { return entries.size(); }
std::vector<Directory::Entry> getEntries() override;
Expand Down
4 changes: 2 additions & 2 deletions system/lib/wasmfs/syscalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1014,8 +1014,8 @@ int __syscall_renameat(int olddirfd,
}

// Perform the move.
if (!lockedNewParent.insertMove(newFileName, oldFile)) {
return -EPERM;
if (auto err = lockedNewParent.insertMove(newFileName, oldFile)) {
return err;
}
return 0;
}
Expand Down

0 comments on commit 3ce1f72

Please sign in to comment.