You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by sansanichfb <gi...@git.apache.org> on 2016/10/04 01:10:14 UTC
[GitHub] incubator-hawq pull request #951: HAWQ-1083
GitHub user sansanichfb opened a pull request:
https://github.com/apache/incubator-hawq/pull/951
HAWQ-1083
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sansanichfb/incubator-hawq HAWQ-1083
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/951.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #951
----
commit a27d02edba36d56ab03eaeedf2b7a97ba31c940e
Author: Oleksandr Diachenko <od...@pivotal.io>
Date: 2016-10-03T23:35:54Z
Added substitution of localhost string with ip address of any loopback interface.
commit dcf07669a1310f6f9166afbe929dc7ae7cffd272
Author: Oleksandr Diachenko <od...@pivotal.io>
Date: 2016-10-04T01:06:02Z
HAWQ-1083. Added comments.
commit 8e68ef059164e2e466d129d3f61773ebf0150d1e
Author: Oleksandr Diachenko <od...@pivotal.io>
Date: 2016-10-04T01:08:25Z
HAWQ-1083. Added comments.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81798661
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
--- End diff --
Can't this be inlined into the if statement
`if ((start = strstr(url, LocalhostIpV4) != NULL)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81862293
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
+ //allocate new url with replaced host
+ char* replaced_url = palloc(strlen(url) + strlen(loopback_addr) - strlen(LocalhostIpV4) + 1);
+ //token before host
+ char* before_host = pnstrdup(url, start - url);
+ //token after host
+ char* after_host = pstrdup(url + (start - url) + strlen(LocalhostIpV4));
+ //construct replaced url using loopback interface ip
+ sprintf(replaced_url, "%s%s%s", before_host, loopback_addr, after_host);
+ set_curl_option(context, CURLOPT_URL, replaced_url);
+
+ //release memory
+ pfree(before_host);
+ pfree(after_host);
+ pfree(replaced_url);
+ pfree(loopback_addr);
+ } else
+ set_curl_option(context, CURLOPT_URL, url);
--- End diff --
Two cases are different, because in case if url contains "localhost" string I am allocating new string with replaced localhost to actual ip address thus I need to release memory in this case. In other case I do not need to free any memory because I haven't allocated any.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81863084
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
--- End diff --
Ok, makes sense. I overlooked the previous line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81830370
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
+ }
+ else
+ {
+ loopback_addr = palloc(strlen(host) + 3);
+ sprintf(loopback_addr, "[%s]", host);
+ }
+ break;
+ }
+ }
+ }
+
+ freeifaddrs(ifaddr);
+
+ if (loopback_addr == NULL)
--- End diff --
Sure, will add check.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81801521
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
+ }
+ else
+ {
+ loopback_addr = palloc(strlen(host) + 3);
+ sprintf(loopback_addr, "[%s]", host);
+ }
+ break;
+ }
+ }
+ }
+
+ freeifaddrs(ifaddr);
+
+ if (loopback_addr == NULL)
--- End diff --
Maybe also check that `loopback_addr` is not also an empty string
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by denalex <gi...@git.apache.org>.
Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81803797
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
+ //allocate new url with replaced host
+ char* replaced_url = palloc(strlen(url) + strlen(loopback_addr) - strlen(LocalhostIpV4) + 1);
+ //token before host
+ char* before_host = pnstrdup(url, start - url);
+ //token after host
+ char* after_host = pstrdup(url + (start - url) + strlen(LocalhostIpV4));
+ //construct replaced url using loopback interface ip
+ sprintf(replaced_url, "%s%s%s", before_host, loopback_addr, after_host);
+ set_curl_option(context, CURLOPT_URL, replaced_url);
+
+ //release memory
+ pfree(before_host);
+ pfree(after_host);
+ pfree(replaced_url);
+ pfree(loopback_addr);
+ } else
+ set_curl_option(context, CURLOPT_URL, url);
--- End diff --
this statement is executed both in if and else branches -- would be cleaner to just overwrite "url" ref in if branch, drop else and have the statement after the block:
url = default
if (problem) {
url = new_stuff
}
set_option(url)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81869294
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
+ //allocate new url with replaced host
+ char* replaced_url = palloc(strlen(url) + strlen(loopback_addr) - strlen(LocalhostIpV4) + 1);
+ //token before host
+ char* before_host = pnstrdup(url, start - url);
+ //token after host
+ char* after_host = pstrdup(url + (start - url) + strlen(LocalhostIpV4));
+ //construct replaced url using loopback interface ip
+ sprintf(replaced_url, "%s%s%s", before_host, loopback_addr, after_host);
--- End diff --
Sounds good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81796370
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
+ }
+ else
+ {
+ loopback_addr = palloc(strlen(host) + 3);
+ sprintf(loopback_addr, "[%s]", host);
+ }
+ break;
+ }
+ }
+ }
+
+ freeifaddrs(ifaddr);
+
+ if (loopback_addr == NULL)
+ elog(ERROR, "Unable to get loop back address");
--- End diff --
Should we instead just default to using 127.0.0.1 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81869079
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
+ //allocate new url with replaced host
+ char* replaced_url = palloc(strlen(url) + strlen(loopback_addr) - strlen(LocalhostIpV4) + 1);
+ //token before host
+ char* before_host = pnstrdup(url, start - url);
+ //token after host
+ char* after_host = pstrdup(url + (start - url) + strlen(LocalhostIpV4));
+ //construct replaced url using loopback interface ip
+ sprintf(replaced_url, "%s%s%s", before_host, loopback_addr, after_host);
--- End diff --
Checked through HAWQ code - haven't found much null checks after pallocating memory. I guess palloc might check for NULL inside.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb closed the pull request at:
https://github.com/apache/incubator-hawq/pull/951
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81830293
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
--- End diff --
I am allocating memory for it: loopback_addr = palloc(strlen(host) + 1) so loopback_addr should have enough memory.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81800773
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
--- End diff --
If length of `host` is bigger than `loopback_addr` then there will be a buffer overflow, safer to use `snprintf()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81829710
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
--- End diff --
Good idea, will move logic to function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81801574
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
--- End diff --
Would be good if we can move below lines till you contract the new url with the loopback_addr to a new function in pxfutils.c named replaceUri(char *uri, char *hostname) and invoke replaceUri(uri, loopback_addr)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81829992
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
--- End diff --
We already have condition if (strstr(url, LocalhostIpV4) != NULL) above.
Do you think we need to double check it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81824408
--- Diff: src/backend/access/external/pxfutils.c ---
@@ -133,3 +137,60 @@ static void process_request(ClientContext* client_context, char *uri)
}
+
+/*
+ * Finds ip address of any available loopback interface(ipv4/ipv6).
+ * Returns ip for ipv4 addresses and [ip] for ipv6 addresses.
+ *
+ */
+char* get_loopback_ip_addr() {
+ struct ifaddrs *ifaddr, *ifa;
+ int family, s, n;
+ char host[NI_MAXHOST];
+ char *loopback_addr;
+
+ if (getifaddrs(&ifaddr) == -1) {
+ elog(ERROR, "Unable to obtain list of network interfaces.");
+ }
+
+ for (ifa = ifaddr, n = 0; ifa != NULL; ifa = ifa->ifa_next, n++) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+
+ family = ifa->ifa_addr->sa_family;
+
+ if (family == AF_INET || family == AF_INET6) {
+ s = getnameinfo(ifa->ifa_addr,
+ (family == AF_INET) ?
+ sizeof(struct sockaddr_in) :
+ sizeof(struct sockaddr_in6), host, NI_MAXHOST,
+ NULL, 0, NI_NUMERICHOST);
+ if (s != 0) {
+ elog(WARNING, "Unable to get name information for interface, getnameinfo() failed: %s\n", gai_strerror(s));
+ }
+
+ //get loopback interface
+ if (ifa->ifa_flags & IFF_LOOPBACK) {
+ if (family == AF_INET)
+ {
+ loopback_addr = palloc(strlen(host) + 1);
+ sprintf(loopback_addr, "%s", host);
+ }
+ else
+ {
+ loopback_addr = palloc(strlen(host) + 3);
+ sprintf(loopback_addr, "[%s]", host);
+ }
+ break;
+ }
+ }
+ }
+
+ freeifaddrs(ifaddr);
+
+ if (loopback_addr == NULL)
+ elog(ERROR, "Unable to get loop back address");
--- End diff --
If function returned null it means there is no loopback interfaces available, and even if we return "127.0.0.1" no interface could handle requests to PXF.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81865332
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
--- End diff --
Makes sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq issue #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on the issue:
https://github.com/apache/incubator-hawq/pull/951
+1 aside from comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #951: HAWQ-1083
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/951#discussion_r81802588
--- Diff: src/backend/access/external/libchurl.c ---
@@ -312,16 +313,29 @@ CHURL_HANDLE churl_init(const char* url, CHURL_HEADERS headers)
/* needed to resolve localhost */
if (strstr(url, LocalhostIpV4) != NULL) {
- struct curl_slist *resolve_hosts = NULL;
- char *pxf_host_entry = (char *) palloc0(strlen(pxf_service_address) + strlen(LocalhostIpV4Entry) + 1);
- strcat(pxf_host_entry, pxf_service_address);
- strcat(pxf_host_entry, LocalhostIpV4Entry);
- resolve_hosts = curl_slist_append(NULL, pxf_host_entry);
- set_curl_option(context, CURLOPT_RESOLVE, resolve_hosts);
- pfree(pxf_host_entry);
- }
+ //get loopback interface ip address
+ char* loopback_addr = get_loopback_ip_addr();
+ elog(DEBUG1, "Loopback interface IP address: %s", loopback_addr);
+ //find host start position in url
+ char* start = strstr(url, LocalhostIpV4);
+ //allocate new url with replaced host
+ char* replaced_url = palloc(strlen(url) + strlen(loopback_addr) - strlen(LocalhostIpV4) + 1);
+ //token before host
+ char* before_host = pnstrdup(url, start - url);
+ //token after host
+ char* after_host = pstrdup(url + (start - url) + strlen(LocalhostIpV4));
+ //construct replaced url using loopback interface ip
+ sprintf(replaced_url, "%s%s%s", before_host, loopback_addr, after_host);
--- End diff --
Maybe null check the replaced_url, but it might just be overkill
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---