-
-
Notifications
You must be signed in to change notification settings - Fork 483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix systemd socket activation support #491
base: master
Are you sure you want to change the base?
Conversation
Fix systemd socket activation with passing listening sockets. (Seems only inetd style accept=yes systemd socket activation was actually working previously) Can be used with systemd units among the lines of: x11vnc.service == [Unit] Description=VNC server Requires=x11vnc.socket [Service] Type=simple ExecStart=/usr/bin/x11vnc -no6 -xkb -repeat -auth guess -display WAIT:0 -forever -shared == x11vnc.socket == [Unit] Description=VNC server socket [Socket] ListenStream=5900 [Install] WantedBy=sockets.target == systemctl enable x11vnc.socket Signed-off-by: Floris Bos <bos@je-eigen-domein.nl>
If a second socket is passed through systemd, assume it is meant for httpd use. Using it requires systemd units among the lines of: /lib/systemd/system/x11vnc.service == [Unit] Description=VNC server Requires=x11vnc.socket [Service] Type=simple ExecStart=/usr/bin/x11vnc -httpdir /usr/share/novnc -no6 -xkb -repeat -auth guess -display WAIT:0 -forever -shared == /lib/systemd/system/x11vnc.socket == [Unit] Description=VNC server socket [Socket] ListenStream=5900 ListenStream=80 [Install] WantedBy=sockets.target == systemctl enable --now x11vnc.socket Signed-off-by: Floris Bos <bos@je-eigen-domein.nl>
&& getnameinfo((struct sockaddr *)&sa, sa_len, hoststr, sizeof(hoststr), | ||
portstr, sizeof(portstr), NI_NUMERICHOST | NI_NUMERICSERV) == 0) { | ||
rfbLog("Socket activation through systemd. Listening for VNC connections on %s:%s\n", hoststr, portstr); | ||
rfbScreen->port = atoi(portstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's no ListenStream=nnnn
it falls back to the default port? Would you be able to add some quick docs about the different systemd approaches to the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's no ListenStream=nnnn it falls back to the default port?
In that case sd_listen_fds(0) will return 0, which is tested in the "if" some lines above, and the code quoted will not be reached.
Instead it will print "Unable to establish connection with systemd socket\n" to log (not sure if that log message is desired, but that was/is the existing behavior if libvncserver is compiled with systemd support), and fallback to default port.
Would you be able to add some quick docs about the different systemd approaches to the README?
Not sure if a sample systemd file wouldn't be better in x11vnc repo.
Although I left it there because the existing code had support for it, the accept=yes approach is something you would normally NOT use, unless you have some very strange use-case where you want to have a separate VNC server program instance running for each TCP connection opened.
Not sure if you want to document such rare corner case.
One would normally only use the approach without "accept=yes" for x11vnc and the like, where a single program instance manages all client connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'm lacking the knowledge how this is supposed to work. Can you quickly sketch out for me what system activation does in the "accept=yes" mode and in this PR's approach?
My rough understanding is that systemd activation is similar to inetd...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'm lacking the knowledge how this is supposed to work. Can you quickly sketch out for me what system activation
does in the "accept=yes" mode and in this PR's approach?
This PR does not change anything regarding accept=yes mode.
Which is indeed similar to inetd in that it launches a new VNC server program every time there is a new TCP connection.
And which I would NOT recommend.
What the PR does fix is the normal systemd socket activation mode (accept=no, which is the default), which was broken.
With that systemd will start the VNC server program the first time a connection to the port is made, and passes the listening socket to the program.
The program will then behave the same way as when it was started normally without systemd, and can use that listening socket to accept further connections.
So x11vnc can have 10 clients, but there will only be one x11vnc program like normal (when running with -shared).
Main advantage of using systemd socket activation VS starting the program normally, is that the program will only be started if there is an actual connection.
And no memory will be wasted if you are not using VNC on this boot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that from the code, this is the important part that does the real work, that simply starts using the listening socket it gets passed through from systemd (and accepts the first connection, that we know is pending):
FD_ZERO(&(rfbScreen->allFds)); rfbScreen->listenSock = sock; FD_SET(rfbScreen->listenSock, &(rfbScreen->allFds)); rfbScreen->maxFd = rfbScreen->listenSock; rfbProcessNewConnection(rfbScreen);
The part at the top is not strictly necessary.
It tries to figure out what the original listening port was that was configured in systemd, and puts that in rfbScreen->port here:
struct sockaddr_storage sa; int sa_len = sizeof(sa); char hoststr[NI_MAXHOST]; char portstr[NI_MAXSERV]; if (getsockname(sock, (struct sockaddr *) &sa, &sa_len) == 0 && getnameinfo((struct sockaddr *)&sa, sa_len, hoststr, sizeof(hoststr), portstr, sizeof(portstr), NI_NUMERICHOST | NI_NUMERICSERV) == 0) { rfbLog("Socket activation through systemd. Listening for VNC connections on %s:%s\n", hoststr, portstr); rfbScreen->port = atoi(portstr);
I added that because x11vnc prints out the port number that is in rfbScreen->port to the log output.
And it looks weird if it says port 0 there.
But is not strictly necessary for the socket code to function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question wrt the code
I recall this is almost good, someone just needs to document how it's supposed to work. And test this case. |
Closes #314