You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by rvs <gi...@git.apache.org> on 2018/12/01 00:57:28 UTC
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
GitHub user rvs opened a pull request:
https://github.com/apache/guacamole-server/pull/206
GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rvs/guacamole-server master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/206.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 #206
----
commit 5759200c11cfc3554c120d2c16120b337f22df35
Author: Roman Shaposhnik <rv...@...>
Date: 2018-12-01T00:52:31Z
GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread stack sizes
----
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238170864
--- Diff: src/guacd/daemon.c ---
@@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;
+#ifdef PTHREAD_STACK_SIZE
+ /* Set default stack size */
+ pthread_attr_t default_pthread_attr;
+ if (pthread_attr_init(&default_pthread_attr) ||
+ pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+ pthread_setattr_default_np(&default_pthread_attr)) {
+ fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
+ exit(EXIT_FAILURE);
--- End diff --
Please use 4-space indents per level. See: http://guacamole.apache.org/guac-style/#general-style
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507820
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
--- End diff --
I'm not sure I follow this comment -- this is exactly why I'm using.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r245522648
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
--- End diff --
That would probably be better (to avoid users trying things like `--with-pthread_stack=8MB`), though this may no longer be necessary based on your latest suggestion to simplify everything. I agree that it doesn't make sense to have a low-level option to tweak the stack size if it's known that the stack size absolutely must be at least 8MB.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507594
--- Diff: src/guacd/daemon.c ---
@@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;
+#ifdef PTHREAD_STACK_SIZE
+ /* Set default stack size */
+ pthread_attr_t default_pthread_attr;
+ if (pthread_attr_init(&default_pthread_attr) ||
+ pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+ pthread_setattr_default_np(&default_pthread_attr)) {
+ fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
+ exit(EXIT_FAILURE);
--- End diff --
Got it! Will fix.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238170624
--- Diff: src/guacd/daemon.c ---
@@ -19,6 +19,12 @@
#include "config.h"
+#ifdef PTHREAD_STACK_SIZE
+#define _GNU_SOURCE 1
--- End diff --
I'm concerned over explicitly setting `_GNU_SOURCE` and using the non-portable `pthread_setattr_default_np()`. There's no guarantee that `pthread_setattr_default_np()` will be available, or that it will be available if `_GNU_SOURCE` is set.
Testing for these within configure would be an improvement, but the non-portability is still concerning. Is there no portable way to do this?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238171069
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
--- End diff --
This may be confusing as "8MB" is not a value accepted by this option.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507952
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
--- End diff --
See the comment above. It is automagically created by autoconf because of the test you quoted (testing for pthread_create in a library called pthread).
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507693
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
--- End diff --
Would you prefer it to read "8388608 (8MB)"
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238171680
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
--- End diff --
Not all platforms use libpthread for POSIX threads. Some have this built into libc. If necessary, you might be able to use `$PTHREAD_LIBS` which is set by an earlier test:
https://github.com/apache/guacamole-server/blob/bbb6afaf462f6930a589f962302806c52f350c0b/configure.ac#L61-L64
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238171724
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
--- End diff --
Where does `ac_cv_lib_pthread_pthread_create` come from?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238508851
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
+ AC_RUN_IFELSE([AC_LANG_SOURCE([[
--- End diff --
I honestly don't know if there is. I couldn't come up with one. That said -- this is basically a runtime check for a warning message. As such I can remove it (while leaving the rest of the patch). It would, however, be *very* unfortunate to do so since in my personal example it cost me a day to figure out that it was the stack size that was killing guacd on alpine. Anybody building guacd in the future on a similar platform will surely appreciate getting this type of a message early.
ON THE OTHER HAND -- if you're willing to make it a firm guarantee -- not an option that this size of a stack is alway requested -- perhaps we can simplify this patch quite a bit. Is this something that would be better discussed on the mailing list?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238168882
--- Diff: src/guacd/daemon.c ---
@@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;
+#ifdef PTHREAD_STACK_SIZE
+ /* Set default stack size */
+ pthread_attr_t default_pthread_attr;
+ if (pthread_attr_init(&default_pthread_attr) ||
+ pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+ pthread_setattr_default_np(&default_pthread_attr)) {
+ fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
--- End diff --
> `"Couldn't set requested pthread stack size of %d\n"`
At the time this failure occurs, the user hasn't requested anything - the stack size is set permanently at build time. This message should be rephrased such that the nature of the error can be understood by the user running guacd.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507999
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
+ AC_RUN_IFELSE([AC_LANG_SOURCE([[
+ #include <pthread.h>
+
+ int main() {
+ pthread_attr_t default_pthread_attr;
+ size_t stacksize;
+
+ #ifdef PTHREAD_STACK_SIZE
+ return PTHREAD_STACK_SIZE < 8*1024*1024;
+ #else
+ pthread_attr_init(&default_pthread_attr);
+ pthread_attr_getstacksize(&default_pthread_attr, &stacksize);
+ return stacksize < 8*1024*1024;
+ #endif
+ }
+
+ ]])],
+ [AC_MSG_RESULT([yes])],
+ [AC_MSG_RESULT([no])
+ AC_MSG_WARN([
+ --------------------------------------------
+ Default pthread stack size is less than 8MB
--- End diff --
Yes we do. This is the default with glibc.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238172374
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
+ AC_RUN_IFELSE([AC_LANG_SOURCE([[
+ #include <pthread.h>
+
+ int main() {
+ pthread_attr_t default_pthread_attr;
+ size_t stacksize;
+
+ #ifdef PTHREAD_STACK_SIZE
+ return PTHREAD_STACK_SIZE < 8*1024*1024;
+ #else
+ pthread_attr_init(&default_pthread_attr);
+ pthread_attr_getstacksize(&default_pthread_attr, &stacksize);
+ return stacksize < 8*1024*1024;
+ #endif
+ }
+
+ ]])],
+ [AC_MSG_RESULT([yes])],
+ [AC_MSG_RESULT([no])
+ AC_MSG_WARN([
+ --------------------------------------------
+ Default pthread stack size is less than 8MB
--- End diff --
Do we know for certain that 8MB satisfies our stack requirements?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r245522605
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
--- End diff --
What I mean is the failure of this test (whether `pthread_create()` is defined within libpthread) does not mean `pthread_create()` is not present at all, nor that the test for `pthread_setattr_default_np()` need not be performed.
This test is essentially a check for whether libpthread needs to be linked in. If it fails, `pthread_create()`, etc. are still used; POSIX threads are then simply assumed to be part of libc.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507157
--- Diff: src/guacd/daemon.c ---
@@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;
+#ifdef PTHREAD_STACK_SIZE
+ /* Set default stack size */
+ pthread_attr_t default_pthread_attr;
+ if (pthread_attr_init(&default_pthread_attr) ||
+ pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+ pthread_setattr_default_np(&default_pthread_attr)) {
+ fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
--- End diff --
Good point. Will fix both of these.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238172953
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
+ AC_RUN_IFELSE([AC_LANG_SOURCE([[
--- End diff --
[The documentation for `AC_RUN_IFELSE`](https://www.gnu.org/software/autoconf/manual/autoconf-2.68/html_node/Runtime.html) states:
> Avoid running test programs if possible, because this prevents people from configuring your package for cross-compiling.
Is there any other way to achieve this which would not hurt cross-compilation?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r245522526
--- Diff: configure.ac ---
@@ -140,6 +140,46 @@ AC_SUBST([COMMON_SSH_INCLUDE], '-I$(top_srcdir)/src/common-ssh')
AC_SUBST([TERMINAL_LTLIB], '$(top_builddir)/src/terminal/libguac_terminal.la')
AC_SUBST([TERMINAL_INCLUDE], '-I$(top_srcdir)/src/terminal $(PANGO_CFLAGS) $(PANGOCAIRO_CFLAGS) $(COMMON_INCLUDE)')
+# pthread stack size
+AC_ARG_WITH(pthread_stack,
+ [AS_HELP_STRING([--with-pthread_stack=<size in bytes>],
+ [explicitly set pthread stack size (8MB is recommended)])
+ ],pthread_stack_size=$withval
+ AC_DEFINE_UNQUOTED([PTHREAD_STACK_SIZE], [$pthread_stack_size], [pthread stack size (8MB is recommended)])
+)
+if test "x$ac_cv_lib_pthread_pthread_create" = xyes; then
+ AC_MSG_CHECKING([whether default pthread stack is larger than 8MB])
+ ac_save_libs="$LIBS"
+ LIBS="$LIBS -lpthread"
+ AC_RUN_IFELSE([AC_LANG_SOURCE([[
--- End diff --
> ON THE OTHER HAND -- if you're willing to make it a firm guarantee -- not an option that this size of a stack is alway requested -- perhaps we can simplify this patch quite a bit.
Making this a firm guarantee sounds like the best idea, particularly since you've demonstrated things simply don't work unless the stack size is at least 8MB. I'd suggest:
1. If default stack size is less than 8MB, guacd should attempt to set the default stack size to 8MB.
2. If the default stack size cannot be set (either because setting the attribute fails or `pthread_setattr_default_np()` is unavailable, a warning should be logged.
> Is this something that would be better discussed on the mailing list?
If there are other angles to consider, hopping onto the mailing list is always a good thing.
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238167956
--- Diff: src/guacd/daemon.c ---
@@ -275,6 +281,17 @@ int main(int argc, char* argv[]) {
/* General */
int retval;
+#ifdef PTHREAD_STACK_SIZE
+ /* Set default stack size */
+ pthread_attr_t default_pthread_attr;
+ if (pthread_attr_init(&default_pthread_attr) ||
+ pthread_attr_setstacksize(&default_pthread_attr, PTHREAD_STACK_SIZE) ||
+ pthread_setattr_default_np(&default_pthread_attr)) {
+ fprintf(stderr, "Couldn't set requested pthread stack size of %d\n", PTHREAD_STACK_SIZE);
--- End diff --
Why `fprintf()` early rather than `guacd_log()` after the logging system is ready?
---
[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...
Posted by rvs <gi...@git.apache.org>.
Github user rvs commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/206#discussion_r238507556
--- Diff: src/guacd/daemon.c ---
@@ -19,6 +19,12 @@
#include "config.h"
+#ifdef PTHREAD_STACK_SIZE
+#define _GNU_SOURCE 1
--- End diff --
pthread_setattr_default_np() is available for quite sometime with _GNU_SOURCE so I don't think this should be that big of a concern. To make it reliable I can add testing for pthread_setattr_default_np to the configure part of this patch. Consider this done.
As for the more portable way of doing this -- the only way I know of is to add add explicit setting of the stack size right before each call of pthread_create. Please let me know if you'd prefer this way of doing it (I can see myself wrapping it up in a single function of course).
---