second-state / wasmedge_wasi_socket

A Rust lib for socket on WasmEdge.
https://second-state.github.io/wasmedge_wasi_socket/
Apache License 2.0
70 stars 29 forks source link

[Lib] fix `Socket::get_peer` & `Socket::get_local` #88

Closed L-jasmine closed 1 year ago

L-jasmine commented 1 year ago

fix Socket::get_peer & Socket::get_local. Synchronize this pr. https://github.com/WasmEdge/WasmEdge/pull/2557

juntao commented 1 year ago

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


This pull request, titled "[Lib] fix Socket::get_peer & Socket::get_local", introduces changes across multiple aspects of the repository, including modification of existing code, updating the package version, and adding a new CI workflow.

Potential issues and errors:

  1. In the modified code, there is a risk of type mismatch because of casting between AddressFamily enum and numeric values. Future changes in the AddressFamily enum definition may introduce issues.
  2. The changes lack additional tests to verify the new logic and ensure correctness.
  3. Documentation and comments are not updated, leaving the rationale for certain changes unclear.
  4. The use of specific numeric values for AddressFamily, i.e., 4 and 6, lacks justification and reasoning.
  5. The patch mainly aims for compatibility either with an older library version or to handle inconsistent AddressFamily enumeration values but does not provide enough context for these changes.

Key findings:

  1. The patch updates the local and peer address retrieval methods for sockets, improving the conditions for checking address families.
  2. Package version for the "wasmedge_wasi_socket" has been incremented from 0.5.0 to 0.5.1.
  3. A new CI workflow named example_0.12.yml has been added to the repository, which includes tests and builds for various examples. However, the HTTP client example is disabled due to a cyclic dependency.

It is recommended to address the potential problems, primarily by providing proper documentation and comments explaining the changes, and implementing additional tests to verify the new logic. Also, revisiting the compatibility changes related to AddressFamily enumeration values could result in a cleaner and more maintainable solution.

Details

Commit c771c3b7a7e5cbdd44fff77b89f76f1824626f91

Summary of key changes:

  1. In examples/tcp_stream.rs, the local address of the TcpStream object is printed using the local_addr() method.
  2. In src/socket.rs, the if conditions to check addr_type in both get_peer and get_local methods are modified. Instead of integer literals (4 and 6), the corresponding enum values AddressFamily::Inet4 and AddressFamily::Inet6 are used.

Potential problems:

  1. There is a type casting from enum (AddressFamily) to u8 and then to u32. Type mismatch could be a potential issue in the future if any changes occur in the enum definition.
  2. The patch does not have any additional tests to check if the new logic works as intended. Adding tests to cover the changes will help ensure the correctness of the implementation.
  3. No documentation/comments have been updated to reflect the changes made in the code. Adding relevant documentation will help others understand the rationale behind the changes.

Commit edf60948358cbed7a457a9e6905f736f865f97f0

This GitHub patch updates the version number of the "wasmedge_wasi_socket" package in the Cargo.toml file. The key change made is bumping the version from 0.5.0 to 0.5.1.

There are no potential problems identified in this patch since it's only a minor version change. However, it's important to ensure that the package's functionality and any relevant documentation are updated to reflect the change. Additionally, any dependencies should be checked to ensure they continue to work with the new version.

Commit 4422527279d9bcaf557182d11b8f97688c0eb8b8

This patch adds a new GitHub workflow file called example_0.12.yml. The workflow is triggered on push and pull_request events for the main branch and on manual workflow_dispatch. The workflow runs on an ubuntu-20.04 environment and installs the necessary dependencies like wget, git, curl, software-properties-common, and build-essential. It also installs the Rust target wasm32-wasi and the WasmEdge project.

The patch includes the following tests and builds:

  1. HTTP async client example - builds nonblock_http_client and runs it.
  2. HTTP server example - builds http_server, runs it, and tests it by sending 6 POST requests to the server.
  3. UDP Socket Example - builds and runs the udp_socket example with built-in DNS features.
  4. DNS Example - builds and runs the get_addrinfo DNS example.
  5. ToSocketAddrs Example - builds and runs the socket_addr example.

Potential problems and observations:

Overall, this patch seems to focus on improving the CI workflow by adding tests for various examples and installing the necessary dependencies for building and running those examples.

Commit 866b864d0b9f413bda0fe1b2abac51dea34b2668

The key changes in the patch are:

  1. Added an extra println! line in examples/tcp_stream.rs to print the peer address using the stream.peer_addr().unwrap() method.

  2. In src/socket.rs, the conditions for checking the AddressFamily values have been modified. The original code checked if the addr_type was equal to the AddressFamily value casted to an u8 and then to a u32. The patch also considers value 4 for AddressFamily::Inet4 and 6 for AddressFamily::Inet6 in addition to the original conditions.

Potential problems:

  1. The patch seems to be aiming for compatibility with an older version of the library (0.12.1), but there is no clear explanation on why this change is being made or why the specific values of 4 and 6 have been chosen.

  2. Alternatively, if these changes are being introduced to handle inconsistent AddressFamily enumeration values, it might be better to fix the inconsistency at the source (where the values are defined) or create a utility function to handle these checks correctly, instead of implementing the conditional checks in multiple places.

  3. In terms of code readability and maintainability, a comment should be added to explain the reason behind the new condition check for both address families, especially as this change might not be self-explanatory for future developers working on this project.