Zephyr: WASI support for sockets and file system #3633
Conversation
- Port socket and file system APIs to Zephyr - Add 2 samples demonstrating the new functionality - Document the samples for ease of use - Address feedback from previous code reviews
|
Hello @wenyongh ! Reaching out after two months, like we said previously I tried to make CI run. I might need some helps with the various platforms to pass every tests. Thanks you. |
@lucasAbadFr Welcome. Thanks for the great effort, please feel free to ask question about CI if needed, we will try to help resolve it. And BTW, had better rebase your repo with the main branch since some CI errors were fixed recently. |
…to not redefine them in sgx_file.c
|
@wenyongh I managed to make most tests pass, but there are still 6 tests failing:
Not sure why these builds/tests are failing. |
|
@lucasAbadFr the errors reported by CIs can be found below (you can click Actions or visit https://github.com/bytecodealliance/wasm-micro-runtime/actions): https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/10090660314/job/27900575529 D:\a\wasm-micro-runtime\wasm-micro-runtime\core\shared\platform\windows\win_file.c(1799,9): error C2231: '.type': left operand points to 'struct', use '->' [D:\a\wasm-micro-runtime\wasm-micro-runtime\product-mini\platforms\windows\build\libiwasm.vcxproj]
D:\a\wasm-micro-runtime\wasm-micro-runtime\core\shared\platform\windows\win_file.c(1799,9): error C2231: '.type': left operand points to 'struct', use '->' [D:\a\wasm-micro-runtime\wasm-micro-runtime\product-mini\platforms\windows\build\vmlib.vcxproj]
D:\a\wasm-micro-runtime\wasm-micro-runtime\core\shared\platform\windows\win_file.c(1799,25): error C2231: '.type': left operand points to 'struct', use '->' [D:\a\wasm-micro-runtime\wasm-micro-runtime\product-mini\platforms\windows\build\libiwasm.vcxproj]
D:\a\wasm-micro-runtime\wasm-micro-runtime\core\shared\platform\windows\win_file.c(1799,25): error C2231: '.type': left operand points to 'struct', use '->' [D:\a\wasm-micro-runtime\wasm-micro-runtime\product-mini\platforms\windows\build\vmlib.vcxproj]
D:\a\wasm-micro-runtime\wasm-micro-runtime\core\shared\platform\windows\win_file.c(1803,9): error C2231: '.fdflags': left operand points to 'struct', use '->' [D:\a\wasm-micro-runtime\wasm-micro-runtime\product-mini\platforms\windows\build\libiwasm.vcxproj]https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/10090660328/job/27900702537 Test poll_oneoff_stdio failed
[exit_code] 0 == 1
STDOUT:
Exception: unreachable
STDERR:
thread 'main' panicked at 'stdin read should return at least 1 event', src/bin/poll_oneoff_stdio.rs:51:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
...
Test tcp_udp failed
[exit_code] 0 == 1
STDOUT:
Testing address family: 1 protocol: 6
Exception: unreachable
STDERR:
Assertion failed: sizeof(struct sockaddr_in6) <= addrlen (../src/wasi/wasi_socket_ext.c: sockaddr_to_wasi_addr: 74)And there are compilation warnings: In file included from /home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/shared/utils/bh_platform.h:13,
from /home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/ssp_config.h:17,
from /home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c:14:
In function ‘wasmtime_ssp_sock_recv_from’,
inlined from ‘wasmtime_ssp_sock_recv’ at /home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c:2803:12:
/home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/shared/utils/bh_common.h:17:20: warning: ‘src_addr’ may be used uninitialized [-Wmaybe-uninitialized]
17 | int _ret = b_memcpy_s(dest, dlen, src, slen); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c:2819:5: note: in expansion of macro ‘bh_memcpy_s’
2819 | bh_memcpy_s(&src_addr_copy, sizeof(__wasi_addr_t), src_addr,
| ^~~~~~~~~~~
/home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c: In function ‘wasmtime_ssp_sock_recv’:
/home/runner/work/wasm-micro-runtime/wasm-micro-runtime/core/shared/utils/bh_common.h:51:1: note: by argument 3 of type ‘const void *’ to ‘b_memcpy_s’ declared here
51 | b_memcpy_s(void *s1, unsigned int s1max, const void *s2, unsigned int n);Maybe you can try to resolve the compiler error and warnings first, and refer to the CI steps to download/build related tools/cases to test the wasi case failure (Test poll_oneoff_stdio/tcp_udp failed). Since there are lots of changes in the PR, we need more time to read the code and give comments, the response may be a little late. Thanks. |
|
BTW, the PR #3650 was merged, had better rebase your code with main branch. |
product-mini/platforms/zephyr/simple-http/wasm-apps/http_get.wasm
Outdated
Show resolved
Hide resolved
samples/socket-api/file.wasm
Outdated
There was a problem hiding this comment.
could you also remove this file?
|
|
||
| ptr->path = strdup(new_path); | ||
| if (ptr->path == NULL) { | ||
| ptr->path = old_path; |
There was a problem hiding this comment.
had better also fs_rename(abs_new_path, abs_old_path)?
There was a problem hiding this comment.
Yes, we should.
Then here, if the (new) fs_rename fail what should we do ? This will put the app in a weird state.
Should we retry:
do {
rc = fs_rename(abs_new_path, abs_old_path);
retry_count++;
} while (rc < 0 && retry_count < 5);
if (rc < 0) {
// Error: failed to rename back
return __WASI_EIO;
}The do while is just to illustrate we could use a for then if and break.
There was a problem hiding this comment.
Is it better to strdup first and then fs_rename? When fs_rename failed, we can just free the string allocated by strdup.
There was a problem hiding this comment.
And also here had better GET_FILE_SYSTEM_DESCRIPTOR first, and then strdup, then fs_rename? Since the function may return failure inside the macro, and fs_rename and free are not called before return.
There was a problem hiding this comment.
Another issue is that we should also free the original ptr->path but not set it directly. Do you think the below like code is better?
GET_FILE_SYSTEM_DESCRIPTOR(old_handle->fd, ptr);
char *path = strdup(new_path);
if (path == NULL) {
return __WASI_ENOMEM;
}
int rc = fs_rename(abs_old_path, abs_new_path);
if (rc < 0) {
free(path);
return convert_errno(-rc);
}
free(ptr->path);
ptr->path = path;
return __WASI_ESUCCESS;
|
I am working on getting WASI support working on Zephyr and came across this thread. Are there any plans to complete and merge this PR? I'm happy to contribute, just wanted to see if I should start form here or the current trunk. |
I am not sure whether @lucasAbadFr has bandwidth to finish it, in fact most of the comments were resolved exception several ones. Maybe we can merge it into another branch (e.g. dev/zephyr_file_socket) and let @srberard continue the work? And when the feature is done, we can merge the commits into the main branch. @lucasAbadFr how do you think? |
|
@wenyongh that works for me. |
wenyongh
requested review from
loganek,
lum1n0us,
no1wudi,
xujuntwt95329 and
yamt
as code owners
wenyongh
merged commit 0464262
into
bytecodealliance:dev/zephyr_file_socket
Great! I think @lucasAbadFr is busy and may not be able to response quickly, so I merged this PR to branch @lucasAbadFr Many thanks for your contribution! Please let us know if you have any concerns. |
|
Excellent. I will start on this later this week and update this thread. |
You're welcome, and many thanks for all the time taken during reviews. |
To address #3311.
This work also implement the WASI support on Zephyr.