[GitHub] guacamole-server pull request #207: GUACAMOLE-673: Create automatically need...

2018-12-03 Thread dejan032
GitHub user dejan032 opened a pull request:

https://github.com/apache/guacamole-server/pull/207

GUACAMOLE-673: Create automatically needed directory for file transfer.

Automatically create transfer folder in guacd file system when user is 
logged in to rdp, so that the transfer network drive can be used immediately 
without needing to open it first one time to start copying things into and out 
of it.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/dejan032/guacamole-server master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/guacamole-server/pull/207.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 #207


commit 48974cf79fa629dc63d35665d57cfdc79dd8fda4
Author: Dejan Milovanovic 
Date:   2018-12-04T07:31:45Z

GUACAMOLE-673: Create automatically needed directory for file transfer.




---


[GitHub] guacamole-server pull request #204: GUACAMOLE-354: Add Swiss-German keymap f...

2018-12-03 Thread andrin-jost
Github user andrin-jost commented on a diff in the pull request:

https://github.com/apache/guacamole-server/pull/204#discussion_r238549377
  
--- Diff: src/protocols/rdp/keymaps/de_ch_qwertz.keymap ---
@@ -0,0 +1,58 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+parent  "base"
+name"ch-de-qwertz"
--- End diff --

Oh, yes you're right


---


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


---


Re: Help with CAS Module Regression

2018-12-03 Thread Nick Couchman
On Mon, Dec 3, 2018 at 10:51 AM Nick Couchman  wrote:

> On Mon, Dec 3, 2018 at 10:49 AM Mike Jumper  wrote:
>
>> Did this error occur in 0.9.14?
>>
>> If this is breaking things, and did not occur when CAS support was
>> initially released, I'd suggest a git bisect to narrow down when it
>> started.
>>
>>
> I believe everything worked fine in 0.9.14, and that something in the
> changes between there and staging/1.0.0 has broken things.  I'll work on a
> git bisect of the changes and see if I can track it down.
>
>
Looks like the change to AngularJS 1.6.9 broke something:


Re: Help with CAS Module Regression

2018-12-03 Thread Nick Couchman
On Mon, Dec 3, 2018 at 3:57 PM Nick Couchman  wrote:

> On Mon, Dec 3, 2018 at 10:51 AM Nick Couchman  wrote:
>
>> On Mon, Dec 3, 2018 at 10:49 AM Mike Jumper  wrote:
>>
>>> Did this error occur in 0.9.14?
>>>
>>> If this is breaking things, and did not occur when CAS support was
>>> initially released, I'd suggest a git bisect to narrow down when it
>>> started.
>>>
>>>
>> I believe everything worked fine in 0.9.14, and that something in the
>> changes between there and staging/1.0.0 has broken things.  I'll work on a
>> git bisect of the changes and see if I can track it down.
>>
>>
> Looks like the change to AngularJS 1.6.9 broke something:
>

b3eeb36b8726211597bdc921a632dbbc5c87ee16 is the first bad commit
commit b3eeb36b8726211597bdc921a632dbbc5c87ee16
Author: James Muehlner 
Date:   Wed Apr 25 22:25:02 2018 -0700

GUACAMOLE-526: Update webapp to angular 1.6.9.

:04 04 461bb123891a0c463bd1fbf8c84fbd3070633d09
11d67328a11952849a126e88fb68e6895d5f92b0 M guacamole


Re: Help with CAS Module Regression

2018-12-03 Thread Nick Couchman
On Mon, Dec 3, 2018 at 10:49 AM Mike Jumper  wrote:

> Did this error occur in 0.9.14?
>
> If this is breaking things, and did not occur when CAS support was
> initially released, I'd suggest a git bisect to narrow down when it
> started.
>
>
I believe everything worked fine in 0.9.14, and that something in the
changes between there and staging/1.0.0 has broken things.  I'll work on a
git bisect of the changes and see if I can track it down.

-Nick


Re: Help with CAS Module Regression

2018-12-03 Thread Mike Jumper
On Sun, Dec 2, 2018 at 1:48 PM Nick Couchman  wrote:
>
> Okay, banging my head against the wall here trying to figure out what's
> going on with the CAS module.  At this point the redirect doesn't work at
> all - the CAS module just sits on the screen with the non-translated
> message:
>
> {{ 'LOGIN.INFO_CAS_REDIRECT_PENDING' | translate }}
>
> and never redirects.  In the JS console I get the following:
>
> Failed to retrieve field with type "GUAC_CAS_TICKET"
>

That specific message will occur if a custom field type cannot be used:

https://github.com/apache/guacamole-client/blob/fc457c080d813044e30e1f4e8fe855d6a5900259/guacamole/src/main/webapp/app/form/directives/formField.js#L152

That's just a generic "catch", though. If an exception occurs
somewhere within the promise handling (due to something now being
broken in our handling of custom fields?), you could see the same
result, despite the field type being correctly registered. If this is
the case, other things that use custom fields like Duo or TOTP may
also not work as expected. Worth rechecking.

> Any ideas what might be going on here?  I've looked at the various parts of
> the CAS module, particularly the AngularJS stuff, and compared to OpenID to
> see what differences might be there, and can't see anything obvious.  I'm
> sure it's something simple/stupid, but I need another set of eyes on it.
> Also, has the OpenID module been tested recently to make sure it works?  I
> don't think I have an OpenID provider I can test against, else I would do
> it myself...

I have a test deployment which uses OpenID against Google - I'll recheck.

>
> Let me know if I can provide any additional information.  I don't see
> anything obviously wrong in the catalina.out file, so figure it's something
> AngularJS-based that's going on.
>

Did this error occur in 0.9.14?

If this is breaking things, and did not occur when CAS support was
initially released, I'd suggest a git bisect to narrow down when it
started.

- Mike


[GitHub] guacamole-server pull request #204: GUACAMOLE-354: Add Swiss-German keymap f...

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/204#discussion_r238316700
  
--- Diff: src/protocols/rdp/keymaps/de_ch_qwertz.keymap ---
@@ -0,0 +1,58 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+parent  "base"
+name"ch-de-qwertz"
--- End diff --

The keyboard name should be LANG-COUNTRY-VARIANT, same as the filename 
except with dashes instead of underscores. Is "de-ch-qwertz" correct here or 
"ch-de-qwertz"?


---


[GitHub] guacamole-client pull request #341: GUACAMOLE-670: Add slf4j logging to CAS ...

2018-12-03 Thread asfgit
Github user asfgit closed the pull request at:

https://github.com/apache/guacamole-client/pull/341


---


[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?


---