[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...

2019-01-06 Thread mike-jumper
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=],
+[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...

2019-01-06 Thread mike-jumper
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=],
+[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...

2019-01-06 Thread mike-jumper
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=],
+[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...

2018-12-03 Thread rvs
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=],
+[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...

2018-12-03 Thread rvs
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=],
+[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...

2018-12-03 Thread rvs
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=],
+[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 
+
+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(_pthread_attr);
+pthread_attr_getstacksize(_pthread_attr, 
);
+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...

2018-12-03 Thread rvs
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=],
+[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...

2018-12-03 Thread rvs
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=],
+[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...

2018-12-03 Thread rvs
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(_pthread_attr) ||
+pthread_attr_setstacksize(_pthread_attr, 
PTHREAD_STACK_SIZE) ||
+pthread_setattr_default_np(_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...

2018-12-03 Thread rvs
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).


---


[GitHub] guacamole-server pull request #206: GUACAMOLE-663: guacd SEGVs intermittentl...

2018-12-03 Thread rvs
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(_pthread_attr) ||
+pthread_attr_setstacksize(_pthread_attr, 
PTHREAD_STACK_SIZE) ||
+pthread_setattr_default_np(_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...

2018-12-03 Thread mike-jumper
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(_pthread_attr) ||
+pthread_attr_setstacksize(_pthread_attr, 
PTHREAD_STACK_SIZE) ||
+pthread_setattr_default_np(_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...

2018-12-03 Thread mike-jumper
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...

2018-12-03 Thread mike-jumper
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=],
+[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...

2018-12-03 Thread mike-jumper
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=],
+[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...

2018-12-03 Thread mike-jumper
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(_pthread_attr) ||
+pthread_attr_setstacksize(_pthread_attr, 
PTHREAD_STACK_SIZE) ||
+pthread_setattr_default_np(_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...

2018-12-03 Thread mike-jumper
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=],
+[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...

2018-12-03 Thread mike-jumper
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(_pthread_attr) ||
+pthread_attr_setstacksize(_pthread_attr, 
PTHREAD_STACK_SIZE) ||
+pthread_setattr_default_np(_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...

2018-12-03 Thread mike-jumper
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=],
+[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 
+
+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(_pthread_attr);
+pthread_attr_getstacksize(_pthread_attr, 
);
+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...

2018-12-03 Thread mike-jumper
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=],
+[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...

2018-11-30 Thread rvs
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 
Date:   2018-12-01T00:52:31Z

GUACAMOLE-663: guacd SEGVs intermittently on systems with small(er) thread 
stack sizes




---