Skip to content

Conversation

@loganek
Copy link
Collaborator

@loganek loganek commented Jul 21, 2022

I also slightly changed the interface - since we already have a __wasi_addr_t
structure which is an union, there's no need for passing length around - the
address buffer will always have the right length (i.e. max of all address
families).

@loganek loganek force-pushed the loganek/sock_addr_local branch 3 times, most recently from 838706b to 91de2e8 Compare July 21, 2022 21:57
@loganek loganek force-pushed the loganek/sock_addr_local branch 5 times, most recently from e508289 to a74f1cd Compare July 22, 2022 10:06
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

only some code-style problems

@loganek
Copy link
Collaborator Author

loganek commented Jul 25, 2022

only some code-style problems

Thanks, already replied.

@loganek loganek force-pushed the loganek/sock_addr_local branch 3 times, most recently from 7f61c25 to b5f8ce6 Compare July 27, 2022 09:16
@wenyongh wenyongh changed the base branch from main to dev/socket July 28, 2022 04:25
@loganek loganek force-pushed the loganek/sock_addr_local branch 5 times, most recently from b38dda4 to a0a6ea8 Compare July 28, 2022 20:53
@wenyongh
Copy link
Contributor

@loganek There are compilation errors on linux-sgx platform:

usr/bin/ld: libvmlib.a(posix.c.o): in function `wasi_ssp_sock_addr_local':
posix.c:(.text.wasi_ssp_sock_addr_local+0x157): undefined reference to `ntohl'
/usr/bin/ld: libvmlib.a(posix.c.o): in function `wasi_ssp_sock_addr_resolve':
posix.c:(.text.wasi_ssp_sock_addr_resolve+0x151): undefined reference to `ntohl'

These functions are not provided by sgx-sdk, but they were implemented in linux-sgx sgx_socket.c as static functions:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/linux-sgx/sgx_socket.c#L65-L130

Could you please add the API declarations in core/shared/platform/linux-sgx/platform_internal.h, and move the implementation into core/shared/platform/linux-sgx/platform_init.c and remove static flag?

@loganek loganek force-pushed the loganek/sock_addr_local branch from a0a6ea8 to eefbfd9 Compare July 30, 2022 19:58
@loganek loganek force-pushed the loganek/sock_addr_local branch 2 times, most recently from a3864d7 to 77448c0 Compare July 31, 2022 20:36
I also slightly changed the interface - since we already have a `__wasi_addr_t`
structure which is an union, there's no need for passing length around - the
address buffer will always have the right length (i.e. max of all address
families).
@loganek loganek force-pushed the loganek/sock_addr_local branch from 77448c0 to 6a3ddd7 Compare July 31, 2022 21:14
@loganek
Copy link
Collaborator Author

loganek commented Aug 1, 2022

@loganek There are compilation errors on linux-sgx platform:

usr/bin/ld: libvmlib.a(posix.c.o): in function `wasi_ssp_sock_addr_local':
posix.c:(.text.wasi_ssp_sock_addr_local+0x157): undefined reference to `ntohl'
/usr/bin/ld: libvmlib.a(posix.c.o): in function `wasi_ssp_sock_addr_resolve':
posix.c:(.text.wasi_ssp_sock_addr_resolve+0x151): undefined reference to `ntohl'

These functions are not provided by sgx-sdk, but they were implemented in linux-sgx sgx_socket.c as static functions: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/linux-sgx/sgx_socket.c#L65-L130

Could you please add the API declarations in core/shared/platform/linux-sgx/platform_internal.h, and move the implementation into core/shared/platform/linux-sgx/platform_init.c and remove static flag?

Hi @wenyongh , I've kept the implementation in sgx_socket.c but made it non-static. Is there any reason we'd prefer to move it to platform_init.c? What's the purpose of that file? I think similar functions follow the same pattern (i.e. the implementation exists in sgx_socket file).

Regarding the header file - I added the declaration to sgx_socket.h, as it looked like other functions are declared there, and the file is already included through other headers.

@wenyongh
Copy link
Contributor

wenyongh commented Aug 1, 2022

Hi @wenyongh , I've kept the implementation in sgx_socket.c but made it non-static. Is there any reason we'd prefer to move it to platform_init.c? What's the purpose of that file? I think similar functions follow the same pattern (i.e. the implementation exists in sgx_socket file).

Regarding the header file - I added the declaration to sgx_socket.h, as it looked like other functions are declared there, and the file is already included through other headers.

@loganek It should be also OK, platform_init.c is to implement some common APIs for the platform, here it should be sgx_platform.c, for linux/windows/darwin, we named it platform_init.c. I will review the code again and merge it if there is no issue found.

@wenyongh wenyongh merged commit 3e77b05 into bytecodealliance:dev/socket Aug 1, 2022
wenyongh added a commit that referenced this pull request Sep 22, 2022
Implement more socket APIs, refer to #1336 and below PRs:
- Implement wasi_addr_resolve function (#1319)
- Fix socket-api byte order issue when host/network order are the same (#1327)
- Enhance sock_addr_local syscall (#1320)
- Implement sock_addr_remote syscall (#1360)
- Add support for IPv6 in WAMR (#1411)
- Implement ns lookup allowlist (#1420)
- Implement sock_send_to and sock_recv_from system calls (#1457)
- Added http downloader and multicast socket options (#1467)
- Fix `bind()` calls to receive the correct size of `sockaddr` structure (#1490)
- Assert on correct parameters (#1505)
- Copy only received bytes from socket recv buffer into the app buffer (#1497)

Co-authored-by: Marcin Kolny <[email protected]>
Co-authored-by: Marcin Kolny <[email protected]>
Co-authored-by: Callum Macmillan <[email protected]>
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Slightly change the __wasi_sock_addr_local interface - since we already have
a `__wasi_addr_t` structure which is an union, there's no need for passing the
length around - the address buffer will always have the right length (i.e. max
of all address families).
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Implement more socket APIs, refer to bytecodealliance#1336 and below PRs:
- Implement wasi_addr_resolve function (bytecodealliance#1319)
- Fix socket-api byte order issue when host/network order are the same (bytecodealliance#1327)
- Enhance sock_addr_local syscall (bytecodealliance#1320)
- Implement sock_addr_remote syscall (bytecodealliance#1360)
- Add support for IPv6 in WAMR (bytecodealliance#1411)
- Implement ns lookup allowlist (bytecodealliance#1420)
- Implement sock_send_to and sock_recv_from system calls (bytecodealliance#1457)
- Added http downloader and multicast socket options (bytecodealliance#1467)
- Fix `bind()` calls to receive the correct size of `sockaddr` structure (bytecodealliance#1490)
- Assert on correct parameters (bytecodealliance#1505)
- Copy only received bytes from socket recv buffer into the app buffer (bytecodealliance#1497)

Co-authored-by: Marcin Kolny <[email protected]>
Co-authored-by: Marcin Kolny <[email protected]>
Co-authored-by: Callum Macmillan <[email protected]>
@loganek loganek deleted the loganek/sock_addr_local branch June 10, 2024 12:48
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