[WIP]feat: support ipv6 #100
Conversation
WalkthroughThis pull request introduces comprehensive modifications to support multiple IP addresses and enhance IPv6 compatibility across the KiwiDB networking stack. The changes span configuration management, socket handling, and network event processing. Key updates include allowing multiple listening addresses, improving socket creation for IPv4 and IPv6, and refactoring configuration handling to support more flexible network configurations. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant EventServer as Event Server
participant ListenSocket as Listen Socket
participant NetworkStack as Network Stack
Config->>Config: Define multiple IPs
Config->>EventServer: Pass IP list
EventServer->>ListenSocket: Create socket for each IP
ListenSocket->>NetworkStack: Bind and listen
NetworkStack-->>EventServer: Confirm socket readiness
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
marsevilspirit
requested review from
lqxhub and
luky116
and removed request for
lqxhub and
luky116
|
想本地跑下看看效果，发现使用这个分支在本地无法进行编译，我是 mac m2 芯片，你看看
|
可能是我删除了一些头文件的问题，我修改一下 这个pr目前还没有完成，目标是可以同时bind ipv4 和 ipv6，创造出两个fd 去 listen，目前只能bind其中之一。 |
|
试试在 free BSD 系统是否能通过，他代表mac系统的运行环境 |
|
目前支持了ipv4和ipv6的同时bind和listen, 后续准备优化一下代码，目前的代码有些许丑陋，并准备在conf中添加ipv6的使用选项 |
|
conf 的 ip 配置处理完毕, 可review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
etc/conf/kiwi.conf(2 hunks)src/cmd_admin.cc(1 hunks)src/config.cc(2 hunks)src/config.h(4 hunks)src/kiwi.cc(1 hunks)src/kiwi_logo.h(1 hunks)src/net/base_event.h(4 hunks)src/net/base_socket.cc(2 hunks)src/net/base_socket.h(2 hunks)src/net/callback_function.h(1 hunks)src/net/client_socket.cc(2 hunks)src/net/epoll_event.cc(3 hunks)src/net/epoll_event.h(2 hunks)src/net/event_server.h(4 hunks)src/net/kqueue_event.cc(3 hunks)src/net/kqueue_event.h(1 hunks)src/net/listen_socket.cc(3 hunks)src/net/listen_socket.h(1 hunks)src/net/socket_addr.h(1 hunks)src/net/stream_socket.cc(1 hunks)src/net/thread_manager.h(5 hunks)src/raft/raft.cc(2 hunks)src/replication.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/net/listen_socket.h
- src/kiwi.cc
- src/net/base_socket.h
- src/net/kqueue_event.h
- src/net/callback_function.h
- src/net/client_socket.cc
- src/replication.cc
- src/kiwi_logo.h
- src/net/listen_socket.cc
- src/net/stream_socket.cc
- src/net/base_socket.cc
- src/net/kqueue_event.cc
- src/net/epoll_event.cc
- src/net/base_event.h
- src/cmd_admin.cc
🔇 Additional comments (13)
src/raft/raft.cc (1)
327-327: Verify consistency of IP configuration usage.The change from
g_config.iptog_config.raft_ipshould be consistently applied across all Raft-related functionality.Run the following script to verify consistent usage:
✅ Verification successful
The change from
g_config.iptog_config.raft_ipis consistently appliedThe codebase shows proper separation between general IP configuration (
g_config.ip/ips) and Raft-specific IP configuration (g_config.raft_ip). All Raft-related functionality consistently usesg_config.raft_ip.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of g_config.ip that should be g_config.raft_ip rg 'g_config\.ip' src/raft/Length of output: 126
Script:
#!/bin/bash # Search for any remaining g_config.ip instances in the entire codebase rg 'g_config\.ip' -A 2 # Search for g_config.raft_ip usage to verify consistency rg 'g_config\.raft_ip' -A 2Length of output: 1321
src/net/epoll_event.h (1)
26-27: LGTM! Constructor updated to support multiple listen sockets.The constructor has been properly modified to handle multiple listening sockets, which is necessary for supporting both IPv4 and IPv6 simultaneously. The initialization list correctly forwards the parameters to the base class.
src/net/socket_addr.h (2)
21-25: LGTM! Memory operations are safe and efficient.The use of
memcpywithsizeof(addr_)is appropriate for copying the union structure.
101-112: LGTM! Comparison operators handle both IPv4 and IPv6 correctly.The equality comparison is properly implemented with appropriate checks for address family and memory comparison for IPv6 addresses.
src/config.cc (2)
10-10: Remove unnecessary include.The
<iostream>include was previously flagged for removal and should be deleted as it's not being used.-#include <iostream> //delete
137-138: LGTM! Good IPv6 support implementation.The changes correctly implement support for multiple IP addresses and Raft configuration:
- Using
AddStringArrayfor multiple IP bindings supports both IPv4 and IPv6- Separate Raft IP configuration allows for flexible network topology
src/net/thread_manager.h (2)
58-58: LGTM! Well-structured interface changes for multi-socket support.The method signatures are properly updated to handle multiple listening sockets, which is essential for IPv6 support. The parameter names are clear and follow the project's naming conventions.
Also applies to: 86-87
127-129: LGTM! Clean implementation of multi-socket initialization.The implementation correctly handles the initialization of multiple listening sockets and maintains backward compatibility with the existing timer functionality.
etc/conf/kiwi.conf (3)
12-12: LGTM! Clear IPv6 configuration.The configuration now properly supports both IPv4 (
127.0.0.1) and IPv6 (::1) addresses.
345-345: LGTM! Fixed configuration syntax.Removed trailing semicolon for consistency with the configuration format.
348-351: LGTM! Well-documented Raft IP configuration.The Raft IP configuration is properly documented and maintains consistent spacing with surrounding sections.
src/config.h (2)
286-288: LGTM! Clear documentation and implementation for IPv6 support.The changes properly implement IPv6 support:
- Documentation clearly explains the dual-stack support
- Default configuration includes both IPv4 and IPv6 loopback addresses
345-347: LGTM! Well-structured Raft IP configuration.The Raft IP configuration is properly implemented with a sensible default value.
|
build bug fixd |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/net/event_server.h (1)
Line range hint
278-282: Add comprehensive error handling.Consider adding:
- Cleanup of listen sockets when thread start fails
- Validation that listenSockets is not empty in server mode
Apply this diff to add the suggested error handling:
+ if (serverMode && listenSockets.empty()) { + return static_cast<int>(NetListen::ERROR); + } bool ret = i == 0 ? thread->Start(listenSockets, timer_) : thread->Start(listenSockets, nullptr); if (!ret) { + listenSockets.clear(); // Clean up sockets on thread start failure return -1; }src/net/socket_addr.h (1)
46-48: Enhance error handling with more descriptive messages.Add the scope resolution operator and provide more descriptive error messages.
- if (::inet_pton(AF_INET6, ip.c_str(), &addr_.addr6_.sin6_addr) != 1) { - throw std::invalid_argument("Invalid IP address format"); + if (::inet_pton(AF_INET6, ip.c_str(), &addr_.addr6_.sin6_addr) != 1) { + throw ::std::invalid_argument("Invalid IPv6 address format: " + ip);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/kiwi.cc(2 hunks)src/net/epoll_event.cc(6 hunks)src/net/event_server.h(4 hunks)src/net/kqueue_event.cc(4 hunks)src/net/listen_socket.cc(4 hunks)src/net/socket_addr.h(1 hunks)src/net/timer.cc(1 hunks)src/raft/raft.cc(2 hunks)src/storage/src/debug.h(1 hunks)src/storage/src/options_helper.h(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/storage/src/debug.h
- src/storage/src/options_helper.h
- src/net/timer.cc
🚧 Files skipped from review as they are similar to previous changes (5)
- src/kiwi.cc
- src/net/listen_socket.cc
- src/raft/raft.cc
- src/net/epoll_event.cc
- src/net/kqueue_event.cc
🔇 Additional comments (8)
src/net/event_server.h (3)
10-10: LGTM! Good preparation for multi-address support.The addition of
sys/socket.hand conversion oflistenAddrs_to a vector properly sets up the foundation for supporting multiple listening addresses, which is essential for dual-stack IPv4/IPv6 support.Also applies to: 92-92
46-46: Implementation follows previous review feedback.Good use of
emplace_backfor efficient in-place construction of socket addresses.
257-258: LGTM! Proper cleanup implementation.Good implementation of socket cleanup on initialization failure, following the previous review feedback.
Also applies to: 271-272
src/net/socket_addr.h (5)
21-28: LGTM! Copy operations are correctly implemented.The copy constructor and assignment operator correctly handle both IPv4 and IPv6 addresses using safe memory operations.
97-101: LGTM! Utility methods are well-implemented.The validity and address family checking methods are concise and correct.
103-124: LGTM! Memory management and comparisons are well-implemented.The Clear method, equality operators, and address storage union are correctly implemented with proper handling of both IPv4 and IPv6 addresses.
82-88:⚠️ Potential issueAdd error handling to GetIP buffer method.
The buffer-based GetIP method lacks error handling and could potentially write beyond buffer bounds.
std::string GetIP(char *buf, socklen_t size) const { if (IsIPV4()) { - ::inet_ntop(AF_INET, &addr_.addr4_.sin_addr, buf, size); + if (!::inet_ntop(AF_INET, &addr_.addr4_.sin_addr, buf, size)) { + throw ::std::runtime_error("Failed to convert IPv4 address to string"); + } return buf; } - ::inet_ntop(AF_INET6, &addr_.addr6_.sin6_addr, buf, size); + if (!::inet_ntop(AF_INET6, &addr_.addr6_.sin6_addr, buf, size)) { + throw ::std::runtime_error("Failed to convert IPv6 address to string"); + } return buf; }Likely invalid or redundant comment.
53-65: Verify usage of GetAddr and GetAddrLen in socket operations.Let's verify that these methods are used correctly in socket creation and binding operations throughout the codebase.
✅ Verification successful
GetAddr and GetAddrLen methods are correctly used in socket operations
The methods are properly used in pairs for both server-side binding and client connections, with appropriate error handling and correct parameter types for socket API calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for socket creation and binding operations rg -A 5 "socket\(" rg -A 5 "bind\(" # Search for GetAddr and GetAddrLen usage rg -A 2 "GetAddr\(\)" rg -A 2 "GetAddrLen\(\)"Length of output: 3427
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/net/event_server.h (1)
266-273: Consider adding configuration option for port reuse.While the port reuse implementation is correct, consider making it configurable through
NetOptionssince it might not be desired in all deployment scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net/event_server.h(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (6)
src/net/event_server.h (6)
10-15: LGTM! Appropriate system header inclusion.The addition of
<sys/socket.h>is necessary for socket-related functionality and follows proper include organization.
46-46: LGTM! Efficient implementation using emplace_back.The method correctly uses
emplace_backfor efficient insertion into the vector, as previously suggested.
92-92: LGTM! Vector implementation supports multiple addresses.The change from single address to vector enables listening on multiple addresses, supporting both IPv4 and IPv6 simultaneously.
278-278: LGTM! Proper thread initialization with timer control.The implementation correctly initializes threads with the socket vector and restricts timer to the first thread.
251-259: Verify IPv6 socket initialization across different platforms.The socket initialization looks correct but should be tested on various platforms (as mentioned in PR comments about FreeBSD and macOS).
Run this script to check socket implementation across files:
✅ Verification successful
IPv6 socket initialization verified as platform-independent
The implementation uses standard POSIX socket APIs and proper error handling, making it safe across different platforms including FreeBSD and macOS. No platform-specific code is required as it follows established networking standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for IPv6-related socket configurations and platform-specific code rg -A 3 "AF_INET6|IPV6_V6ONLY|in6_addr" # Check for platform-specific socket implementations ast-grep --pattern 'if defined(__FreeBSD__) || defined(__APPLE__) { $$$ }'Length of output: 1974
249-273: Verify dual-stack socket behavior.Please ensure the implementation correctly handles dual-stack (IPv4 + IPv6) scenarios:
- Binding to both IPv4 and IPv6 addresses simultaneously
- Proper handling of IPv4-mapped IPv6 addresses
- Correct behavior when only IPv6 is available
Run this script to check dual-stack implementation:
✅ Verification successful
Dual-stack socket implementation verified
The implementation correctly handles dual-stack scenarios with proper:
- Address family detection and socket creation
- IPv4 and IPv6 address initialization
- Dual-stack control via IPV6_V6ONLY socket option
- Address validation and conversion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dual-stack socket configurations rg -A 5 "IPV6_V6ONLY|AF_INET6.*AF_INET" # Check for IPv4-mapped IPv6 address handling ast-grep --pattern 'struct sockaddr_in6 { $$$ }'Length of output: 448
Script:
#!/bin/bash # Check socket creation and address family handling ast-grep --pattern 'socket($$$)' # Find address family selection logic rg -B 3 -A 3 "AF_INET6|AF_INET" src/net/ # Look for bind implementation ast-grep --pattern 'bind($$$)'Length of output: 3712
|
待看 japson 如何处理配置多个 IP 地址的问题，再考虑合并 |
It remains to be seen how japson handles the problem of configuring multiple IP addresses before considering merging it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/config.h (1)
352-354: Consider using dual-stack for Raft communication.The Raft IP configuration currently defaults to IPv4 only. Consider supporting both IPv4 and IPv6 for Raft communication as well.
Consider updating the Raft IP configuration similar to the main IP configuration:
- std::string raft_ip = "127.0.0.1"; + std::vector<std::string> raft_ips = {"127.0.0.1", "::1"};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
etc/conf/kiwi.conf(2 hunks)src/config.cc(2 hunks)src/config.h(4 hunks)src/kiwi.cc(1 hunks)src/net/base_socket.cc(2 hunks)src/net/base_socket.h(2 hunks)src/net/event_server.h(4 hunks)src/net/thread_manager.h(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/net/base_socket.h
- src/kiwi.cc
- src/config.cc
- src/net/base_socket.cc
- etc/conf/kiwi.conf
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (8)
src/net/event_server.h (3)
46-46: LGTM! Method inlining optimization.The
AddListenAddrmethod is now marked as inline, which is appropriate for this small, frequently called method.
92-92: LGTM! Data structure change to support multiple addresses.Changed
listenAddrs_from a singleSocketAddrtostd::vector<SocketAddr>to support multiple listening addresses for IPv4/IPv6.
249-281: 🛠️ Refactor suggestionVerify error handling in socket initialization loop.
The socket initialization loop now handles multiple addresses, but there's a potential resource leak if initialization fails for any socket after the first one.
Apply this diff to ensure proper cleanup:
if (serverMode) { for (auto &listenAddr : listenAddrs_) { std::shared_ptr<ListenSocket> listen(ListenSocket::CreateTCPListen()); listen->SetListenAddr(listenAddr); listen->SetBSTcpKeepAlive(tcpKeepAlive); - listenSockets.push_back(listen); if (auto ret = (listen->Init() != static_cast<int>(NetListen::OK))) { listenSockets.clear(); // Clean up all sockets return ret; } + listenSockets.push_back(listen); } }Run this script to verify error handling in similar patterns:
src/net/thread_manager.h (4)
58-58: LGTM! Method signatures updated for multiple sockets.The method signatures have been updated to accept a vector of sockets instead of a single socket, aligning with the IPv6 support changes.
Also applies to: 86-87
128-130: LGTM! Start method updated for multiple sockets.The Start method has been updated to pass the vector of sockets to CreateReadThread.
279-280: LGTM! Event creation updated for multiple sockets.The event creation has been updated to handle multiple sockets for both epoll and kqueue events.
Also applies to: 288-288, 290-290
322-322: LGTM! Write thread initialization with empty socket list.The write thread is correctly initialized with an empty vector of sockets as it doesn't need to listen for new connections.
Also applies to: 324-324
src/config.h (1)
293-295: LGTM! Configuration updated for dual-stack support.The IP configuration has been updated to support both IPv4 (127.0.0.1) and IPv6 (::1) addresses by default.
Run this script to verify the configuration handling:
✅ Verification successful
Dual-stack support properly implemented and verified
The implementation correctly supports both IPv4 and IPv6:
- Socket layer has proper dual-stack support with IPv6 socket options
- Configuration system correctly handles multiple IP addresses
- Raft subsystem intentionally uses a single configurable IP for cluster setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the configuration parser correctly handles multiple IP addresses # Check if the configuration parser handles both IPv4 and IPv6 addresses ast-grep --pattern 'AddStringArray($_, $_, $_)' # Check for any hardcoded IP addresses that might need to be updated rg -g '!*.md' '127\.0\.0\.1|::1'Length of output: 1635
Script:
#!/bin/bash # Check raft implementation for IPv6 support and config parsing # Look for IP address parsing/validation in raft code rg -g '*.{h,cc}' -B 2 -A 2 'ParseIP|ValidateIP|addr|address' src/raft/ # Check configuration parsing implementation ast-grep --pattern 'class Config { $$$ }' # Look for any IPv6 related code rg -g '*.{h,cc}' '(?i)ipv6|::1|AF_INET6'Length of output: 23217
是 jephsen 测试 |
is jephsen test |
support ipv6
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
Configuration Updates
Network Improvements
Performance Enhancements
Bug Fixes