[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-12-01 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r533555103



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the
+ * path of the unix domain socket, or NULL to build an 
address

Review comment:
   Darn, I missed that one.





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-12-01 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r533486240



##
File path: xdocs/index.xml
##
@@ -174,6 +175,14 @@ list of changes.
 
   Please see the Apache Tomcat documentation for configuration specifics.
 
+
+
+  When using unix domain sockets a cleanup is registered to delete the

Review comment:
   Last nit, use Unix Domain Sockets (uppercase) consistently.





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532264241



##
File path: native/src/network.c
##
@@ -316,6 +337,11 @@ TCN_IMPLEMENT_CALL(jint, Socket, bind)(TCN_STDARGS, jlong 
sock,
 TCN_ASSERT(sock != 0);
 TCN_ASSERT(s->sock != NULL);
 rv = (jint)apr_socket_bind(s->sock, a);
+
+apr_pool_cleanup_register(s->pool, (const void *)s,

Review comment:
   OK, can you document the procedure in the case of a crash scenario?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532261154



##
File path: native/src/network.c
##
@@ -316,6 +337,11 @@ TCN_IMPLEMENT_CALL(jint, Socket, bind)(TCN_STDARGS, jlong 
sock,
 TCN_ASSERT(sock != 0);
 TCN_ASSERT(s->sock != NULL);
 rv = (jint)apr_socket_bind(s->sock, a);
+
+apr_pool_cleanup_register(s->pool, (const void *)s,

Review comment:
   OK, so basically when the Tomcat instance is shut down and all memory 
pools cleared, the file is removed. Is the socket information retained that 
long? What will happen when the process crashes, will a restart rebind to the 
same sock file with `REUSEADDR`?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532255441



##
File path: native/src/network.c
##
@@ -79,6 +80,17 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+#ifdef APR_UNIX
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {
+char *sock;
+apr_getnameinfo(, sa, 0);
+if (sock) {
+apr_file_remove(sock, s->pool);

Review comment:
   This is an interesting one, the file is deleted *before* the socket is 
closed?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532255441



##
File path: native/src/network.c
##
@@ -79,6 +80,17 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+#ifdef APR_UNIX
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {
+char *sock;
+apr_getnameinfo(, sa, 0);
+if (sock) {
+apr_file_remove(sock, s->pool);

Review comment:
   This isn't an interesting one, the file is deleted *before* the socket 
is closed?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532254789



##
File path: native/include/tcn.h
##
@@ -281,11 +281,20 @@ typedef struct {
 #define APR_INET6 APR_INET
 #endif
 
+#ifdef APR_UNIX
 #define GET_S_FAMILY(T, F)   \
 if (F == 0) T = APR_UNSPEC;  \
 else if (F == 1) T = APR_INET;   \
 else if (F == 2) T = APR_INET6;  \
+else if (F == 3) T = APR_UNIX;   \

Review comment:
   Right, I completely missed this one that this is a macro. Apologies.





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532254706



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the

Review comment:
   OK, consistency makes sense. I wonder why this has not been addressed in 
1.7 or 2.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.

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



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[GitHub] [tomcat-native] michael-o commented on a change in pull request #8: Expose support for Unix Domain Sockets in APR v1.6 and up.

2020-11-29 Thread GitBox 


michael-o commented on a change in pull request #8:
URL: https://github.com/apache/tomcat-native/pull/8#discussion_r532249132



##
File path: java/org/apache/tomcat/jni/Address.java
##
@@ -40,8 +40,9 @@
 
 /**
  * Create apr_sockaddr_t from hostname, address family, and port.
- * @param hostname The hostname or numeric address string to 
resolve/parse, or
- *   NULL to build an address that corresponds to 0.0.0.0 or ::
+ * @param hostname The hostname or numeric address string to 
resolve/parse, the

Review comment:
   I think the generic name is address which represents `hostname:port`,  
`IPv4:port' , `[IPv6]:port`, '/path`. Should we deprecate this one and replace 
with a new one?

##
File path: native/src/info.c
##
@@ -198,6 +198,9 @@ static void fill_ainfo(JNIEnv *e, jobject obj, 
apr_sockaddr_t *info)
 if (info->family == APR_UNSPEC) f = 0;
 else if (info->family == APR_INET) f = 1;
 else if (info->family == APR_INET6) f = 2;
+#ifdef APR_UNIX
+else if (info->family == APR_UNIX) f = 3;

Review comment:
   Just like here...

##
File path: native/src/network.c
##
@@ -79,6 +80,15 @@ static apr_status_t sp_socket_cleanup(void *data)
 (*s->net->cleanup)(s->opaque);
 if (s->sock) {
 apr_socket_t *as = s->sock;
+apr_sockaddr_t *sa = NULL;
+apr_socket_addr_get(, APR_LOCAL, as);
+if (sa && sa->family == APR_UNIX) {

Review comment:
   If you guard all the rest with `#ifdef APR_UNIX` why not this one?

##
File path: native/include/tcn.h
##
@@ -281,11 +281,20 @@ typedef struct {
 #define APR_INET6 APR_INET
 #endif
 
+#ifdef APR_UNIX
 #define GET_S_FAMILY(T, F)   \
 if (F == 0) T = APR_UNSPEC;  \
 else if (F == 1) T = APR_INET;   \
 else if (F == 2) T = APR_INET6;  \
+else if (F == 3) T = APR_UNIX;   \

Review comment:
   Why not make the ifdef around this line?





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:
us...@infra.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org