chore(tools): Refactor saunafs legacy packet functions LS-361#758
chore(tools): Refactor saunafs legacy packet functions LS-361#758
Conversation
|
@gemini-code-assist review |
Summary of ChangesHello @rolysr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly modernizes the network communication layer within various Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of the networking code in several tool files. The move from manual C-style buffer management and error checking to a more modern C++ approach using ServerConnection and try-catch blocks greatly improves code readability, maintainability, and safety. The changes are applied consistently for the most part. I've found a few minor inconsistencies in the new error handling and an unused variable, which I've commented on. Once these are addressed, this will be an excellent improvement.
There was a problem hiding this comment.
Pull request overview
This PR refactors legacy packet communication functions in SaunaFS tools to use a more modern and consistent pattern. The changes introduce the ServerConnection::sendAndReceive method for handling master server communication, replacing manual packet serialization and deserialization code. Additionally, the PR adds support for infinite timeouts in the Timeout class by treating negative timeout values as infinite.
Changes:
- Refactored 9 tool files to use
ServerConnection::sendAndReceivewith consistent exception handling - Added infinite timeout support to the
Timeoutclass with corresponding unit tests - Updated master registration to use the new packet serialization pattern
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/set_trashtime.cc | Refactored to use ServerConnection::sendAndReceive; replaced manual packet handling |
| src/tools/set_eattr.cc | Refactored to use ServerConnection::sendAndReceive; simplified error handling |
| src/tools/get_trashtime.cc | Refactored to use ServerConnection::sendAndReceive; improved code structure |
| src/tools/get_eattr.cc | Refactored to use ServerConnection::sendAndReceive; cleaner aggregated mode handling |
| src/tools/file_repair.cc | Refactored to use ServerConnection::sendAndReceive; simplified response parsing |
| src/tools/file_info.cc | Refactored to use ServerConnection::sendAndReceive; wrapped in try-catch |
| src/tools/dir_info.cc | Refactored to use ServerConnection::sendAndReceive; improved legacy format handling |
| src/tools/check_file.cc | Refactored to use ServerConnection::sendAndReceive; cleaner format detection |
| src/tools/append_file.cc | Refactored to use deserializeAllLegacyPacketDataNoHeader; simplified parsing |
| src/tools/master_functions.cc | Refactored master_register to use ServerConnection::sendAndReceive |
| src/common/time_utils.h | Added infinite_ flag to Timeout class |
| src/common/time_utils.cc | Implemented infinite timeout support |
| src/common/time_utils_unittest.cc | Added test coverage for infinite timeout functionality |
There was a problem hiding this comment.
Code Review
The pull request successfully refactors several legacy packet functions in the SaunaFS tools to use the more modern ServerConnection and MessageBuffer abstractions. This significantly improves code readability and maintainability by removing manual buffer management and socket I/O. Additionally, the Timeout class has been enhanced to support infinite timeouts. I have noted a couple of instances of variable shadowing in the refactored code that should be addressed to avoid confusion.
rolysr
changed the title
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
2 times, most recently
from
fbed236 to
dac5473
Compare
rolysr
changed the title
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
from
dac5473 to
98aca38
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors several SaunaFS tools to use the centralized ServerConnection for network communication, which improves maintainability and error handling. The modernization of the Timeout class to support infinite durations is also a welcome change. I have identified a potential buffer over-read in the master registration logic and an opportunity to better utilize the new infinite timeout functionality in the fileinfo tool.
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
from
98aca38 to
4149085
Compare
rolysr
requested review from
GigaCronos,
antuan96314,
dmga44,
lgsilva3087,
ralcolea and
uristdwarf
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
2 times, most recently
from
ee2ad9a to
f1f964b
Compare
lgsilva3087
left a comment
lgsilva3087
left a comment
There was a problem hiding this comment.
Great change Roly.
Consider my comment and in general:
Standardize exception catching to const references.
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
2 times, most recently
from
956f09a to
fc987e7
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request is a great step towards modernizing the network communication in the saunafs tools. Centralizing message handling with ServerConnection and replacing manual tcpwrite/tcpread significantly cleans up the code and improves maintainability. The introduction of exception-based error handling is also a major improvement for robustness. The changes to support infinite timeouts are a welcome addition.
I've found a recurring issue where the new try-catch blocks are not fully utilized. In several files, error conditions inside the try block are still handled with printf and return -1 instead of throwing an exception. This undermines the new centralized error handling model. I've left a couple of specific comments on this, but the issue is present in check_file.cc, dir_info.cc, file_info.cc, file_repair.cc, get_eattr.cc, get_trashtime.cc, master_functions.cc, set_eattr.cc, and set_trashtime.cc. It would be best to convert all these manual error checks to throw Exception(...) for consistency.
Additionally, I've noted an opportunity to consistently use the new infinite timeout feature in file_info.cc.
Overall, this is a very positive refactoring. Addressing the consistency of error handling will make it even better.
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
from
fc987e7 to
21e9940
Compare
The previous appendchunks implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous checkfile implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous dirinfo implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous fileinfo implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous filerepair implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
Previously, passing a negative timeout to a Timeout constructor, resulted in an immediately expired Timeout instance due to negative duration handling. This behavior differed from the legacy poll()-based implementation, where passing -1 meant waiting indefinitely. Timeout now explicitly supports infinite mode. When constructed with a negative duration, it behaves as an infinite timeout: - remaining_*() returns -1 (poll-compatible) - expired() always returns false - deadline() returns max time point This allows seamless substitution of legacy timeout logic with the new Timeout-based implementation without changing caller semantics. Unit tests were extended to validate infinite timeout behavior. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous settrashtime implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous gettrashtime implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous register to master implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous geteattr implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
The previous seteattr implementation in the saunafs command used an outdated, error-prone manual approach for building, sending, and receiving packets to and from the master. This made the code difficult to read and unnecessarily verbose, especially since a cleaner implementation for handling legacy packets already existed in the codebase. This commit replaces the old approach with the existing, cleaner solution. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
rolysr
force-pushed
the
refactor-saunafs-legacy-packet-functions
branch
from
21e9940 to
704f600
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in modernizing the networking code within the saunafs tools. Centralizing the communication logic in ServerConnection and using exceptions for error handling significantly improves code readability, maintainability, and robustness. The addition of an infinite timeout capability is also a valuable improvement. My review focuses on ensuring consistency and correctness in the application of these new features. I've identified a potential conflict with a design rule regarding infinite timeouts in locking mechanisms, and a potential unintended change in timeout values for some operations. Overall, this is a very positive refactoring.
Previous tcpwrite/tcpread used for master commnunication on tools functions had a 10 seconds timeout set as default, which is not the default value if not specified in the corresponding substitutions of ServerConnection::sendAndReceive functions. This commit sets that timeout to the expected one as before given new communication bindings on tools functions. Signed-off-by: Rolando Sánchez Ramos <rolysr@leil.io>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, refactoring legacy network communication in several tools to use the modern ServerConnection class. The changes greatly enhance code clarity, maintainability, and error handling by replacing manual buffer management and tcpread/tcpwrite calls with a centralized, exception-based approach. The introduction of support for infinite timeouts in the Timeout class is also a valuable addition. I've found one minor point of inconsistency regarding the use of this new timeout feature, which I've detailed in a specific comment. Overall, this is an excellent and well-executed refactoring.
This PR modernizes network communication in saunafs tools by centralizing message handling with ServerConnection, replacing manual tcpwrite/tcpread and buffer management. It standardizes request/response processing, improves error handling via exceptions, and simplifies tool logic.
Key changes:
Impact: Cleaner, safer, and more maintainable tool code with robust and unified networking.