Skip to content
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

[WasmFS] Allow backends to report errors from flush #17781

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Sep 1, 2022

And catch and report such errors in the OPFS backend to prevent errors from
causing the async task to hang and create deadlocks.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__wasi_fd_sync calls flush in syscalls.cpp, I think that needs to check the return value now? lgtm with that.

await accessHandle.flush();
} catch {
let err = -{{{ cDefine('EIO') }}};
{{{ makeSetValue('errPtr', 0, 'err', 'i32') }}};
Copy link
Member

@kripken kripken Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this adds errPtr since there wasn't an existing return value to reuse? Just want to make sure I understand the convention.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. That's what was happening before with setSize as well.

@tlively
Copy link
Member Author

tlively commented Sep 1, 2022

Oh right, I forgot that I had been adding [[nodiscard]] to all of these. I'll do that and fix up syscalls.cpp.

@tlively tlively force-pushed the wasmfs-opfs-errors-refactor branch from 410395b to f449ced Compare September 1, 2022 19:19
@tlively tlively force-pushed the wasmfs-flush-errors branch from d8dd6fc to 5dffb10 Compare September 1, 2022 19:19
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but the off_t change could be in a separate PR?

@tlively
Copy link
Member Author

tlively commented Sep 1, 2022

Oops, yeah, I meant to put that in the next PR.

Base automatically changed from wasmfs-opfs-errors-refactor to main September 1, 2022 20:30
And catch and report such errors in the OPFS backend to prevent errors from
causing the async task to hang and create deadlocks.
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.

2 participants