Update zephyr file socket branch #4406
Conversation
Fixed issues in os_renameat Signed-off-by: Stephen Berard <stephen.berard@outlook.com>
Addressed numerous of build warnings on Zephyr Signed-off-by: Stephen Berard <stephen.berard@outlook.com>
Signed-off-by: Stephen Berard <stephen.berard@outlook.com>
Signed-off-by: Stephen Berard <stephen.berard@outlook.com>
Partial implementation — just avoids a hard fault.
lum1n0us
requested review from
loganek,
no1wudi,
TianlongLiang,
wenyongh,
xujuntwt95329 and
yamt
as code owners
core/iwasm/common/wasm_application.c
Outdated
| @@ -289,6 +289,7 @@ wasm_application_execute_main(WASMModuleInstanceCommon *module_inst, int32 argc, | |||
| exec_env = wasm_runtime_get_exec_env_singleton(module_inst); | |||
| if (exec_env) { | |||
| wasm_runtime_dump_mem_consumption(exec_env); | |||
| (WASMModuleInstance *)module_inst->cur_exception | |||
There was a problem hiding this comment.
Hi @lum1n0us, I don't understand what you mean by the above. I don't believe this file was updated as part of this Zephyr work. Can you explain more what you would like to see changed here?
There was a problem hiding this comment.
I used to think it was a typo and that we should just remove it.
| @@ -2212,7 +2212,7 @@ wasmtime_ssp_poll_oneoff(wasm_exec_env_t exec_env, struct fd_table *curfds, | |||
| if (error == 0) { | |||
| // Proper file descriptor on which we can poll(). | |||
| pfds[i] = (os_poll_file_handle){ | |||
| .fd = fos[i]->file_handle, | |||
| .fd = fos[i]->file_handle->fd, | |||
There was a problem hiding this comment.
file_handle's type is os_file_handle which is typedef int os_file_handle. So a compilation error will be raised: error: invalid type argument of ‘->’ (have ‘os_file_handle’ {aka ‘int’}).
There was a problem hiding this comment.
In #3633, @lucasAbadFr changed for Zephyr to be the following:
typedef struct zephyr_handle *os_file_handle;
I'll see what I can do to unify this better to align with the POSIX/Linux implementation.
| if (dup == NULL) { | ||
| return NULL; // Allocation failed | ||
| } | ||
|
|
There was a problem hiding this comment.
May need memset to zero out the buffer.
There was a problem hiding this comment.
It shouldn't be needed here. We allocate the string length plus 1 for the terminator. If the allocation fails, the malloc failed and the buffer was not allocated; NULL is returned to indicate error. If the malloc succeeded, memory is allocated and a memcpy is performed for the full length of the buffer (which is strlen + 1). This will copy the full string plus the NULL terminator. A memset prior would not cause an issue but would work done unnecessarily (perf impact).
In the code above, I'm using malloc when I should probably use os_malloc instead. Existing code uses the BH_MALLOC macro (which calls os_malloc). I've update the code to use this instead.
There was a problem hiding this comment.
I just tested this change to BH_MALLOC and it results in a hard-fault on my device. I have not tracked down the root cause, however it appears to be when the duplicated string is accessed.
There was a problem hiding this comment.
Apologies, the above can be ignored. My issue turned out to be something else memory related in my setup.
| @@ -715,13 +802,18 @@ os_renameat(os_file_handle old_handle, const char *old_path, | |||
|
|
|||
| GET_FILE_SYSTEM_DESCRIPTOR(old_handle->fd, ptr); | |||
|
|
|||
| char *path = strdup(new_path); | |||
| char *path = duplicate_string(new_path); | |||
There was a problem hiding this comment.
need to free path somewhere
|
|
||
| return ret; | ||
| // return ret; |
There was a problem hiding this comment.
need to remove commented code
lum1n0us
merged commit 442ea20
into
bytecodealliance:dev/zephyr_file_socket
This is based on #4377.
@srberard Please help me confirm this update. This PR is supposed to contain all and only your latest modifications. If it is as expected, I will merge this PR while ignoring the CI results. I hope we can use new PRs to fix those CI failures.