Copilot commented on code in PR #3179:
URL: https://github.com/apache/brpc/pull/3179#discussion_r2647470691
##########
src/brpc/socket.cpp:
##########
@@ -1283,8 +1283,10 @@ int Socket::Connect(const timespec* abstime,
_ssl_state = SSL_OFF;
}
struct sockaddr_storage serv_addr;
+ struct sockaddr_storage cli_addr;
socklen_t addr_size = 0;
- if (butil::endpoint2sockaddr(remote_side(), &serv_addr, &addr_size) != 0) {
+ if (butil::endpoint2sockaddr(remote_side(), &serv_addr, &addr_size) != 0 ||
+ butil::endpoint2sockaddr(local_side(), &cli_addr, &addr_size) != 0) {
Review Comment:
The code unconditionally calls endpoint2sockaddr on local_side(), but when
client_host is not specified, local_side() will be default-initialized to
IP_ANY with port 0. This will cause endpoint2sockaddr to succeed with an
INADDR_ANY address, and the subsequent bind() call will always execute even
when no client IP binding is intended. While binding to INADDR_ANY typically
succeeds, this is unnecessary overhead and could potentially cause issues in
certain network configurations. Consider checking if local_side() is non-empty
before converting it to sockaddr.
##########
src/brpc/socket_map.cpp:
##########
@@ -92,8 +92,9 @@ SocketMap* get_or_new_client_side_socket_map() {
int SocketMapInsert(const SocketMapKey& key, SocketId* id,
const std::shared_ptr<SocketSSLContext>& ssl_ctx,
bool use_rdma,
- const HealthCheckOption& hc_option) {
- return get_or_new_client_side_socket_map()->Insert(key, id, ssl_ctx,
use_rdma, hc_option);
+ const HealthCheckOption& hc_option,
+ butil::EndPoint& client_end_point) {
Review Comment:
The client_end_point parameter should be passed by const reference for
consistency and correctness. Change "butil::EndPoint& client_end_point" to
"const butil::EndPoint& client_end_point".
```suggestion
const butil::EndPoint& client_end_point) {
```
##########
src/brpc/socket.cpp:
##########
@@ -1283,8 +1283,10 @@ int Socket::Connect(const timespec* abstime,
_ssl_state = SSL_OFF;
}
struct sockaddr_storage serv_addr;
+ struct sockaddr_storage cli_addr;
socklen_t addr_size = 0;
- if (butil::endpoint2sockaddr(remote_side(), &serv_addr, &addr_size) != 0) {
+ if (butil::endpoint2sockaddr(remote_side(), &serv_addr, &addr_size) != 0 ||
+ butil::endpoint2sockaddr(local_side(), &cli_addr, &addr_size) != 0) {
Review Comment:
The addr_size variable is shared between both endpoint2sockaddr calls, but
it's only set by the first call (for remote_side). If local_side() and
remote_side() have different address families (e.g., IPv4 vs IPv6), the
addr_size from the remote_side conversion may not be appropriate for the
local_side conversion. Consider using separate addr_size variables for each
conversion: one for serv_addr and one for cli_addr.
##########
src/brpc/channel.h:
##########
@@ -148,6 +148,10 @@ struct ChannelOptions {
// Its priority is higher than FLAGS_health_check_path and
FLAGS_health_check_timeout_ms.
// When it is not set, FLAGS_health_check_path and
FLAGS_health_check_timeout_ms will take effect.
HealthCheckOption hc_option;
+
+ // IP address or host name of the client
Review Comment:
The documentation comment is too brief. Consider adding more details about
the expected format of client_host (IP address or hostname), behavior when the
value is invalid, and any platform-specific considerations. For example,
clarify whether IPv4, IPv6, or both are supported, and mention that the
IP/hostname will be bound to the client socket before connecting to the server.
```suggestion
// Optional local address to bind the client-side socket to before
// connecting to the server.
//
// Expected format:
// - An IP address (IPv4 or IPv6) or a resolvable host name.
// - Typically specified without port; the system chooses an ephemeral
// source port unless otherwise configured.
//
// Behavior:
// - When empty (""), the channel does not explicitly bind a local
// address and the OS selects an appropriate source address.
// - When non-empty, the value is resolved (if a host name) and used to
// bind the client socket's local address before connecting.
// - If the address/host name is invalid, cannot be resolved, or is not
// usable on the current platform or interface, channel
initialization
// or subsequent connection attempts may fail with a network error.
//
// Notes:
// - Both IPv4 and IPv6 may be used, subject to OS and environment
// support.
// - Platform-specific socket behavior (for example, how wildcards are
// interpreted) is determined by the underlying operating system.
```
##########
src/brpc/socket.cpp:
##########
@@ -1297,6 +1299,10 @@ int Socket::Connect(const timespec* abstime,
// We need to do async connect (to manage the timeout by ourselves).
CHECK_EQ(0, butil::make_non_blocking(sockfd));
+ if (::bind(sockfd, (struct sockaddr*)& cli_addr, addr_size) != 0) {
Review Comment:
There's an extra space in the cast: "(struct sockaddr*)& cli_addr" should be
"(struct sockaddr*)&cli_addr" (no space between '&' and 'cli_addr') to be
consistent with line 1307 where the same pattern is used correctly.
```suggestion
if (::bind(sockfd, (struct sockaddr*)&cli_addr, addr_size) != 0) {
```
##########
src/brpc/socket.cpp:
##########
@@ -1297,6 +1299,10 @@ int Socket::Connect(const timespec* abstime,
// We need to do async connect (to manage the timeout by ourselves).
CHECK_EQ(0, butil::make_non_blocking(sockfd));
+ if (::bind(sockfd, (struct sockaddr*)& cli_addr, addr_size) != 0) {
+ LOG(FATAL) << "Fail to bind socket, errno=" << strerror(errno);
Review Comment:
The strerror(errno) call should be replaced with berror() or PLOG macro to
be consistent with error handling patterns used elsewhere in this file (see
lines 1290, 1295, 1309, etc.). Also, errno might not be set if bind succeeds,
so it should be saved immediately after the bind() call returns an error.
```suggestion
const int saved_errno = errno;
LOG(FATAL) << "Fail to bind socket: " << berror(saved_errno);
```
##########
src/brpc/socket.cpp:
##########
@@ -1297,6 +1299,10 @@ int Socket::Connect(const timespec* abstime,
// We need to do async connect (to manage the timeout by ourselves).
CHECK_EQ(0, butil::make_non_blocking(sockfd));
+ if (::bind(sockfd, (struct sockaddr*)& cli_addr, addr_size) != 0) {
+ LOG(FATAL) << "Fail to bind socket, errno=" << strerror(errno);
+ return -1;
+ }
Review Comment:
The bind() call is executed unconditionally, even when local_side() is
default-initialized (IP_ANY with port 0). This means bind() will be called for
all connections, not just those where the user explicitly specified a
client_host. Consider only calling bind() when local_side() represents a valid
non-default endpoint. You can check this by comparing local_side() against
butil::EndPoint() or checking if the IP is not IP_ANY.
##########
src/brpc/socket_map.h:
##########
@@ -82,18 +82,21 @@ struct SocketMapKeyHasher {
int SocketMapInsert(const SocketMapKey& key, SocketId* id,
const std::shared_ptr<SocketSSLContext>& ssl_ctx,
bool use_rdma,
- const HealthCheckOption& hc_option);
+ const HealthCheckOption& hc_option,
+ butil::EndPoint& client_end_point);
Review Comment:
The client_end_point parameter is passed by non-const reference, but it's
only used as an input parameter (not modified). This should be passed by const
reference to indicate that the function doesn't modify the parameter and to
allow temporary objects to be passed. Change "butil::EndPoint&
client_end_point" to "const butil::EndPoint& client_end_point".
```suggestion
const butil::EndPoint& client_end_point);
```
##########
src/brpc/socket_map.h:
##########
@@ -155,17 +158,20 @@ class SocketMap {
int Insert(const SocketMapKey& key, SocketId* id,
const std::shared_ptr<SocketSSLContext>& ssl_ctx,
bool use_rdma,
- const HealthCheckOption& hc_option);
+ const HealthCheckOption& hc_option,
+ butil::EndPoint& client_end_point);
Review Comment:
The client_end_point parameter should be passed by const reference for
consistency and correctness. Change "butil::EndPoint& client_end_point" to
"const butil::EndPoint& client_end_point".
```suggestion
const butil::EndPoint& client_end_point);
```
##########
src/brpc/channel.cpp:
##########
@@ -397,6 +410,13 @@ int Channel::Init(const char* ns_url,
_options.mutable_ssl_options()->sni_name = _service_name;
}
}
+ butil::EndPoint client_end_point;
+ if (!_options.client_host.empty() &&
+ butil::str2ip(_options.client_host.c_str(), &client_end_point.ip) != 0
&&
+ butil::hostname2ip(_options.client_host.c_str(), &client_end_point.ip)
!= 0) {
+ LOG(ERROR) << "Invalid client host=`" << _options.client_host << '\'';
+ return -1;
+ }
Review Comment:
The logic for parsing client_host is duplicated in both InitSingle and Init
methods (lines 370-376 and 413-419). Consider extracting this common logic into
a helper method to avoid code duplication and ensure consistency. This will
make maintenance easier and reduce the risk of divergence between the two
implementations.
##########
src/brpc/socket_map.cpp:
##########
@@ -229,7 +230,8 @@ void SocketMap::ShowSocketMapInBvarIfNeed() {
int SocketMap::Insert(const SocketMapKey& key, SocketId* id,
const std::shared_ptr<SocketSSLContext>& ssl_ctx,
bool use_rdma,
- const HealthCheckOption& hc_option) {
+ const HealthCheckOption& hc_option,
+ butil::EndPoint& client_end_point) {
Review Comment:
The client_end_point parameter should be passed by const reference for
consistency and correctness. Change "butil::EndPoint& client_end_point" to
"const butil::EndPoint& client_end_point".
```suggestion
const butil::EndPoint& client_end_point) {
```
##########
src/brpc/socket.cpp:
##########
@@ -1297,6 +1299,10 @@ int Socket::Connect(const timespec* abstime,
// We need to do async connect (to manage the timeout by ourselves).
CHECK_EQ(0, butil::make_non_blocking(sockfd));
+ if (::bind(sockfd, (struct sockaddr*)& cli_addr, addr_size) != 0) {
+ LOG(FATAL) << "Fail to bind socket, errno=" << strerror(errno);
Review Comment:
Using LOG(FATAL) for bind() failure will terminate the process. This is
overly severe for what could be a user configuration error (e.g., invalid
client IP). Consider using LOG(ERROR) or LOG(WARNING) instead and allowing the
error to propagate up so that the application can handle it gracefully. The
function already returns -1 on error, which is the appropriate error handling
mechanism.
```suggestion
LOG(ERROR) << "Fail to bind socket, errno=" << strerror(errno);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
