-
Notifications
You must be signed in to change notification settings - Fork 311
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
add handler for wasm-micro-runtime (wamr) #1497
Conversation
podman system tests failed. @containers/packit-build please check. |
thanks for working on this. Could you run a Also, please squash the commits into one since it is only one new feature. @flouthoc PTAL |
@giuseppe I will have more time in 2 weeks so I will put some more love into this work, thanks for the tips! |
src/libcrun/handlers/wamr.c
Outdated
} | ||
|
||
// Function to read a WebAssembly binary file into a buffer | ||
char *read_wasm_binary_to_buffer(const char *pathname, uint32_t *size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you just use read_all_file(...)
from utils.h
?
src/libcrun/handlers/wamr.c
Outdated
fseek(file, 0, SEEK_SET); | ||
|
||
// Allocate memory for the buffer | ||
buffer = (char *)malloc(file_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crun uses xmalloc
across the code base, i think it should exit on its own when failure happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giuseppe WDYT ?
src/libcrun/handlers/wamr.c
Outdated
error (EXIT_FAILURE, 0, "could not find wasm_runtime_set_wasi_args symbol in `libiwasm.so`"); | ||
|
||
char *buffer, error_buf[128]; | ||
uint32_t size, stack_size = 8096, heap_size = 8096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for choosing 8096
as stack and heap size ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, I will try playing with this once this is ready. |
please squash the commits |
Signed-off-by: Maciej <[email protected]> wamr is working! Signed-off-by: Maciej <[email protected]> cleanup the code Signed-off-by: Maciej <[email protected]> clang-format Signed-off-by: Maciej <[email protected]> remove unnecessary functions load Signed-off-by: Maciej <[email protected]> Signed-off-by: Maciej <[email protected]>
|
||
// Function to read a WebAssembly binary file into a buffer | ||
char * | ||
read_wasm_binary_to_buffer (const char *pathname, uint32_t *size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use read_all_file
from utils.h
instead of this function
// Read the file into the buffer | ||
if (fread (buffer, 1, file_size, file) != file_size) | ||
{ | ||
error (EXIT_FAILURE, 0, "Failed to read file"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lines
free (buffer);
fclose (file);
return NULL;
are never executed because EXIT_FAILURE
(that is nonzero) is passed as the first argument to error().
Reference:
quote from
https://man7.org/linux/man-pages/man3/error.3.html
If status has a nonzero value, then error() calls exit(3) to
terminate the program using the given value as the exit status;
otherwise it returns after printing the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I now realised my comment about error() was unnecessary because Giuseppe
already suggested replacing the whole function.
if (! module_inst) | ||
error (EXIT_FAILURE, 0, "Failed to instantiate the WASM module"); | ||
|
||
/* lookup a WASM function by its name; the function signature can be NULL here */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookup
->
look up
(and the same in the error message on line 214)
if (! func || func == NULL) | ||
error (EXIT_FAILURE, 0, "Failed to lookup the WASM function"); | ||
|
||
/* creat an execution environment to execute the WASM functions */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creat
->
create
Please edit the comment #1497 (comment) and add a new line with the text
This will create a link to the issue. When the PR is merged, the issue will automatically be closed. |
@macko99 still working on this? |
I got no feedback. Please reopen if you are still working on it |
preview, not yet fully developed