You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fr0stbyte <gi...@git.apache.org> on 2017/12/08 05:22:09 UTC
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
GitHub user fr0stbyte opened a pull request:
https://github.com/apache/zookeeper/pull/427
[ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defined
@phunt @anmolnar
PR for 3.4 branch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fr0stbyte/zookeeper fr0stbyte/ZOOKEEPER-2338-3.4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/427.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 #427
----
commit 025142999b3fee596d6f41d63e0076582cca539f
Author: Radu Brumariu <ra...@groupon.com>
Date: 2017-10-25T18:24:17Z
[ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defined
----
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by mfenes <gi...@git.apache.org>.
Github user mfenes commented on the issue:
https://github.com/apache/zookeeper/pull/427
@fr0stbyte could you please add a description to your PR so that everybody could understand the purpose of this change?
---
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/427#discussion_r157325096
--- Diff: src/c/configure.ac ---
@@ -130,18 +130,66 @@ main()
else
exit(0);
}
-], AC_MSG_RESULT(yes)
- ipv6=yes,
- AC_MSG_RESULT(no)
- ipv6=no,
- AC_MSG_RESULT(no)
+], AC_MSG_RESULT(yes)
+ ipv6=yes,
+ AC_MSG_RESULT(no)
+ ipv6=no,
+ AC_MSG_RESULT(no)
ipv6=no)
if test x"$ipv6" = xyes; then
USEIPV6="-DZOO_IPV6_ENABLED"
AC_SUBST(USEIPV6)
fi
+# use SOCK_CLOEXEC if available and wanted
+AC_ARG_WITH([sock_cloexec],
+[AS_HELP_STRING([--with-sock-cloexec],[build with SOCK_CLOEXEC flag set on the connections])],
+[],[with_sock_cloexec=no])
+
+AC_MSG_CHECKING([whether SOCK_CLOEXEC is available])
+
+AC_TRY_RUN([ /* is SOCK_CLOEXEC available ? */
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <stdlib.h>
+main()
+{
+#ifdef SOCK_CLOEXEC
+ exit(0);
+#else
+ exit(1);
+#endif
+}
+], AC_MSG_RESULT(yes)
+ has_sock_cloexec=yes,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no)
+
+if test "x$with_sock_cloexec" != xno && test "x$has_sock_cloexec" = xno; then
+ AC_MSG_WARN([cannot use SOCK_CLOEXEC -- SOCK_CLOEXEC undefined on this platform])
+ with_sock_cloexec=no
+fi
+
+if test "x$with_sock_cloexec" != xno; then
+AC_MSG_NOTICE([building with SOCK_CLOEXEC])
+else
+AC_MSG_NOTICE([building without SOCK_CLOEXEC])
+fi
+
+AS_IF([test x"$with_sock_cloexec" != xno], [AC_DEFINE([SOCK_CLOEXEC_ENABLED], [1], [Define to 1, if SOCK_CLOEXEC is defined and wanted])])
+AM_CONDITIONAL([SOCK_CLOEXEC_ENABLED],[test "x$with_sock_cloexec" != xno])
+
+# Determine which libraries we need to use clock_gettime
--- End diff --
Sorry I was unclear @fr0stbyte - I meant the following:
+# Determine which libraries we need to use clock_gettime
+saved_LIBS="$LIBS"
+LIBS=""
+AC_CHECK_LIB(rt, clock_gettime)
+CLOCK_GETTIME_LIBS=$LIBS
+AC_SUBST(CLOCK_GETTIME_LIBS)
+LIBS="$saved_LIBS"
those lines are included in this patch, but not #410
Could you remove them? Or am I totally missing something?
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/427
@fr0stbyte no thanks, that's fine, I've dug it out already.
Going forward I suggest to make linter / indentation / whitespace changes in a commit separate from the code changes.
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/427
For this one I'm fine with the whitespace as it also got into the trunk/3.5 commit.
It's unfortunate that github doesn't have an option to remove whitespace changes from the diff they show but you can get around this by diffing the commit locally. (which is what I typically do)
Agree with @mfenes 's comment - even though this is a backport it's still nice to have.
Thanks @fr0stbyte !
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/427
@phunt thanks, will keep that in mind.
---
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/427#discussion_r156860284
--- Diff: src/c/configure.ac ---
@@ -130,18 +130,66 @@ main()
else
exit(0);
}
-], AC_MSG_RESULT(yes)
- ipv6=yes,
- AC_MSG_RESULT(no)
- ipv6=no,
- AC_MSG_RESULT(no)
+], AC_MSG_RESULT(yes)
+ ipv6=yes,
+ AC_MSG_RESULT(no)
+ ipv6=no,
+ AC_MSG_RESULT(no)
ipv6=no)
if test x"$ipv6" = xyes; then
USEIPV6="-DZOO_IPV6_ENABLED"
AC_SUBST(USEIPV6)
fi
+# use SOCK_CLOEXEC if available and wanted
+AC_ARG_WITH([sock_cloexec],
+[AS_HELP_STRING([--with-sock-cloexec],[build with SOCK_CLOEXEC flag set on the connections])],
+[],[with_sock_cloexec=no])
+
+AC_MSG_CHECKING([whether SOCK_CLOEXEC is available])
+
+AC_TRY_RUN([ /* is SOCK_CLOEXEC available ? */
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <stdlib.h>
+main()
+{
+#ifdef SOCK_CLOEXEC
+ exit(0);
+#else
+ exit(1);
+#endif
+}
+], AC_MSG_RESULT(yes)
+ has_sock_cloexec=yes,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no)
+
+if test "x$with_sock_cloexec" != xno && test "x$has_sock_cloexec" = xno; then
+ AC_MSG_WARN([cannot use SOCK_CLOEXEC -- SOCK_CLOEXEC undefined on this platform])
+ with_sock_cloexec=no
+fi
+
+if test "x$with_sock_cloexec" != xno; then
+AC_MSG_NOTICE([building with SOCK_CLOEXEC])
+else
+AC_MSG_NOTICE([building without SOCK_CLOEXEC])
+fi
+
+AS_IF([test x"$with_sock_cloexec" != xno], [AC_DEFINE([SOCK_CLOEXEC_ENABLED], [1], [Define to 1, if SOCK_CLOEXEC is defined and wanted])])
+AM_CONDITIONAL([SOCK_CLOEXEC_ENABLED],[test "x$with_sock_cloexec" != xno])
+
+# Determine which libraries we need to use clock_gettime
--- End diff --
@phunt I think it was : https://github.com/apache/zookeeper/pull/410/files#diff-79599273360e5b5ede514927c8033356R182
---
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/427#discussion_r156800318
--- Diff: src/c/configure.ac ---
@@ -130,18 +130,66 @@ main()
else
exit(0);
}
-], AC_MSG_RESULT(yes)
- ipv6=yes,
- AC_MSG_RESULT(no)
- ipv6=no,
- AC_MSG_RESULT(no)
+], AC_MSG_RESULT(yes)
+ ipv6=yes,
+ AC_MSG_RESULT(no)
+ ipv6=no,
+ AC_MSG_RESULT(no)
ipv6=no)
if test x"$ipv6" = xyes; then
USEIPV6="-DZOO_IPV6_ENABLED"
AC_SUBST(USEIPV6)
fi
+# use SOCK_CLOEXEC if available and wanted
+AC_ARG_WITH([sock_cloexec],
+[AS_HELP_STRING([--with-sock-cloexec],[build with SOCK_CLOEXEC flag set on the connections])],
+[],[with_sock_cloexec=no])
+
+AC_MSG_CHECKING([whether SOCK_CLOEXEC is available])
+
+AC_TRY_RUN([ /* is SOCK_CLOEXEC available ? */
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <stdlib.h>
+main()
+{
+#ifdef SOCK_CLOEXEC
+ exit(0);
+#else
+ exit(1);
+#endif
+}
+], AC_MSG_RESULT(yes)
+ has_sock_cloexec=yes,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no)
+
+if test "x$with_sock_cloexec" != xno && test "x$has_sock_cloexec" = xno; then
+ AC_MSG_WARN([cannot use SOCK_CLOEXEC -- SOCK_CLOEXEC undefined on this platform])
+ with_sock_cloexec=no
+fi
+
+if test "x$with_sock_cloexec" != xno; then
+AC_MSG_NOTICE([building with SOCK_CLOEXEC])
+else
+AC_MSG_NOTICE([building without SOCK_CLOEXEC])
+fi
+
+AS_IF([test x"$with_sock_cloexec" != xno], [AC_DEFINE([SOCK_CLOEXEC_ENABLED], [1], [Define to 1, if SOCK_CLOEXEC is defined and wanted])])
+AM_CONDITIONAL([SOCK_CLOEXEC_ENABLED],[test "x$with_sock_cloexec" != xno])
+
+# Determine which libraries we need to use clock_gettime
--- End diff --
I believe you may have pulled this in by accident? It's not part of the 3.5/trunk patch. Please remove this section (if I read that right).
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/427
@phunt @mfenes updated the initial comment of this PR. Let me know if you need me to move it into a separate comment
---
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/427#discussion_r162510701
--- Diff: src/c/configure.ac ---
@@ -130,18 +130,66 @@ main()
else
exit(0);
}
-], AC_MSG_RESULT(yes)
- ipv6=yes,
- AC_MSG_RESULT(no)
- ipv6=no,
- AC_MSG_RESULT(no)
+], AC_MSG_RESULT(yes)
+ ipv6=yes,
+ AC_MSG_RESULT(no)
+ ipv6=no,
+ AC_MSG_RESULT(no)
ipv6=no)
if test x"$ipv6" = xyes; then
USEIPV6="-DZOO_IPV6_ENABLED"
AC_SUBST(USEIPV6)
fi
+# use SOCK_CLOEXEC if available and wanted
+AC_ARG_WITH([sock_cloexec],
+[AS_HELP_STRING([--with-sock-cloexec],[build with SOCK_CLOEXEC flag set on the connections])],
+[],[with_sock_cloexec=no])
+
+AC_MSG_CHECKING([whether SOCK_CLOEXEC is available])
+
+AC_TRY_RUN([ /* is SOCK_CLOEXEC available ? */
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <stdlib.h>
+main()
+{
+#ifdef SOCK_CLOEXEC
+ exit(0);
+#else
+ exit(1);
+#endif
+}
+], AC_MSG_RESULT(yes)
+ has_sock_cloexec=yes,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no)
+
+if test "x$with_sock_cloexec" != xno && test "x$has_sock_cloexec" = xno; then
+ AC_MSG_WARN([cannot use SOCK_CLOEXEC -- SOCK_CLOEXEC undefined on this platform])
+ with_sock_cloexec=no
+fi
+
+if test "x$with_sock_cloexec" != xno; then
+AC_MSG_NOTICE([building with SOCK_CLOEXEC])
+else
+AC_MSG_NOTICE([building without SOCK_CLOEXEC])
+fi
+
+AS_IF([test x"$with_sock_cloexec" != xno], [AC_DEFINE([SOCK_CLOEXEC_ENABLED], [1], [Define to 1, if SOCK_CLOEXEC is defined and wanted])])
+AM_CONDITIONAL([SOCK_CLOEXEC_ENABLED],[test "x$with_sock_cloexec" != xno])
+
+# Determine which libraries we need to use clock_gettime
--- End diff --
@fr0stbyte I'd still like to get this committed - does my most recent comment make sense or am I missing something?
---
[GitHub] zookeeper pull request #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/427#discussion_r156801636
--- Diff: src/c/configure.ac ---
@@ -130,18 +130,66 @@ main()
else
exit(0);
}
-], AC_MSG_RESULT(yes)
- ipv6=yes,
- AC_MSG_RESULT(no)
- ipv6=no,
- AC_MSG_RESULT(no)
+], AC_MSG_RESULT(yes)
+ ipv6=yes,
+ AC_MSG_RESULT(no)
+ ipv6=no,
+ AC_MSG_RESULT(no)
ipv6=no)
if test x"$ipv6" = xyes; then
USEIPV6="-DZOO_IPV6_ENABLED"
AC_SUBST(USEIPV6)
fi
+# use SOCK_CLOEXEC if available and wanted
+AC_ARG_WITH([sock_cloexec],
+[AS_HELP_STRING([--with-sock-cloexec],[build with SOCK_CLOEXEC flag set on the connections])],
+[],[with_sock_cloexec=no])
+
+AC_MSG_CHECKING([whether SOCK_CLOEXEC is available])
+
+AC_TRY_RUN([ /* is SOCK_CLOEXEC available ? */
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <stdlib.h>
+main()
+{
+#ifdef SOCK_CLOEXEC
+ exit(0);
+#else
+ exit(1);
+#endif
+}
+], AC_MSG_RESULT(yes)
+ has_sock_cloexec=yes,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no,
+ AC_MSG_RESULT(no)
+ has_sock_cloexec=no)
+
+if test "x$with_sock_cloexec" != xno && test "x$has_sock_cloexec" = xno; then
+ AC_MSG_WARN([cannot use SOCK_CLOEXEC -- SOCK_CLOEXEC undefined on this platform])
+ with_sock_cloexec=no
+fi
+
+if test "x$with_sock_cloexec" != xno; then
+AC_MSG_NOTICE([building with SOCK_CLOEXEC])
+else
+AC_MSG_NOTICE([building without SOCK_CLOEXEC])
+fi
+
+AS_IF([test x"$with_sock_cloexec" != xno], [AC_DEFINE([SOCK_CLOEXEC_ENABLED], [1], [Define to 1, if SOCK_CLOEXEC is defined and wanted])])
+AM_CONDITIONAL([SOCK_CLOEXEC_ENABLED],[test "x$with_sock_cloexec" != xno])
+
+# Determine which libraries we need to use clock_gettime
--- End diff --
Aside from the the rest looks ok. Ready to commit once this item is addressed.
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/427
@anmolnar sorry, my editor is configured to strip the trailing white spaces . Do you need me to disable that?
---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/427
@fr0stbyte in future I believe you need to update the commit message, that doesn't seem to be updated along with the PR description I'm afraid. (something to consider for future changes, I'll commit this as-is for now, esp as it matches what went into trunk...)
---