[GitHub] [tvm] mdw-octoml commented on a change in pull request #7766: Replace 0.0.0.0 with 127.0.0.1 for client connections

[GitBox <gi...@apache.org>]

mdw-octoml commented on a change in pull request #7766:
URL: https://github.com/apache/tvm/pull/7766#discussion_r613356425



##########
File path: python/tvm/rpc/proxy.py
##########
@@ -512,7 +512,7 @@ def __init__(
                 self.port = my_port
                 break
             except socket.error as sock_err:
-                if sock_err.errno in [98, 48]:
+                if sock_err.errno in [98, 48, 10098, 10048]:

Review comment:
       I would recommend using symbols from the `errno` library here instead of hardcoding values.
   If that does not work, could you at least add a comment here indicating what these error codes are?

##########
File path: python/tvm/rpc/server.py
##########
@@ -419,7 +419,7 @@ def __init__(
                     self.port = my_port
                     break
                 except socket.error as sock_err:
-                    if sock_err.errno in [98, 48]:
+                    if sock_err.errno in [98, 48, 10098, 10048]:

Review comment:
       See comment above on errnos.

##########
File path: src/support/socket.h
##########
@@ -118,7 +118,7 @@ struct SockAddr {
     std::string host = url.substr(2, sep - 3);
     std::string port = url.substr(sep + 1, url.length() - 1);
     ICHECK(ValidateIP(host)) << "Url address is not valid " << url;
-    if (host == "localhost") {
+    if (host == "127.0.0.1") {

Review comment:
       This should not be necessary, since it's already that value :-)

##########
File path: src/support/socket.h
##########
@@ -85,7 +85,7 @@ inline std::string GetHostName() {
  * \return result of operation.
  */
 inline bool ValidateIP(std::string ip) {
-  if (ip == "localhost") {
+  if (ip == "127.0.0.1") {

Review comment:
       This should not be necessary since "127.0.0.1" should pass the is_ipv4 check below.

##########
File path: tests/python/unittest/test_runtime_rpc.py
##########
@@ -45,7 +45,7 @@
 # run individual functions. Somewhere along the way, the imports are being
 # lost, so the server ends up not registering the functions.
 pytestmark = pytest.mark.skipif(
-    multiprocessing.get_start_method() != "fork",
+    sys.platform.startswith("win") == False and multiprocessing.get_start_method() != "fork",

Review comment:
       Could you add a comment here why Windows is being excluded here?

##########
File path: python/tvm/rpc/tracker.py
##########
@@ -399,7 +399,7 @@ def __init__(self, host, port=9190, port_end=9199, silent=False):
                 self.port = my_port
                 break
             except socket.error as sock_err:
-                if sock_err.errno in [98, 48]:
+                if sock_err.errno in [98, 48, 10098, 10048]:

Review comment:
       See comment above on errnos.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org