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

chore(go): remove experimental FS API usage in Wasm #3299

Merged
merged 3 commits into from
Dec 18, 2022

Conversation

mathetake
Copy link
Contributor

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Description

Previously, wazero introduced context-based FS (tetratelabs/wazero#571) and used in Trivy to change the underlying file system anytime it calls Analyze API. But it came with the runtime overhead and there's actually an easy way to achieve the same thing without that experimental API.

This PR, hence, removes the usage of the experimental API by introducing the wrapper over memfs.FS which can change the underlying file system in response to the Analyze calls.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake requested a review from knqyf263 as a code owner December 14, 2022 04:16
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake changed the title wasm: remove experimental FS API usage chore(wasm): remove experimental FS API usage Dec 14, 2022
@mathetake mathetake changed the title chore(wasm): remove experimental FS API usage chore(go): remove experimental FS API usage in Wasm Dec 14, 2022
Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! I left a comment.

pkg/module/memfs.go Show resolved Hide resolved
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake requested review from knqyf263 and codefromthecrypt and removed request for knqyf263 and codefromthecrypt December 16, 2022 01:55
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

the lock is at the right place for the filesystem concern. the normal parallel concerns still exist, but can be addressed later (e.g. the other function calls aren't locked and they could end up malloc etc concurrently, but that isn't the heart of this issue)

@knqyf263 knqyf263 merged commit b95d435 into aquasecurity:main Dec 18, 2022
@knqyf263
Copy link
Collaborator

Thanks!

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.

3 participants