guacamole-server
guacamole-server copied to clipboard
apache
GUACAMOLE-168: Add experimental X.Org driver implementing the Guacamole protocol.
Here it is - the X.Org driver. There's some documentation to be done with some of the older code, so I've opened this up as a draft and I'll get on that. In the meantime, definitely feel free to peek. It's a hefty bit of code.
@mike-jumper I ran into an issue compiling this on rhel7, I danced around it by using a fix similar to this one:
https://github.com/varnish/libvmod-example/pull/18/commits/ad96d3bc5ebe126c8cb6f03591c6fc62ffd6f3d6
The error was this:
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, `build-aux'.
libtoolize: copying file `build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
configure.ac:1166: error: possibly undefined macro: PKG_CHECK_VAR
If this token and others are legitimate, please use m4_pattern_allow.
See the Autoconf documentation.
autoreconf: /usr/bin/autoconf failed with exit status: 1
@mike-jumper I ran into an issue compiling this on rhel7, ...
What specific issue?
... I danced around it by using a fix similar to this one:
What specific fix?
Ah, OK.
... configure.ac:1166: error: possibly undefined macro: PKG_CHECK_VAR If this token and others are legitimate, please use m4_pattern_allow. See the Autoconf documentation. ...
Doesn't this just mean that pkg-config needs to be installed for the PKG_CHECK_VAR macro to be defined?
I ran into similar issues while compiling guacamole-server. Usually I think this type of issue means you don't have the right dependencies or the right version of dependencies installed. Here is the Dockerfile I've been using for building guacamole-server on centos7, but note that I'm not using this in a production environment but only setting up this docker image to be used by another image for testing. It contains some things that might not be necessary or not best practice (it also does not use pulse-audio).
Note that I haven't tried compiling this specific pull request but I'm assuming it's a (mostly) clean port of mike-jumper/guacamole-server's xf86-video-guac branch, as that branch primarily only added new code into src/drivers and only made very minor modifications to the build process/setup. I'm willing to try testing out this specific pull request however I'm not yet wholly comfortable with GitHub/Git (lots of experience with mercurial) so I would need some help figuring out how to download this change along with applying a small changeset on top of it for my test environment. The test environment I'm working with I rebased xf86-video-guac on top of guacamole 1.2.0 tagged revision and applied a small change on top of.
My apologies, I have pkgconfig-0.27.1-4.el7 installed but I think I need at least version 0.28 in order to get it to work. I don't currently have access to that in my repos. Thanks for the info! @neandrake
@mike-jumper:
Doesn't this just mean that pkg-config needs to be installed for the PKG_CHECK_VAR macro to be defined?
As @knacktim mentioned, no, this isn't just the presence of pkg-config, but apparently this was introduced in version 0.28, which isn't available in EL7. See similar issues:
https://wiki.strongswan.org/issues/3406 https://www.redhat.com/archives/libguestfs/2018-July/msg00080.html
So, only really required during autoreconf operations. But, as EL7 is still relatively popular, might be worth finding a way to work around this.
I remember running into this issue a while back and I think the use of scl and devtoolset-8 was one way I had resolved this.
$ yum install -y centos-release-scl
$ yum install -y devtoolset-8
$ scl enable devtoolset-8 'autoreconf --install --force'
@mike-jumper:
Doesn't this just mean that pkg-config needs to be installed for the PKG_CHECK_VAR macro to be defined?
As @knacktim mentioned, no, this isn't just the presence of pkg-config, but apparently this was introduced in version 0.28, which isn't available in EL7. See similar issues:
https://wiki.strongswan.org/issues/3406 https://www.redhat.com/archives/libguestfs/2018-July/msg00080.html
Ah, OK.
So, only really required during autoreconf operations. But, as EL7 is still relatively popular, might be worth finding a way to work around this.
Yes, definitely. Perhaps we should instead use the same approach as we use for the FreeRDP module directory:
https://github.com/apache/guacamole-server/blob/c769d18cf692642b788e8e9d8494f846bc2f0767/configure.ac#L658-L667
Yep, that would work. I also stumbled across this solution from someone in a completely different project:
https://git.sagemath.org/sage.git/commit?id=16a0a75c66a2e05187e174f3336edd525c65bb41
They wrote a M4 macro to do what PKG_CHECK_VAR does...
From referencing the link to FreeRDP's use of PKG_CHECK_MODULES I toyed with updating this to not use PKG_CHECK_VAR and I think this works appropriately. I used pkg-config --variable=moduledir xorg-server to confirm it printed out the right directory on my system, as well as printed out the same when configuring. Additionally I verified that it places the guac driver files in the xorg/modules folder.
GUACAMOLE-168-pkgconfig.diff.txt (github doesn't allow attaching diff files x.x)
I have several modifications to this branch which I would like to submit as pull requests, however since this branch is itself a pull request I'm not sure what the best approach is. The changes I have are
GUACAMOLE-168: Update configure.ac ...
GUACAMOLE-168: Don't require virtual console ...
GUACAMOLE-168: Add support for "force-lossless" ...
GUACAMOLE-168: Include SSL support for the x-server driver ...
What would be the best way to submit these changes against this PR?
Notably the update for "force-lossless" requires merging this branch into 1.3.0 which adds a little more complexity with how to submit these additional changes.
@neandrake, I can just pull them into this PR. Where's your fork and branch?
I just created a fork and pushed the branch to https://github.com/neandrake/guacamole-server/tree/xorg
As for verification, I've confirmed that with all changes
- I am able to build guacd on CentOS7 with most standard dependencies installed.
- I verified running X.org in a container with the guacd driver functions properly without having to grant the container access to the virtual console.
- I can specify "force-lossless" and from visual inspection it does appear to transmit lossless images. I've been meaning to add logging to confirm that the server is obeying this argument but I have not yet done so.
- I can connect to guacd server when configured both with and without SSL. Turning on SSL does seem to introduce some latency though I haven't done any thorough comparative testing.
@mike-jumper Which window manager do you use when testing out the driver? I'm seeing some odd behaviors with child windows which I originally thought might have been a bug in the window manager I was using. I've tested with two different window managers and see the same behaviors though.
Child windows have their content rendered offset by some arbitrary x/y amount but the mouse activity appears to be correctly interpreted: hovering where a button should render causes the offset rendering to highlight, clicking where it should be also registers that the button was clicked. The window managers I've tested this out on and seen the same behavior are with jwm and xfwm4.
When I don't use a window manager at all the offset does not seem to occur which is why I first thought this was an issue with jwm. However after seeing this same issue with xfwm4 I'm thinking there might be a bug with child window resizing or positioning code in the driver. I've poked around with some of the surface clipping code but I'm still learning the code for this.
I would like to second @landoncolburn : the draft is now 3 years old, and has fallen behind master by over 200 commits, yet there does not seem to be any bugs being uncovered in this branch. What is preventing it from being merged in?
I really would like to see this in play as soon as possible, it seems like a no-brainer faster alternative to RDP. ;-)
@mike-jumper I pulled over this branch anew after your rebase and have added several more changes https://github.com/neandrake/guacamole-server/tree/guacamole-168
* 99466730 GUACAMOLE-168: Extending cursor rendering
* c6c4d702 GUACAMOLE-168: Fix image copying to invalid bounds
* 46cc9550 GUACAMOLE-168: Update SSL/TLS support for OpenSSL 1.1.0+
* 864b8a9c GUACAMOLE-168: Add conversion for some unicode keysyms to legacy
* 6ff5952f GUACAMOLE-168: Corrupted protocol synchronization fix
* f35e2ebd GUACAMOLE-168: Add keep-alive
* 7832e44a GUACAMOLE-168: Initialization fixes
* f7dacbb8 GUACAMOLE-168: Add recording
* 5baaad1c GUACAMOLE-168: Complete the clipboard implementation
@neandrake Can you clarify the need behind 6ff5952f03e78b381463d0649e3392a8d4860feb? The commit message is clear, but the additional mutex appears to only guard functions that are already guarded by the mutex built into guac_common_display. Usage of guac_socket should also already be threadsafe.
@mike-jumper That was one I wasn't fully sure about and wanted to ask about though the change did appear to resolve the issue. I was running into issues where new users joining, and sometimes leaving I believe, would cause a corruption in the protocol stream. The symptom was getting an exception on the web server,
GuacamoleServerException: Non-numeric character in element length.
The most common way to reproduce was to have one user connect to use the workstation and have another user connect to observe. Then have the second user refresh the page to establish new tunnel connections and ~1 in 5 would cause the issue.
After observing the exception and then logging the protocol stream on the web server there seemed to be a pattern of a corruption in the protocol, where an instruction after a SYNC would be truncated early - e.g. a BLOB with size 100 would only actually have 50 bytes.
Through investigating when the x driver sends SYNC commands it appeared as though places where guac_socket_flush() were the source of the corruption. My theory was that while the socket has a lock/mutex when sending data if there is a need to flush the data then that would need to be locked as part of the initial sending of the data.
This additional lock did appear to resolve the corruption issues.
@mike-jumper in taking another look I'm also unsure why the stream would get corrupted. The only thing that stuck out is guac_drv_display_sync_user() in user.c should maybe be using guac_client_end_frame() to make sure the timestamp is updated, rather than using guac_protocol_send_sync() -- that function doesn't appear to be used directly in any other protocol, only in the client's end_frame() function and in some tests. Though I don't see how the timestamp would cause a corruption in the protocol.
I can modify the commit to have a more appropriate change though I'm not sure what that would be or why, but the additional lock around the writing of instructions followed by uses of guac_socket_flush() in the driver appear to resolve the issue.
@mike-jumper I'm still investigating the issue but have removed the commit in question from my branch https://github.com/neandrake/guacamole-server/tree/guacamole-168
* d77f7fae GUACAMOLE-168: Extending cursor rendering
* e854825e GUACAMOLE-168: Fix image copying to invalid bounds
* 6b9d2810 GUACAMOLE-168: Update SSL/TLS support for OpenSSL 1.1.0+
* 494c1590 GUACAMOLE-168: Add conversion for some unicode keysyms to legacy
* f35e2ebd GUACAMOLE-168: Add keep-alive
* 7832e44a GUACAMOLE-168: Initialization fixes
* f7dacbb8 GUACAMOLE-168: Add recording
* 5baaad1c GUACAMOLE-168: Complete the clipboard implementation
@neandrake : is this still being reviewed? When can we hope to see Xorg support folded in? Our sysadmins are rebuilding our server, and I would really like to have this functionality in it, but they will only do main branch. We have not been able to connect with a Ubuntu/Debian hosts (in an AD-based authentication environment) no matter what we tried, except to terminal console (but these leak characters, and having to constantly crtl-l in vi gets annoying very quickly), and I would really like this working by September. Any hope of that?
@esternin I am not a core team member. For this I've submitted a few updates/fixes on top of the existing work. Your question/comment should be directed to the core team/company. They might prefer this discussion on the mailing list rather than here.
@neandrake Can you clarify the need behind 6ff5952? The commit message is clear, but the additional mutex appears to only guard functions that are already guarded by the mutex built into
guac_common_display. Usage ofguac_socketshould also already be threadsafe.
I've removed those changes from my branch I linked earlier. The issue I was seeing had a root cause of using TLS sockets with multiple active connections. I reported as GUACAMOLE-1910 and submitted a PR in https://github.com/apache/guacamole-server/pull/482.