empiredan commented on code in PR #1897:
URL:
https://github.com/apache/incubator-pegasus/pull/1897#discussion_r1496895384
##########
src/common/json_helper.h:
##########
@@ -421,7 +421,9 @@ inline bool json_decode(const dsn::json::JsonObject &in,
dsn::rpc_address &addre
if (rpc_address_string == "invalid address") {
return true;
}
- return address.from_string_ipv4(rpc_address_string.c_str());
+
+ address = dsn::rpc_address::from_host_port(rpc_address_string);
+ return !address.is_invalid();
Review Comment:
```suggestion
return address;
```
##########
src/replica/storage/simple_kv/simple_kv.app.example.h:
##########
@@ -45,8 +45,7 @@ class simple_kv_client_app : public ::dsn::service_app
return ::dsn::ERR_INVALID_PARAMETERS;
printf("%s %s %s\n", args[1].c_str(), args[2].c_str(),
args[3].c_str());
- dsn::rpc_address meta;
- meta.from_string_ipv4(args[2].c_str());
+ dsn::rpc_address meta = rpc_address::from_host_port(args[2]);
Review Comment:
```suggestion
const auto meta = rpc_address::from_host_port(args[2]);
```
##########
src/client/test/ddl_client_test.cpp:
##########
@@ -103,7 +103,7 @@ TEST(DDLClientTest, RetryMetaRequest)
dsn::ERR_BUSY_CREATING},
};
- std::vector<rpc_address> meta_list = {{"127.0.0.1", 34601}};
+ std::vector<rpc_address> meta_list =
{rpc_address::from_ip_port("127.0.0.1", 34601)};
Review Comment:
```suggestion
const std::vector<rpc_address> meta_list =
{rpc_address::from_ip_port("127.0.0.1", 34601)};
```
##########
src/runtime/test/host_port_test.cpp:
##########
@@ -62,7 +62,7 @@ TEST(host_port_test, host_port_build)
ASSERT_EQ("localhost", hp.host());
{
- rpc_address addr = rpc_address("localhost", 8080);
+ rpc_address addr = rpc_address::from_host_port("localhost", 8080);
Review Comment:
```suggestion
const auto addr = rpc_address::from_host_port("localhost", 8080);
```
##########
src/runtime/rpc/rpc_address.h:
##########
@@ -66,71 +66,48 @@ class rpc_address
{
public:
static const rpc_address s_invalid_address;
+
+ // Convert IPv4:port to rpc_address, e.g. "192.168.0.1:12345" or
"localhost:54321".
+ // NOTE:
+ // - IP address without port (e.g. "127.0.0.1") is considered as invalid.
+ static rpc_address from_ip_port(std::string_view ip_port);
+
+ // Similar to the above, but specify the 'ip' and 'port' separately.
+ static rpc_address from_ip_port(std::string_view ip, uint16_t port);
+
+ // Convert hostname:port to rpc_address, e.g. "192.168.0.1:12345",
"localhost:54321" or
+ // "host1:12345".
+ // NOTE:
+ // - Hostname without port (e.g. "host1") is considered as invalid.
+ // - It contains a hostname resolve produce, so typically it's slower
than from_ip_port().
+ // - It requires 'hostname' is a null terminate string which only
contains hostname.
+ static rpc_address from_host_port(std::string_view host_port);
+
+ // Similar to the above, but specify the 'host' and 'port' separately.
+ static rpc_address from_host_port(std::string_view host, uint16_t port);
+
static bool is_docker_netcard(const char *netcard_interface, uint32_t
ip_net);
static bool is_site_local_address(uint32_t ip_net);
- static uint32_t ipv4_from_host(const char *hostname);
+ // TODO(yingchun): Use dsn_resolver to resolve hostname.
+ static error_s ipv4_from_host(const char *hostname, uint32_t *ip);
Review Comment:
Use `std::string_view` instead ?
```suggestion
static error_s ipv4_from_host(std::string_view hostname, uint32_t *ip);
```
##########
src/runtime/rpc/network.cpp:
##########
@@ -579,21 +580,22 @@ message_parser
*network::new_message_parser(network_header_format hdr_format)
uint32_t network::get_local_ipv4()
{
uint32_t ip = 0;
+ error_s s;
if (!utils::is_empty(FLAGS_explicit_host_address)) {
- ip = rpc_address::ipv4_from_host(FLAGS_explicit_host_address);
+ s = rpc_address::ipv4_from_host(FLAGS_explicit_host_address, &ip);
}
- if (0 == ip) {
+ if (!s.is_ok() || 0 == ip) {
Review Comment:
```suggestion
if (!s || 0 == ip) {
```
##########
src/meta/test/misc/misc.cpp:
##########
@@ -60,10 +60,11 @@ uint32_t random32(uint32_t min, uint32_t max)
void generate_node_list(std::vector<dsn::rpc_address> &output_list, int
min_count, int max_count)
{
- int count = random32(min_count, max_count);
+ auto count = random32(min_count, max_count);
Review Comment:
```suggestion
const auto count = random32(min_count, max_count);
```
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -162,8 +162,8 @@ int zookeeper_session::attach(void *callback_owner, const
state_callback &cb)
zoo_sasl_params_t sasl_params = {0};
sasl_params.service = FLAGS_zookeeper_kerberos_service_name;
sasl_params.mechlist = "GSSAPI";
- rpc_address addr;
- CHECK(addr.from_string_ipv4(FLAGS_zookeeper_sasl_service_fqdn),
+ rpc_address addr =
dsn::rpc_address::from_host_port(FLAGS_zookeeper_sasl_service_fqdn);
+ CHECK(addr,
Review Comment:
```suggestion
CHECK(dsn::rpc_address::from_host_port(FLAGS_zookeeper_sasl_service_fqdn),
```
##########
src/runtime/rpc/network.sim.cpp:
##########
@@ -168,12 +168,12 @@ error_code sim_network_provider::start(rpc_channel
channel, int port, bool clien
"invalid given channel {}",
channel);
- _address = ::dsn::rpc_address("localhost", port);
+ _address = dsn::rpc_address::from_host_port("localhost", port);
auto hostname = boost::asio::ip::host_name();
if (!client_only) {
for (int i = NET_HDR_INVALID + 1; i <=
network_header_format::max_value(); i++) {
if (s_switch[channel][i].put(_address, this)) {
- auto ep2 = ::dsn::rpc_address(hostname.c_str(), port);
+ auto ep2 = ::dsn::rpc_address::from_host_port(hostname, port);
s_switch[channel][i].put(ep2, this);
Review Comment:
```suggestion
s_switch[channel][i].put(dsn::rpc_address::from_host_port(hostname, port),
this);
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -205,51 +247,85 @@ const char *rpc_address::ipv4_str() const
if (_addr.v4.type == HOST_TYPE_IPV4) {
net_addr.s_addr = htonl(ip());
- inet_ntop(AF_INET, &net_addr, p, sz);
+ ::inet_ntop(AF_INET, &net_addr, p, sz);
} else {
p = (char *)"invalid_ipv4";
}
return p;
}
-bool rpc_address::from_string_ipv4(const char *s)
+rpc_address rpc_address::from_ip_port(std::string_view ip_port)
{
- set_invalid();
- std::string ip_port(s);
- auto pos = ip_port.find_last_of(':');
- if (pos == std::string::npos) {
- return false;
+ std::string_view ip;
+ uint16_t port;
+ if (dsn_unlikely(!extract_host_port(ip_port, ip, port))) {
+ return {};
}
- std::string ip = ip_port.substr(0, pos);
- std::string port = ip_port.substr(pos + 1);
- // check port
- unsigned int port_num;
- if (!internal::buf2unsigned(port, port_num) || port_num > UINT16_MAX) {
- return false;
+
+ // Use std::string(ip) to add a null terminator.
+ return from_ip_port(std::string(ip), port);
+}
+
+rpc_address rpc_address::from_ip_port(std::string_view ip, uint16_t port)
+{
+ // Check is IPv4 integer.
+ uint32_t ip_num;
+ int ret = ::inet_pton(AF_INET, ip.data(), &ip_num);
+ switch (ret) {
+ case 1:
+ // ::inet_pton() returns 1 on success (network address was
successfully converted)
+ return {ntohl(ip_num), port};
+ case -1:
+ LOG_ERROR(
+ "{}: fail to convert '{}:{}' to rpc_address",
utils::safe_strerror(errno), ip, port);
+ break;
+ default:
+ LOG_ERROR("'{}' does not contain a character string representing a
valid network address",
+ ip);
+ break;
}
- // check localhost & IP
- uint32_t ip_addr;
- if (ip == "localhost" || inet_pton(AF_INET, ip.c_str(), &ip_addr)) {
- assign_ipv4(ip.c_str(), (uint16_t)port_num);
- return true;
+ return {};
+}
+
+rpc_address rpc_address::from_host_port(std::string_view host_port)
+{
+ std::string_view host;
+ uint16_t port;
+ if (dsn_unlikely(!extract_host_port(host_port, host, port))) {
+ return {};
+ }
+
+ // Use std::string(host) to add a null terminator.
+ return from_host_port(std::string(host), port);
+}
+
+rpc_address rpc_address::from_host_port(std::string_view hostname, uint16_t
port)
+{
+ uint32_t ip = 0;
+ if (!ipv4_from_host(hostname.data(), &ip).is_ok()) {
Review Comment:
```suggestion
if (!ipv4_from_host(hostname.data(), &ip)) {
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -205,51 +247,85 @@ const char *rpc_address::ipv4_str() const
if (_addr.v4.type == HOST_TYPE_IPV4) {
net_addr.s_addr = htonl(ip());
- inet_ntop(AF_INET, &net_addr, p, sz);
+ ::inet_ntop(AF_INET, &net_addr, p, sz);
} else {
p = (char *)"invalid_ipv4";
}
return p;
}
-bool rpc_address::from_string_ipv4(const char *s)
+rpc_address rpc_address::from_ip_port(std::string_view ip_port)
{
- set_invalid();
- std::string ip_port(s);
- auto pos = ip_port.find_last_of(':');
- if (pos == std::string::npos) {
- return false;
+ std::string_view ip;
+ uint16_t port;
+ if (dsn_unlikely(!extract_host_port(ip_port, ip, port))) {
+ return {};
}
- std::string ip = ip_port.substr(0, pos);
- std::string port = ip_port.substr(pos + 1);
- // check port
- unsigned int port_num;
- if (!internal::buf2unsigned(port, port_num) || port_num > UINT16_MAX) {
- return false;
+
+ // Use std::string(ip) to add a null terminator.
+ return from_ip_port(std::string(ip), port);
+}
+
+rpc_address rpc_address::from_ip_port(std::string_view ip, uint16_t port)
+{
+ // Check is IPv4 integer.
+ uint32_t ip_num;
+ int ret = ::inet_pton(AF_INET, ip.data(), &ip_num);
+ switch (ret) {
+ case 1:
+ // ::inet_pton() returns 1 on success (network address was
successfully converted)
+ return {ntohl(ip_num), port};
+ case -1:
+ LOG_ERROR(
+ "{}: fail to convert '{}:{}' to rpc_address",
utils::safe_strerror(errno), ip, port);
+ break;
+ default:
+ LOG_ERROR("'{}' does not contain a character string representing a
valid network address",
+ ip);
+ break;
}
- // check localhost & IP
- uint32_t ip_addr;
- if (ip == "localhost" || inet_pton(AF_INET, ip.c_str(), &ip_addr)) {
- assign_ipv4(ip.c_str(), (uint16_t)port_num);
- return true;
+ return {};
+}
+
+rpc_address rpc_address::from_host_port(std::string_view host_port)
+{
+ std::string_view host;
+ uint16_t port;
+ if (dsn_unlikely(!extract_host_port(host_port, host, port))) {
+ return {};
+ }
+
+ // Use std::string(host) to add a null terminator.
+ return from_host_port(std::string(host), port);
+}
+
+rpc_address rpc_address::from_host_port(std::string_view hostname, uint16_t
port)
+{
+ uint32_t ip = 0;
+ if (!ipv4_from_host(hostname.data(), &ip).is_ok()) {
+ return {};
}
- return false;
+
+ rpc_address addr;
+ addr._addr.v4.type = HOST_TYPE_IPV4;
+ addr._addr.v4.ip = ip;
+ addr._addr.v4.port = port;
+ return addr;
}
const char *rpc_address::to_string() const
{
char *p = bf.next();
- auto sz = bf.get_chunk_size();
- struct in_addr net_addr;
- int ip_len;
-
switch (_addr.v4.type) {
- case HOST_TYPE_IPV4:
+ case HOST_TYPE_IPV4: {
+ const auto sz = bf.get_chunk_size();
+ struct in_addr net_addr;
net_addr.s_addr = htonl(ip());
- inet_ntop(AF_INET, &net_addr, p, sz);
- ip_len = strlen(p);
+ ::inet_ntop(AF_INET, &net_addr, p, sz);
+ int ip_len = strlen(p);
Review Comment:
```suggestion
const auto ip_len = strlen(p);
```
##########
src/utils/test/hostname_test.cpp:
##########
@@ -69,8 +41,7 @@ TEST(ip_to_hostname, localhost)
const std::string valid_ip_port_list =
"127.0.0.1:8080,127.0.0.1:8080,127.0.0.1:8080";
const std::string expected_hostname_port_list =
"localhost:8080,localhost:8080,localhost:8080";
- rpc_address rpc_example_valid;
- rpc_example_valid.assign_ipv4(valid_ip.c_str(), 23010);
+ rpc_address rpc_example_valid = rpc_address::from_ip_port(valid_ip, 23010);
Review Comment:
```suggestion
const auto rpc_example_valid = rpc_address::from_ip_port(valid_ip,
23010);
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -68,23 +68,65 @@ error_s rpc_address::GetAddrInfo(const std::string
&hostname, const addrinfo &hi
return error_s::ok();
}
+bool extract_host_port(const std::string_view &host_port, std::string_view
&host, uint16_t &port)
+{
+ // Check the port field is present.
+ const auto pos = host_port.find_last_of(':');
+ if (dsn_unlikely(pos == std::string::npos)) {
+ LOG_ERROR("bad format, should be in the form of <ip|host>:port, but
got '{}'", host_port);
+ return false;
+ }
+
+ // Check port.
+ const auto port_str = host_port.substr(pos + 1);
+ if (dsn_unlikely(!dsn::buf2uint16(port_str, port))) {
+ LOG_ERROR("bad port, should be an uint16_t integer, but got '{}'",
port_str);
+ return false;
+ }
+
+ host = host_port.substr(0, pos);
+ return true;
+}
+
const rpc_address rpc_address::s_invalid_address;
+rpc_address::rpc_address(uint32_t ip, uint16_t port)
+{
+ _addr.v4.type = HOST_TYPE_IPV4;
+ _addr.v4.ip = ip;
+ _addr.v4.port = port;
+ static_assert(sizeof(rpc_address) == sizeof(uint64_t),
+ "make sure rpc_address does not add new payload to "
+ "rpc_address to keep it sizeof(uint64_t)");
+}
+
+rpc_address::rpc_address(const struct sockaddr_in &addr)
+{
+ _addr.v4.type = HOST_TYPE_IPV4;
+ _addr.v4.ip = static_cast<uint32_t>(ntohl(addr.sin_addr.s_addr));
+ _addr.v4.port = ntohs(addr.sin_port);
+}
+
+rpc_address::rpc_address(const rpc_address &another) { *this = another; }
+
+rpc_address::~rpc_address() { set_invalid(); }
+
/*static*/
-uint32_t rpc_address::ipv4_from_host(const char *name)
+error_s rpc_address::ipv4_from_host(const char *hostname, uint32_t *ip)
Review Comment:
Seems that `ipv4_from_host` has nothing to do with `rpc_address` ? Could we
move it to `utils` as a common tool that gets ip from host ?
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -68,23 +68,65 @@ error_s rpc_address::GetAddrInfo(const std::string
&hostname, const addrinfo &hi
return error_s::ok();
}
+bool extract_host_port(const std::string_view &host_port, std::string_view
&host, uint16_t &port)
+{
+ // Check the port field is present.
+ const auto pos = host_port.find_last_of(':');
+ if (dsn_unlikely(pos == std::string::npos)) {
+ LOG_ERROR("bad format, should be in the form of <ip|host>:port, but
got '{}'", host_port);
+ return false;
+ }
+
+ // Check port.
+ const auto port_str = host_port.substr(pos + 1);
+ if (dsn_unlikely(!dsn::buf2uint16(port_str, port))) {
+ LOG_ERROR("bad port, should be an uint16_t integer, but got '{}'",
port_str);
+ return false;
+ }
+
+ host = host_port.substr(0, pos);
+ return true;
+}
+
const rpc_address rpc_address::s_invalid_address;
+rpc_address::rpc_address(uint32_t ip, uint16_t port)
+{
+ _addr.v4.type = HOST_TYPE_IPV4;
+ _addr.v4.ip = ip;
+ _addr.v4.port = port;
+ static_assert(sizeof(rpc_address) == sizeof(uint64_t),
+ "make sure rpc_address does not add new payload to "
+ "rpc_address to keep it sizeof(uint64_t)");
+}
+
+rpc_address::rpc_address(const struct sockaddr_in &addr)
+{
+ _addr.v4.type = HOST_TYPE_IPV4;
+ _addr.v4.ip = static_cast<uint32_t>(ntohl(addr.sin_addr.s_addr));
+ _addr.v4.port = ntohs(addr.sin_port);
+}
+
+rpc_address::rpc_address(const rpc_address &another) { *this = another; }
Review Comment:
Use `rpc_address(const rpc_address &) = default;` instead ?
##########
src/runtime/rpc/rpc_host_port.cpp:
##########
@@ -86,7 +86,8 @@ host_port host_port::from_string(const std::string
&host_port_str)
return hp;
}
- if (dsn_unlikely(rpc_address::ipv4_from_host(hp._host.c_str()) == 0)) {
+ // Validate the hostname.
+ if (dsn_unlikely(!rpc_address::ipv4_from_host(hp._host.c_str(),
nullptr).is_ok())) {
Review Comment:
```suggestion
if (dsn_unlikely(!rpc_address::ipv4_from_host(hp._host.c_str(),
nullptr))) {
```
##########
src/meta/test/meta_backup_test.cpp:
##########
@@ -264,7 +264,7 @@ class backup_engine_test : public meta_test_base
void mock_on_backup_reply_when_timeout(int32_t partition_index, error_code
rpc_err)
{
gpid pid = gpid(_app_id, partition_index);
- rpc_address mock_primary_address = rpc_address("127.0.0.1", 10000 +
partition_index);
+ auto mock_primary_address = rpc_address::from_ip_port("127.0.0.1",
10000 + partition_index);
Review Comment:
```suggestion
const auto mock_primary_address =
rpc_address::from_ip_port("127.0.0.1", 10000 + partition_index);
```
##########
src/meta/test/meta_backup_test.cpp:
##########
@@ -250,7 +250,7 @@ class backup_engine_test : public meta_test_base
int32_t progress)
{
gpid pid = gpid(_app_id, partition_index);
- rpc_address mock_primary_address = rpc_address("127.0.0.1", 10000 +
partition_index);
+ auto mock_primary_address = rpc_address::from_ip_port("127.0.0.1",
10000 + partition_index);
Review Comment:
```suggestion
const auto mock_primary_address =
rpc_address::from_ip_port("127.0.0.1", 10000 + partition_index);
```
--
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]
