You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "GehaFearless (via GitHub)" <gi...@apache.org> on 2024/01/23 11:51:36 UTC
[PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
GehaFearless opened a new pull request, #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869
(no comment)
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1466245741
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
Review Comment:
Use dsn::defer() to release it automatically to prevent forgetting it.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 merged PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1469446201
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -27,44 +27,64 @@
#include "runtime/rpc/rpc_address.h"
#include <arpa/inet.h>
+#include <errno.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <netinet/in.h>
#include <string.h>
#include <sys/socket.h>
+#include "absl/strings/string_view.h"
#include "runtime/rpc/group_address.h"
+#include "utils/error_code.h"
#include "utils/fixed_size_buffer_pool.h"
#include "utils/fmt_logging.h"
#include "utils/ports.h"
+#include "utils/safe_strerror_posix.h"
#include "utils/string_conv.h"
-#include "absl/strings/string_view.h"
#include "utils/strings.h"
namespace dsn {
+/*static*/
+error_s rpc_address::GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
Review Comment:
This function may be slow, it would be better to add cache as well, also add metrics and slow logs like [1] and [2]. I will merge them ASAP.
1. https://github.com/apache/incubator-pegasus/pull/1873
2. https://github.com/apache/incubator-pegasus/pull/1879
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1465914463
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+
+ if (rc != 0) {
+ if (rc == EAI_SYSTEM) {
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, utils::safe_strerror(err));
+ }
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, gai_strerror(err));
+ return 0;
}
+ CHECK(res->ai_family == AF_INET, "");
+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)res->ai_addr;
Review Comment:
```suggestion
auto *ipv4 = reinterpret_cast<struct sockaddr_in *>(res->ai_addr);
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
Review Comment:
Use `freeaddrinfo()` to release memory.
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+
+ if (rc != 0) {
+ if (rc == EAI_SYSTEM) {
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, utils::safe_strerror(err));
+ }
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, gai_strerror(err));
+ return 0;
}
+ CHECK(res->ai_family == AF_INET, "");
+ struct sockaddr_in *ipv4 = (struct sockaddr_in *)res->ai_addr;
// converts from network byte order to host byte order
- return (uint32_t)ntohl(addr.sin_addr.s_addr);
+ return (uint32_t)ntohl(ipv4->sin_addr.s_addr);
Review Comment:
No need to do conversion, since the return type has been `uint32_t`.
```
#include <arpa/inet.h>
uint32_t ntohl(uint32_t netlong);
```
```suggestion
return ntohl(ipv4->sin_addr.s_addr);
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+
+ if (rc != 0) {
+ if (rc == EAI_SYSTEM) {
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, utils::safe_strerror(err));
+ }
+ LOG_ERROR("getaddrinfo failed, name = {}, err = {}", name, gai_strerror(err));
+ return 0;
}
+ CHECK(res->ai_family == AF_INET, "");
Review Comment:
```suggestion
CHECK_EQ(res->ai_family, AF_INET);
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+
+ if (rc != 0) {
Review Comment:
```suggestion
if (dsn_unlikely(rc != 0)) {
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1470559335
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -27,44 +27,64 @@
#include "runtime/rpc/rpc_address.h"
#include <arpa/inet.h>
+#include <errno.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <netinet/in.h>
#include <string.h>
#include <sys/socket.h>
+#include "absl/strings/string_view.h"
#include "runtime/rpc/group_address.h"
+#include "utils/error_code.h"
#include "utils/fixed_size_buffer_pool.h"
#include "utils/fmt_logging.h"
#include "utils/ports.h"
+#include "utils/safe_strerror_posix.h"
#include "utils/string_conv.h"
-#include "absl/strings/string_view.h"
#include "utils/strings.h"
namespace dsn {
+/*static*/
+error_s rpc_address::GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
Review Comment:
We can do it later.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1470606496
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -27,44 +27,64 @@
#include "runtime/rpc/rpc_address.h"
#include <arpa/inet.h>
+#include <errno.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <netinet/in.h>
#include <string.h>
#include <sys/socket.h>
+#include "absl/strings/string_view.h"
#include "runtime/rpc/group_address.h"
+#include "utils/error_code.h"
#include "utils/fixed_size_buffer_pool.h"
#include "utils/fmt_logging.h"
#include "utils/ports.h"
+#include "utils/safe_strerror_posix.h"
#include "utils/string_conv.h"
-#include "absl/strings/string_view.h"
#include "utils/strings.h"
namespace dsn {
+/*static*/
+error_s rpc_address::GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
+{
+ addrinfo *res = nullptr;
+ const int rc = getaddrinfo(hostname.c_str(), nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+ AddrInfo result(res, ::freeaddrinfo);
+ if (rc != 0) {
Review Comment:
```suggestion
if (dsn_unlikely(rc != 0)) {
```
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -27,44 +27,64 @@
#include "runtime/rpc/rpc_address.h"
#include <arpa/inet.h>
+#include <errno.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <netinet/in.h>
#include <string.h>
#include <sys/socket.h>
+#include "absl/strings/string_view.h"
#include "runtime/rpc/group_address.h"
+#include "utils/error_code.h"
#include "utils/fixed_size_buffer_pool.h"
#include "utils/fmt_logging.h"
#include "utils/ports.h"
+#include "utils/safe_strerror_posix.h"
#include "utils/string_conv.h"
-#include "absl/strings/string_view.h"
#include "utils/strings.h"
namespace dsn {
+/*static*/
+error_s rpc_address::GetAddrInfo(const std::string &hostname, const addrinfo &hints, AddrInfo *info)
+{
+ addrinfo *res = nullptr;
+ const int rc = getaddrinfo(hostname.c_str(), nullptr, &hints, &res);
+ const int err = errno; // preserving the errno from the getaddrinfo() call
+ AddrInfo result(res, ::freeaddrinfo);
+ if (rc != 0) {
+ if (rc == EAI_SYSTEM) {
+ LOG_ERROR(
+ "getaddrinfo failed, name = {}, err = {}", hostname, utils::safe_strerror(rc));
+ return error_s::make(ERR_NETWORK_FAILURE, utils::safe_strerror(err));
Review Comment:
```suggestion
const auto &err_msg = utils::safe_strerror(err);
LOG_ERROR(
"getaddrinfo failed, name = {}, err = {}", hostname, err_msg);
return error_s::make(ERR_NETWORK_FAILURE, err_msg);
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat: use getaddrinfo instead of gethostbyname [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1869:
URL: https://github.com/apache/incubator-pegasus/pull/1869#discussion_r1466245741
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
Review Comment:
Use dsn::defer() to release it automatically to prevent forgetting it.
##########
src/runtime/rpc/rpc_address.cpp:
##########
@@ -48,23 +50,27 @@ const rpc_address rpc_address::s_invalid_address;
/*static*/
uint32_t rpc_address::ipv4_from_host(const char *name)
{
- sockaddr_in addr;
- memset(&addr, 0, sizeof(addr));
-
- addr.sin_family = AF_INET;
- if ((addr.sin_addr.s_addr = inet_addr(name)) == (unsigned int)(-1)) {
- // TODO(yingchun): use getaddrinfo instead
- hostent *hp = ::gethostbyname(name);
- if (dsn_unlikely(hp == nullptr)) {
- LOG_ERROR("gethostbyname failed, name = {}, err = {}", name, hstrerror(h_errno));
- return 0;
- }
+ struct addrinfo hints;
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_INET;
+ hints.ai_socktype = SOCK_STREAM;
+ addrinfo *res = nullptr;
- memcpy((void *)&(addr.sin_addr.s_addr), (const void *)hp->h_addr, (size_t)hp->h_length);
+ const int rc = getaddrinfo(name, nullptr, &hints, &res);
Review Comment:
There is a similar implementation `GetAddrInfo()` in rpc_host_port.cpp, how about reusing it to reduce duplicate code?
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org