Introduces an optional dependency on libssh
Details
Connect to VNC servers both via SSH tunnel and via regular connection
Diff Detail
- Repository
- R436 KRDC
- Branch
- ssh_tunnel (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 205 Build 205: arc lint + arc unit
Thanks, this looks like a nice feature addition to KRDC! I have not tested it - but code looks good.
core/remoteview.cpp | ||
---|---|---|
211 | (unnecessary empty line) | |
vnc/vncclientthread.cpp | ||
547 | Have you tested if this change keeps backward compatibility (I have seen that you have moved this part to a new location)? For example when starting KRDC from cli with an url without port. | |
vnc/vncsshtunnelthread.cpp | ||
120 | (whitespace after 'if' is not consistent with other parts of this file) | |
vnc/vncview.cpp | ||
217 | What happens if you have multiple sessions with tunnels open? |
core/remoteview.cpp | ||
---|---|---|
211 | will fix on commit/if i upload a new revision, just don't feel there's a need to upload a new change because just of this. | |
vnc/vncclientthread.cpp | ||
547 | I have not tested it, but the only place m_port in VncClientThread is set is in VncView::start and the only place m_port is set for VncView is on its constructor, so i did the move with relatively high confidence that it would not cause a regression. | |
vnc/vncsshtunnelthread.cpp | ||
120 | will fix on commit/if i upload a new revision, just don't feel there's a need to upload a new change because just of this. | |
vnc/vncview.cpp | ||
217 | That's fine the setsockopt(m_server_sock , SOL_SOCKET, SO_REUSEADDR, &sockopt, sizeof(sockopt)); call makes it work (or so did on my tests) |
vnc/vnchostpreferences.cpp | ||
---|---|---|
30 | static const char quality_config_key[] = (I just rechecked with a small test program, and yes, this is still valid, a const char* goes into .data while a const char[] goes into .rodata) | |
64 | the connect could move under #ifdef LIBSSH_FOUND | |
vnc/vncsshtunnelthread.cpp | ||
56 | I would add a comment here, for the next reader. // This is called by the main thread, but from a slot connected to our signal via BlockingQueuedConnection, so this is safe even without a mutex, the semaphore in BlockingQueuedConnection takes care of the synchronization. | |
129 | (missing space after if, here as well) | |
132 | You could use a local RAII class to avoid duplicating close+ssh_disconnect+ssh_free in every return code path (initialize m_server_sock to -1 in the constructor so you can test for it before calling close). As a benefit this huge method would be a little bit less huge ;) | |
147 | "any error here will propagate" confused me, I was wondering how those two threads would communicate then. | |
188 | missing space after while Should this also say while (!error && ...)? I'm not sure, but it looks like the current code might go into an infinite loop if FD_ISSET always return false for some reason. | |
232 | Instead of the if+continue above, this line could say if (!error && !channel_read_buffer), to match the next if() which also does if (!error && ...). It just seems asymetric to use continue in one case and if in the other. | |
268 | On error, this delete[] might never happen. Maybe do it at end of block too? | |
284 | This is for sure a race on m_server_sock. I guess this is here in order to wake up ssh_select in the thread? In that case, m_server_sock needs to be mutex-protected, everywhere it's used. But actually, ssh_select has a timeout of 200ms so m_stop_thread is checked every 200ms, no? It seems to me that either the timeout stays short and stop() can just wait 200ms, or the timeout becomes much longer and we do need to shutdown here. Am I missing something? |
vnc/vncsshtunnelthread.cpp | ||
---|---|---|
188 | spacing fixed, about adding the error to the while, i really don't know, i *think* if something fails here, the actual vnc part of the code will fail too since it won't get reads/writes and everything will end up failing nicely at some point. But it's one of this things that are really hard to test since i don't really know how to make it fail properly, so i'd like to leave it as is. | |
268 | Added outside the while | |
284 | Actually i don't think i need the shutdown call anymore, i think it's back from when i had a bit of a different code structure and the thread was getting blocked in accept() if i could not connect and i needed to nudge it out to be able to stop the thread, but i reworked that a while ago and i don't really need it anymore (or at least could not reproduce it getting stuck and previously was very easy) So i've just removed the shutdown and then the race is gone :) |