Skip to content

Conversation

usiegl00
Copy link

@usiegl00 usiegl00 commented Dec 1, 2024

This is a revitalization of #1497.
I followed the advice given by a wamr maintainer here:
bytecodealliance/wasm-micro-runtime#3624 (comment)
I also attempted to apply all the requested changes from the original pr.
I have tested the handler with podman on both .wasm and .aot using docs/wasm-wasi-example.md

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

thanks, could you please squash all the patches into a single one?

@usiegl00
Copy link
Author

usiegl00 commented Dec 2, 2024

I have squashed all the patches into a single one.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe
Copy link
Member

giuseppe commented Dec 2, 2024

can you please run make clang-format and amend the changes?

wasm_runtime_set_exception (module_inst, wasi_proc_exit_exception);
ret = false;
}
if (!ret) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ret is always equal to 0 at this line. The if statement condition is thus always true.


// look up a WASM function by its name (The function signature can NULL here)
func = wasm_runtime_lookup_function (module_inst, "_start");
if (!func || func == NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is

if (!func || func == NULL)

equivalent to

if (func == NULL)

?

(I'm not sure why the || is needed)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or to be consistent with style of the other code, just do

if (!func)

@usiegl00
Copy link
Author

usiegl00 commented Dec 3, 2024

Would it be safe to enable the WASI socket api?
https://github.com/bytecodealliance/wasm-micro-runtime/tree/main/samples/socket-api
I believe that wasmedge has it enabled by default:
https://wasmedge.org/docs/develop/rust/socket_networking/server/

@usiegl00 usiegl00 requested a review from eriksjolund December 3, 2024 01:11
@giuseppe
Copy link
Member

giuseppe commented Dec 4, 2024

Would it be safe to enable the WASI socket api?
https://github.com/bytecodealliance/wasm-micro-runtime/tree/main/samples/socket-api
I believe that wasmedge has it enabled by default:
https://wasmedge.org/docs/develop/rust/socket_networking/server/

I think it is fine to enable it

Signed-off-by: usiegl00 <50933431+usiegl00@users.noreply.github.com>
@giuseppe giuseppe merged commit b243185 into containers:main Dec 6, 2024
53 checks passed
@macko99
Copy link

macko99 commented Jan 26, 2025

Thank you all for pushing crun+wamr forward! Sadly I had no time in September to finalize the PR, since I was finalizing my master's thesis on the topic. If anyone is interested in memory footprint evaluation: https://atlarge-research.com/pdfs/2024-mkozub-msc_thesis.pdf

@giuseppe
Copy link
Member

Thank you all for pushing crun+wamr forward! Sadly I had no time in September to finalize the PR, since I was finalizing my master's thesis on the topic. If anyone is interested in memory footprint evaluation: https://atlarge-research.com/pdfs/2024-mkozub-msc_thesis.pdf

thanks, that is interesting

@Mossaka
Copy link

Mossaka commented Jun 11, 2025

Thank you all for pushing crun+wamr forward! Sadly I had no time in September to finalize the PR, since I was finalizing my master's thesis on the topic. If anyone is interested in memory footprint evaluation: https://atlarge-research.com/pdfs/2024-mkozub-msc_thesis.pdf

Hey, @macko99, I am a Runwasi maintainer. Thanks for sharing your thesis on memory footprint and container startup time evaluation on Runwasi.

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.

5 participants