search

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

Feat/0.5.4 #91

Closed L-jasmine closed 8 months ago

L-jasmine commented 8 months ago
juntao commented 8 months ago

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

The "Feat/0.5.4" pull request spans across several significant modifications in the codebase. These updates primarily bring enhancements, increase compatibility, and streamline the overall functionality of the software.

The critical changes include the addition of new methods to the Socket structure, refinement of method implementations, the introduction of new functionalities through conversion functions, addition of constants, and promising error handling techniques. Additionally, the PR involves removing outdated codes, eliminating legacy workflow files, and moving away from the 'wasmedge_0_12' feature, which is not supported anymore.

Both the patch for the Socket structure and the removal of older versions indicate a stronger, narrower focus on forward compatibility and efficient operation in the latest environment. Moreover, defunct features like built-in-dns are removed, and the DNS resolution functionality has been refined. Coupled with changes in the error handling and commenting, this represents a drive towards better practices and more efficient codes.

With that said, several concerns loom over these changes. The use of libc data structures is abundant, and type-casting is a recurring theme in the patches, which pose a potential risk of type mismatch and conversion errors. There's also the matter of removing 'wasmedge_0_12' feature and old workflows that could cause problems to users who are still running older versions of the software. Meanwhile, the absence of certain comments and the heavy reliance on the environment variable for 'nslookup' function may lead to maintainability issues. Thorough testing would be necessary to ensure the new functionalities are working correctly, and the older, deleted functionalities are no longer needed.

Lastly, the pull request also includes a version increment in the Cargo.toml file. Typically, a version bump should imply significant changes, fixes, or upgrades in the code. In this PR, there is no explanation provided about this version change, and thorough review exposed no significant alterations tied to it.

As a software reviewer, to fully evaluate this pull request, a comprehensive review of each change, its potential impact on the software including backwards compatibility and feature checks for different platforms, and testing of new methods and functionalities would be necessary.

Details

Commit 2e20aadac4d0fb90bffb3fe022b5213f7b7f2510

The submitted GitHub patch comes with the following changes to the socket.rs:

  1. Addition of several new methods to the Socket structure, enriching its functionality. These methods handle various socket operations such as sending data in vectored format with send_vectored, sending data to a specific destination with send_to_vectored, receiving data with flags with recv_with_flags, receiving vectored data with recv_vectored, checking if the socket is in non-blocking mode with nonblocking, setting the socket to non-blocking mode with set_nonblocking, checking if the socket is a listener with is_listener, and getting the type of the socket with type, among others.

  2. It introduces impl From<libc::iovec> for IovecRead and impl From<libc::iovec> for IovecWrite, which allow the transformation of a libc::iovec structure into IovecRead or IovecWrite structures.

  3. Two new conversion functions are implemented, impl TryFrom<i32> for SocketOptLevel and impl TryFrom<i32> for SocketOptName, which allow the conversion of an i32 value into a SocketOptLevel or SocketOptName enum.

  4. It defines new public constants: MSG_PEEK, MSG_WAITALL and MSG_TRUNC.

  5. New error handling cases are included whereby the functions return std::io::Errors when encountering unexpected scenarios.

Possible problems:

  1. Unsafe blocks - These must be carefully reviewed and tested to mitigate the chances of undefined behaviors and ensure memory safety.

  2. Error Handling - The error handling approach seems to be returning a std::io::Error whenever an unexpected scenario occurs. While this is an acceptable practice, different error types might be a better fit for some of the scenarios.

  3. Extensive use of casting and libc data structures - There is extensive usage of different data types from libc and there are numerous instances of type-casting, which carry a risk of possible type mismatch or conversion errors.

  4. Missing Feature Check - The patch needs to go through feature check to ensure compatibility and functionality across different platforms.

  5. Unimplemented Match Cases - The patch includes several "unimplemented!" calls, which will crash the program when these operation modes are encountered. This implies that support for some conditions is not provided yet. This should be addressed and handled appropriately.

  6. Testing - This is a substantial update, adding lots of new methods to the Socket structure. Therefore, it is critical to have a comprehensive suite of tests to evaluate each newly added function.

Commit 8a8a1c01db70412e3533f64d848e7283a0f64e0e

This patch primarily focuses on removing older versions of code. By deleting "example_0.11.yml" and "example_0.12.yml", it gets rid of workflows that are no longer necessary. Those files are deleted entirely, indicated by "delete mode 100644" in the diff.

The patch renames workflow file "example_0.10.yml" to "example_0.13.yml", and updates the name and version used within the workflow file.

The feature 'wasmedge_0_12' is also removed from "Cargo.toml" and modifications are made to the "src/socket.rs" file to remove parts of the code specific to that version of wasmedge.

Key Changes:

  1. Removing 'wasmedge_0_12' feature from "Cargo.toml".
  2. Deleting workflows for "example_0.11.yml", "example_0.12.yml".
  3. Renaming and modifying "example_0.10.yml" to "example_0.13.yml", with an updated version.
  4. Removing functionality specific to the 'wasmedge_0_12' feature in "src/socket.rs".

Potential Issues:

  1. As the patch removes backwards compatibility with older versions, users still using 'wasmedge_0_12' may encounter problems.
  2. If the newer code isn't completely error-free or fully replaces the functionality of the code specific to 'wasmedge_0_12' feature that's being removed, it might lead to unwanted behavior or breaks in the code.
  3. If any other parts of the codebase depend on the 'wasmedge_0_12' feature or the deleted workflow files, this could also lead to breaks or unwelcome changes in functionality.

The next steps for evaluating this pull request would be to thoroughly test all affected features ensuring new changes have not introduced bugs or broken functionality, and review rest of the codebase to ensure no dependencies on the removed feature or workflows.

Commit fb122268dd88f7a0ce4379742bba31928fc1b64f

Summary of changes:

  1. The feature built-in-dns is removed from the codebase.
  2. In .github/workflows/example_0.13.yml, the cargo build command has removed the built-in-dns feature.
  3. In Cargo.toml, the built-in-dns feature has been removed from the features list.
  4. In src/lib.rs:
    • The function nslookup has been refactored. It now checks if the DNS_SERVER environment variable is set and acting accordingly.
    • A new function nslookup_with_host is introduced that appears to perform DNS resolution without a specific DNS server.
    • Another new function nslookup_with_dns_server as well as changes made existing nslookup; both designed to enhance the functionality of DNS queries.
  5. In src/socket.rs, built-in-dns LLVM feature-gating is removed and the WasiAddrinfo struct as it now appears to be always needed.

Potential issues:

  1. The operations of the removed feature built-in-dns need to be handled in another way, or not needed anymore. This might cause issues if the removed DNS feature was required somewhere in the project and has not been re-implemented under the new system.
  2. The DNS lookup now defaults to a local resolver, which might not always be available. If the environment variable DNS_SERVER is not set, the code will execute nslookup_with_host which interacts with WasiAddrinfo. The behavior of this is very different from the previous DNS lookup functionality, and may result in problems if the host environment doesn't support this.
  3. The connection timeout has been increased from one second to five seconds in nslookup_with_dns_server. Depending on the application, this could be problematic if slower response times were relied upon.
  4. While it is good to see increased code comment on newly added functions nslookup_with_dns_server() and nslookup_with_host(), absence of comment on nslookup() which seem to work based on environment variable may reduce maintainability of the code.
  5. Error handling and fallback scenarios should be very well tested as they appear to be removing a feature and replacing it with a different implementation.

Commit 46d65217aae5fba4aa2a0f7656f65102b4ee9093

The primary change in this pull request titled "Feat/0.5.4" is the increment of the software version from 0.5.3 to 0.5.4 in the Cargo.toml file. The changes are made for the package "wasmedge_wasi_socket".

As a software reviewer, the issue or potential problem I can note from this patch is that there's no additional context provided along with bumping of the version. Usually, a version number increment comes along when there are significant changes, fixes, or upgrades in code. Those should be documented as it helps others understand why a new version was released and what features it introduces or bugs it fixes. Without this context, it may be difficult to track changes or potential impact of version changes in the future.

For a comprehensive review, it's crucial to verify whether any new features, updates, or bug fixes were made in the codebase that aren’t reflected in this commit. If this patch is associated with changes made in the codebase, those changes need to be reviewed too.

On the other hand, if no changes have been made to the codebase along with this version increment, this could imply a mistake or an unnecessary version bump, which needs to be clarified with the author, or it might just be prepping for future expansion/modifications.

To conclude, the Pull Request would benefit from more details about why the version is being incremented in the associated PR notes.