On Thu, 2018-02-01 at 16:40 +0000, Frediano Ziglio wrote:
> This allows to manage properly multiple servers (currently only Xorg).
> The executable will run as a service forking the proper agent.
> The agent will then manage the active server.
> The server receive just minimal information for the various
> graphic terminals and use to fork the agent.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> Makefile.am | 8 +
> configure.ac | 4 +
> data/90-spice-guest-streaming.rules | 3 +-
> data/spice-streaming-agent.service.in | 11 +
> data/spice-streaming-agent.socket | 10 +
> spice-streaming-agent.spec.in | 5 +-
> src/Makefile.am | 2 +
> src/daemon.cpp | 542 ++++++++++++++++++++++++++++++++++
> src/daemon.h | 19 ++
> src/spice-streaming-agent.cpp | 17 +-
> 10 files changed, 616 insertions(+), 5 deletions(-)
> create mode 100644 data/spice-streaming-agent.service.in
> create mode 100644 data/spice-streaming-agent.socket
> create mode 100644 src/daemon.cpp
> create mode 100644 src/daemon.h
>
> diff --git a/Makefile.am b/Makefile.am
> index aaf1638..11ef9a1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -18,9 +18,17 @@ pkgconfig_DATA = spice-streaming-agent.pc
> udevrulesdir = /usr/lib/udev/rules.d
> udevrules_DATA = $(srcdir)/data/90-spice-guest-streaming.rules
>
> +systemdunitdir = $(SYSTEMDSYSTEMUNITDIR)
> +systemdunit_DATA = \
> + $(srcdir)/data/spice-streaming-agent.service \
> + $(srcdir)/data/spice-streaming-agent.socket \
> + $(NULL)
> +
What is this $(NULL) thing... I've already noticed it earlier all over
the Makefile.am files, I see no meaning in it...
> EXTRA_DIST = \
> spice-streaming-agent.spec \
> spice-streaming-agent.pc \
> data/90-spice-guest-streaming.rules \
> data/spice-streaming.desktop \
> + data/spice-streaming-agent.socket \
> + data/spice-streaming-agent.service \
> $(NULL)
> diff --git a/configure.ac b/configure.ac
> index 8795dae..fe0afd9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -49,6 +49,9 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> AC_MSG_ERROR([libjpeg not found]))
> AC_SUBST(JPEG_LIBS)
>
> +SYSTEMDSYSTEMUNITDIR=`${PKG_CONFIG} systemd --variable=systemdsystemunitdir`
> +AC_SUBST(SYSTEMDSYSTEMUNITDIR)
> +
> dnl ===========================================================================
> dnl check compiler flags
>
> @@ -83,6 +86,7 @@ AC_DEFINE_DIR([BINDIR], [bindir], [Where data are placed to.])
> AC_OUTPUT([
> spice-streaming-agent.spec
> data/spice-streaming.desktop
> +data/spice-streaming-agent.service
> Makefile
> src/Makefile
> src/unittests/Makefile
> diff --git a/data/90-spice-guest-streaming.rules b/data/90-spice-guest-streaming.rules
> index 6cedd5c..7980ab9 100644
> --- a/data/90-spice-guest-streaming.rules
> +++ b/data/90-spice-guest-streaming.rules
> @@ -1,2 +1 @@
> -ACTION=="add", SUBSYSTEM=="virtio-ports", ENV{DEVLINKS}=="/dev/virtio-ports/com.redhat.stream.0", MODE="0666"
> -
> +ACTION=="add", SUBSYSTEM=="virtio-ports", ENV{DEVLINKS}=="/dev/virtio-ports/com.redhat.stream.0", ENV{SYSTEMD_WANTS}="spice-streaming-agent.socket"
> diff --git a/data/spice-streaming-agent.service.in b/data/spice-streaming-agent.service.in
> new file mode 100644
> index 0000000..df47230
> --- /dev/null
> +++ b/data/spice-streaming-agent.service.in
> @@ -0,0 +1,11 @@
> +[Unit]
> +Description=Agent daemon for SPICE streaming agent
> +Requires=spice-streaming-agent.socket
> +
> +[Service]
> +Type=simple
> +EnvironmentFile=-/etc/sysconfig/spice-streaming-agent
> +ExecStart=@BINDIR@/spice-streaming-agent $SPICE_STREAMING_EXTRA_ARGS
> +
> +[Install]
> +Also=spice-streaming-agent.socket
> diff --git a/data/spice-streaming-agent.socket b/data/spice-streaming-agent.socket
> new file mode 100644
> index 0000000..c59d0bd
> --- /dev/null
> +++ b/data/spice-streaming-agent.socket
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=Activation socket for SPICE streaming agent daemon
> +# only start the socket if the virtio port device exists
> +Requisite=dev-virtio\x2dports-com.redhat.stream.0.device
> +
> +[Socket]
> +ListenStream=@spice-streaming-agent-daemon
> +
> +[Install]
> +WantedBy=sockets.target
> diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
> index 57872d1..eef8f90 100644
> --- a/spice-streaming-agent.spec.in
> +++ b/spice-streaming-agent.spec.in
> @@ -9,6 +9,7 @@ Source0: %{name}-%{version}.tar.xz
> BuildRequires: spice-protocol >= @SPICE_PROTOCOL_MIN_VER@
> BuildRequires: libX11-devel libXfixes-devel
> BuildRequires: libjpeg-turbo-devel
> +BuildRequires: systemd-devel
> # we need /usr/sbin/semanage program which is available on different
> # packages depending on distribution
> Requires(post): /usr/sbin/semanage
> @@ -51,10 +52,12 @@ fi
>
> %files
> %doc COPYING ChangeLog README
> -/lib/udev/rules.d/90-spice-guest-streaming.rules
> +%{_udevrulesdir}/90-spice-guest-streaming.rules
> %{_bindir}/spice-streaming-agent
> %{_sysconfdir}/xdg/autostart/spice-streaming.desktop
> %{_datadir}/gdm/greeter/autostart/spice-streaming.desktop
> +/usr/lib/systemd/system/spice-streaming-agent.socket
> +/usr/lib/systemd/system/spice-streaming-agent.service
>
> %files devel
> %defattr(-,root,root,-)
> diff --git a/src/Makefile.am b/src/Makefile.am
> index bbfaa35..d2f8a47 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -58,4 +58,6 @@ spice_streaming_agent_SOURCES = \
> mjpeg-fallback.cpp \
> jpeg.cpp \
> jpeg.hpp \
> + daemon.cpp \
> + daemon.h \
> $(NULL)
> diff --git a/src/daemon.cpp b/src/daemon.cpp
> new file mode 100644
> index 0000000..87201a1
> --- /dev/null
> +++ b/src/daemon.cpp
> @@ -0,0 +1,542 @@
> +/* An implementation of a SPICE streaming agent
> + *
> + * \copyright
> + * Copyright 2016-2017 Red Hat Inc. All rights reserved.
> + */
> +
> +/* This file implement the daemon part required to
> + * avoid permission issues.
> + * The daemon will listen to a socket for clients.
> + * Clients simply will send informations about new display.
> + * Daemon will monitor current terminal and launch a proper
> + * agent with information passed.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <getopt.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <syslog.h>
> +#include <poll.h>
> +#include <err.h>
> +#include <errno.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <fcntl.h>
> +#include <pwd.h>
> +#include <grp.h>
> +#include <systemd/sd-daemon.h>
> +
> +#include "daemon.h"
> +
> +/*
> + * There are 3 "rules" for the agent:
did you mean: "roles"? "rules" certainly doesn't make sense here.
> + * - main agent;
> + * - daemon
> + * - helper
This nomenclature is really blurry here. Especially between 'agent' and
'daemon'. The streaming agent is already supposed to run as a daemon.
You also say 'main agent' here, but later on actually call the main
process 'daemon' and the subprocess 'agent'. Totally confusing.
I suggest calling the main process 'streaming agent' and the
subprocesses 'streaming worker'. Not my best name to date but sounds
servicable :D I'll keep thinking about it :)
I still haven't made up my mind on helpers.
> + * The rule of the agent is to handle a given graphical session
> + * capturing and sending video stream to SPICE server.
> + * The rule of the daemon is to listen to information from helpers
> + * collecting Unix session information from the helpers and from
> + * system and managing agents.
> + * The helper just send session information to the daemon.
I think this could be described more in detail, as in:
- What session information (don't make a list of what exactly, but
describe what is the nature of the information, i.e. 'session
information for the X server connection')
- How does it send the information.
- How is the information further used.
> + * The agent are usually child (forked) of the daemon.
'usually'? Describe when, I am still confused about this...
> + * This schema is used for different reasons:
> + * - the daemon can be run as root having access to the streaming
> + * device file;
> + * - the daemon can control the live of the agent making possible to
'life', 'making it possible'
> + * switch between sessions;
> + * - running agents directly launched from a graphical session cause
> + * some issue with SELinux, launching outside a Unix session allows
'causes some issues' :)
> + * the process to have less restrictions.
> + */
> +
> +/*
> + * The protocol between helper and daemon is pretty simple.
> + * Helper connects to daemon and send all information needed to connect
> + * to the graphical interface.
> + * The helper send a single message which is pretty small (should be at
> + * most 1K) through a Unix socket.
> + * This:
> + * - allows to pass credentials;
> + * - a single small message prevent the helper to consume memory on the
> + * daemon side;
> + * - allows dynamic activation using SystemD;
> + * - writing the client is really easy and can be written in a script
> + * language.
> + *
> + * Message is:
> + * - 1 byte. Version. Has to be 1;
> + * - a set of strings, each:
> + * - 1 byte type field, currently:
> + * 1) DISPLAY environment;
> + * 2) XAUTHORITY environment;
> + * - 1 byte for length
> + * - data
> + * - DISPLAY content. The DISPLAY should be local like ":1";
> + * - XAUTHORITY environment content (a filename).
I am still not entirely sure about this in general, but I would really
like if we started using some extablished serialization protocol for
these arbitrary communication channels. I know, this is just a really
simple thing. It may not always be so. Seems to me we have quite a
number of communication channels, with a lot of code around them. Of
course it is mainly the SPICE protocol, which would be a huge task to
replace, if it would ever happen. One can dream, though :)
It's more of a food for thought, this little thing probably doesn't
warrant it. But we could have more places for it, I'm still not that
familiar with SPICE.
As for concrete examples of what we could use, Cap'n Proto [1] came to
my mind. There are others, some research would have to be done. These
days nobody really implements their own serialization protocol, right?
:D
[1] https://capnproto.org/
> + *
> + * Note: Linux allows to read the peer credentials (user/group) and
> + * PID which we need. If we would need to extent to other systems
> + * (like *BSD/Mac OS X) these allows to pass these informations using
> + * an ancilliary message and SCM_CREDS (Linux also has a similar
> + * SCM_CREDENTIALS).
> + */
> +
> +const unsigned max_clients = 63;
> +enum {
> + LISTEN_FD,
> + VT_FD,
> + FIXED_FDS
> +};
> +static struct pollfd fds[max_clients+FIXED_FDS];
> +static int listen_fd = -1;
> +static struct pollfd * const client_fds = fds+FIXED_FDS;
> +static unsigned num_clients = 0;
> +static volatile pid_t child_pid = -1;
Okay... Why do you put C code into a .cpp file? :)
As I said, the static variables are a bad practice... Why not create a
class called Daemon (or, taking into account my suggestions for naming,
StreamingWorker) and put them into the class. The functions below can
become the methods of the class and you have a nice context for your
data.
> +#if CLIENT_DATA
> +struct ClientData
> +{
> + char display[256];
> + char xauthority[256];
> +};
std::string
> +static ClientData client_data[max_clients];
std::vector (and then you don't need max_clients)
> +#endif
> +
> +struct TerminalInfo
> +{
> + bool valid;
> +
> + char display[256];
> + char xauthority[256];
std::string
> + uid_t uid;
> +};
> +static TerminalInfo terminal_info[64];
> +
> +static bool fd_is_agent(int fd)
> +{
> + // must be a socket
> + struct stat st;
> + if (fstat(fd, &st) != 0)
> + return false;
So.. what about the rule of 'always brackets for ifs'? :)
> + if ((st.st_mode & S_IFMT) != S_IFSOCK)
> + return false;
> +
> + // must have our address
> + struct sockaddr_un address;
> + socklen_t len = sizeof(address);
> + if (getsockname(fd, (sockaddr *) &address, &len) != 0)
> + return false;
> + if (address.sun_family != AF_UNIX)
> + return false;
> + if (address.sun_path[0] != 0)
> + return false;
> + address.sun_path[0] = '@';
> + if (len != SUN_LEN(&address) || strcmp(address.sun_path, "@spice-streaming-agent-daemon") != 0)
You could put the constant into a std::string (it's used again few
lines below) and use ==, just sayin :)
> + return false;
> + // TODO must be in listening
> +
> + return true;
> +}
> +static void init()
> +{
> + int ret;
> +
> + int fd;
> + if (fd_is_agent(0)) {
> + fd = 0;
> + } else if (fd_is_agent(SD_LISTEN_FDS_START)) {
> + fd = SD_LISTEN_FDS_START;
> + } else {
> + // open socket
> + fd = socket(PF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0);
> + if (fd < 0)
> + err(1, "socket");
> +
> + struct sockaddr_un address;
> + memset(&address, 0, sizeof(address));
> + address.sun_family = AF_UNIX;
> + strcpy(address.sun_path, "@spice-streaming-agent-daemon");
> + int len = SUN_LEN(&address);
> + address.sun_path[0] = 0;
> + ret = bind(fd, (struct sockaddr *)&address, len);
> + if (ret < 0)
> + err(1, "bind");
Throw exceptions, use descriptive error messages?
> +
> + // listen to socket
> + ret = listen(fd, 5);
> + if (ret < 0)
> + err(1, "listen");
> + }
> +
> + listen_fd = fd;
> + fds[LISTEN_FD].fd = fd;
> +
> + // detect TTY changes
> + fd = open("/sys/class/tty/tty0/active", O_RDONLY|O_CLOEXEC);
> + if (fd < 0)
> + err(1, "open");
> + fds[VT_FD].fd = fd;
> + fds[VT_FD].events = POLLPRI;
> +}
> +
> +static bool check_xauthority(const char *fn, uid_t uid)
> +{
> + // TODO timeout on check, could have passed a weird NFS
> + // impersonate uid
> + // file must be present
> + // file must be small
> + // read file
> + // check for keys (memmem)
> + // MIT-MAGIC-COOKIE-1
> + // XDM-AUTHORIZATION-1
> + // MIT-KERBEROS-5
> + // SUN-DES-1
> + return true;
> +}
> +
> +static void remove_client(unsigned n)
> +{
> + close(client_fds[n].fd);
> + client_fds[n].fd = -1;
> + if (n != num_clients-1) {
> + client_fds[n] = client_fds[num_clients-1];
> +#if CLIENT_DATA
> + client_data[n] = client_data[num_clients-1];
> +#endif
> + }
> + --num_clients;
> +}
> +
> +/**
> + * Get terminal number from PID.
> + * @returns terminal or -1 if error
> + */
> +static int get_terminal(pid_t pid)
> +{
> + char fn[128];
> + sprintf(fn, "/proc/%u/stat", pid);
> + FILE *f = fopen(fn, "re");
> + if (!f)
> + return -1;
> +
> + char line[1024*2];
> + if (fgets(line, sizeof(line), f) == NULL) {
> + fclose(f);
> + return -1;
> + }
> + fclose(f);
This could also be done using std::ifstream (though streams in C++ are
kind of borked...).
Again, no error handling.
> + int terminal = -1, tty = -1;
> + const char *end_exename = strstr(line, ") ");
> + if (end_exename && sscanf(end_exename+2, "%*c %*d %*d %*d %d", &tty) > 0) {
> + // check tty is a physical one (just looks at major/minor)
> + int major = tty >> 8;
> + int minor = tty & 0xff;
> + if (major == 4 && minor > 0 && minor < 64)
> + terminal = minor;
> + }
> + return terminal;
> +}
> +
> +/**
> + * Parse a message from a client
> + * @returns true if message is valid.
> + */
> +static bool parse_message(TerminalInfo& info, const uint8_t *msg, size_t msg_len)
> +{
> + memset(&info, 0, sizeof(info));
Oldschool!
Just create info on the stack and return it at the end. In case of
error, throw an exception.
> + if (msg_len < 1 || msg[0] != 1)
> + return false;
> +
> + auto msg_end = msg + msg_len;
> + ++msg;
> + while (msg_end - msg >= 2) {
> + uint8_t type = *msg++;
> + uint8_t len = *msg++;
> + if (msg_end - msg < len)
> + return false;
> +
> + char *out;
> + switch (type) {
> + case 1:
> + out = info.display;
> + break;
> + case 2:
> + out = info.xauthority;
> + break;
> + default:
> + return false;
> + }
> +
> + memcpy(out, msg, len);
> + out[len] = 0;
> + msg += len;
> + }
> + if (msg != msg_end)
> + return false;
> + info.valid = true;
> + return true;
> +}
> +
> +// check message, should contain a DISPLAY and XAUTHORITY
> +// callback when data are received
> +static void data_from_client(unsigned n)
> +{
> + const int fd = client_fds[n].fd;
> +
> + // get message, the protocol specify a single message
> + char msg_buffer[1024*4];
> + iovec iov[1];
> + iov[0].iov_base = msg_buffer;
> + iov[0].iov_len = sizeof(msg_buffer);
> + msghdr msg;
> + memset(&msg, 0, sizeof(msg));
> + msg.msg_iov = iov;
> + msg.msg_iovlen = 1;
> + ssize_t msg_len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC|MSG_DONTWAIT);
> + if (msg_len < 0 && errno == EAGAIN)
> + return;
> + if (msg_len < 0 && errno == EWOULDBLOCK)
> + return;
> + if (msg_len <= 0) {
> + remove_client(n);
> + return;
> + }
> +
> + // get credentials (uid+pid)
> + ucred cred;
> + socklen_t cred_length = sizeof(cred);
> + if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &cred, &cred_length) < 0) {
> + remove_client(n);
> + return;
> + }
> +
> + // get tty terminal from process
> + int num_terminal = get_terminal(cred.pid);
> +
> + // send an ack to the client, we got all the information
> + remove_client(n);
> +
> + if (num_terminal < 0)
> + return;
> +
> + // parse message, should contain data and credentials
> + TerminalInfo info;
> + if (!parse_message(info, (const uint8_t *) msg_buffer, msg_len))
> + return;
> +
> + // check Xauthority using the uid passed
> + if (!check_xauthority(info.xauthority, cred.uid))
> + return;
> +
> + // set final informations
> + info.uid = cred.uid;
> + terminal_info[num_terminal] = info;
> +}
> +
> +static void handle_new_fd(int fd)
> +{
> + // limit number of clients
> + if (num_clients >= max_clients) {
> + close(fd);
> + return;
> + }
> +
> + // append to loop handlers
> + client_fds[num_clients].fd = fd;
> + client_fds[num_clients].events = POLLIN;
> + client_fds[num_clients].revents = 0;
> +#if CLIENT_DATA
> + memset(&client_data[num_clients], 0, sizeof(client_data[num_clients]));
> +#endif
> + ++num_clients;
> +
> + // TODO timeout for data
> +}
> +
> +static void handle_fd_events(unsigned n, unsigned events)
> +{
> + if (events == POLLIN) {
> + data_from_client(n);
> + return;
> + }
> + // delete client if other events
> + if (events)
> + remove_client(n);
> +}
> +
> +static int current_tty = -1;
> +static int working_tty = -1;
> +
> +static void handle_vt_change(int fd)
> +{
> + char vt_name[128];
> + for (;;) {
> + auto len = read(fd, vt_name, sizeof(vt_name));
> + if (len < 0 && errno == EINTR)
> + continue;
> + if (len < 0)
> + // TODO error
> + return;
> +
> + unsigned tty_num;
> + if (sscanf(vt_name, "tty%u", &tty_num) == 1 && tty_num < 64) {
> + current_tty = tty_num;
> + }
> + lseek(fd, 0, SEEK_SET);
> + break;
> + }
> +}
> +
> +static bool check_agent()
> +{
> + syslog(LOG_WARNING, "tty working %d current %d", working_tty, current_tty);
> +
> + // TODO execute this part also on main loop
> + // when should we try again ?
> + if (working_tty == current_tty && child_pid != -1)
> + return false;
> +
> + syslog(LOG_WARNING, "trace %d", __LINE__);
> + // can we handle a new TTY ?
> + if (!terminal_info[current_tty].valid)
> + return false;
> +
> + syslog(LOG_WARNING, "trace %d child pid %d", __LINE__, (int) child_pid);
> + // TODO who clear TTY when reset ?
> + // TODO switch to text ?
> +
> + if (child_pid != -1)
> + return false;
> +
> + syslog(LOG_WARNING, "trace %d", __LINE__);
> + // save pid, manage only one agent
> + child_pid = fork();
Is the whole code structure upside down now? The daemon here is the
parent process which should be forking children, but main() is in
spice-streaming-agent.cpp. Does this even work after the first fork?
This is really ugly and some serious code refactoring should have
happened before introducing this... I've already been thinking about
it. The streaming code should be separated from the main .cpp file and
encapsulated and only be called from there.
> + switch (child_pid) {
> + case -1:
> + // TODO
> + return false;
> + case 0:
> + // child
> + child_pid = -1;
> + break;
> + default:
> + // parent
> + // TODO close streamfd opened to lock it
> + return false;
> + }
> +
> + // we are the child here, we return to do the stream work
> + uid_t uid = terminal_info[current_tty].uid;
> + syslog(LOG_WARNING, "trace %d uid %d", __LINE__, (int) uid);
> + passwd *pw = getpwuid(uid);
> + if (!pw) {
> + exit(1);
> + }
> +
> + working_tty = current_tty;
> + return true;
> +}
> +
> +static void loop()
> +{
> + // poll for new events
> + while (true) {
> + // check if we need to execute the agent
> + // this should be done here so if poll get a EINTR
> + // for a closed child we check again
> + if (check_agent())
> + break;
> +
> + // limit clients
> + if (num_clients >= max_clients) {
> + fds[0].events = 0;
> + } else {
> + fds[0].events = POLLIN;
> + }
> + if (poll(fds, num_clients+FIXED_FDS, -1) < 0)
> + // TODO errors
> + continue;
> + if ((fds[LISTEN_FD].revents & POLLIN) != 0) {
> + // accept
> + int new_fd = accept(listen_fd, NULL, NULL);
> + if (new_fd < 0)
> + continue;
> + handle_new_fd(new_fd);
> + }
> + if ((fds[VT_FD].revents & POLLPRI) != 0) {
> + handle_vt_change(fds[VT_FD].fd);
> + }
> + for (unsigned n = num_clients; n-- > 0; )
> + if (client_fds[n].revents)
> + handle_fd_events(n, client_fds[n].revents);
> + }
> +}
> +
> +static void handle_sigchild(int)
> +{
> + pid_t pid;
> + int status;
> + while ((pid=waitpid(-1, &status, WNOHANG)) != -1) {
> + if (pid == child_pid) {
> + child_pid = -1;
> + // TODO try to open streamfd if not already opened
> + }
> + }
> +}
> +
> +void daemonize(const char *streamport, int *streamfd)
> +{
This function really does not daemonize anything, so the name is wrong?
> + // ignore pipe, prevent signal handling data from file descriptors
> + signal(SIGPIPE, SIG_IGN);
> + signal(SIGCHLD, handle_sigchild);
> +
> + init();
> +
> + loop();
> +
> + signal(SIGCHLD, SIG_DFL);
> + signal(SIGPIPE, SIG_DFL);
> +
> + // open device before switching user
> + *streamfd = open(streamport, O_RDWR|O_CLOEXEC);
> + if (*streamfd < 0)
> + exit(1);
> +
> + uid_t uid = terminal_info[current_tty].uid;
> + passwd *pw = getpwuid(uid);
> + if (!pw) {
> + exit(1);
> + }
> + if (setgid(pw->pw_gid) != 0) {
> + exit(1);
> + }
> + if (initgroups(pw->pw_name, pw->pw_gid) != 0) {
> + exit(1);
> + }
> + if (setuid(uid) != 0) {
> + exit(1);
> + }
Error handling once again.
> + for (unsigned n = 0; n < num_clients+FIXED_FDS; ++n) {
> + close(fds[n].fd);
> + fds[n].fd = -1;
> + }
> +
> + setenv("DISPLAY", terminal_info[current_tty].display, 1);
> + setenv("XAUTHORITY", terminal_info[current_tty].xauthority, 1);
> +}
> diff --git a/src/daemon.h b/src/daemon.h
> new file mode 100644
> index 0000000..007ba7f
> --- /dev/null
> +++ b/src/daemon.h
> @@ -0,0 +1,19 @@
> +/* Declaration for daemon code
> + *
> + * \copyright
> + * Copyright 2017 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_DAEMON_H_
> +#define SPICE_STREAMING_AGENT_DAEMON_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +void daemonize(const char *streamport, int *streamfd);
> +
> +#ifdef __cplusplus
> +}
> +#endif
Why all this? C file suffix (.h) and the extern "C" for this function?
> +
> +#endif
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 2c32070..91dfead 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -37,6 +37,7 @@
> #include "hexdump.h"
> #include "xorg-utils.hpp"
> #include "concrete-agent.hpp"
> +#include "daemon.h"
>
> using namespace std;
> using namespace SpiceStreamingAgent;
> @@ -166,6 +167,8 @@ static int read_command_from_device(void)
> static void start_capture()
> {
> for (int attempts = 0; ; ) {
> + if (streamfd >= 0)
> + break;
> streamfd = open(streamport.c_str(), O_RDWR);
> if (streamfd >= 0)
> break;
> @@ -206,7 +209,7 @@ static int read_command(bool blocking)
> if (vt_active) {
> start_capture();
> } else {
> - stop_capture();
> + exit(0);
> }
> } else {
> n = read_command_from_stdin();
> @@ -380,7 +383,13 @@ static void cursor_changes(Display *display, int event_base)
> if (vt_property == None || event.xproperty.atom != vt_property)
> continue;
> // update vt property, activate screen read if needed
> - vt_active.store(get_win_prop_int(display, rootwindow, vt_property) != 0, std::memory_order_relaxed);
> + bool vt_active = get_win_prop_int(display, rootwindow, vt_property) != 0;
> + if (!vt_active) {
> + printf("exiting...\n");
> + // this is necessary as to avoid a clean exit that will hangs :(
> + _Exit(0);
> + }
> + ::vt_active.store(vt_active, std::memory_order_relaxed);
> std::atomic_thread_fence(std::memory_order_acquire);
> uint64_t n = 1;
> write(update_fd, &n, sizeof(n));
> @@ -533,6 +542,10 @@ int main(int argc, char* argv[])
> }
> }
>
> + daemonize(streamport.c_str(), &streamfd);
> +
> + // this should be done after the fork to avoid duplicating
> + // resources
> agent.LoadPlugins(PLUGINSDIR);
>
> update_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel