On Mon, 2018-05-21 at 11:45 +0100, 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>
> ---
> .gitignore | 1 +
> 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 | 3 +
> src/Makefile.am | 2 +
> src/daemon.cpp | 619 ++++++++++++++++++++++++++
> src/daemon.hpp | 11 +
> src/spice-streaming-agent.cpp | 32 +-
> src/stream-port.cpp | 7 +
> src/stream-port.hpp | 1 +
> 13 files changed, 700 insertions(+), 12 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.hpp
>
> diff --git a/.gitignore b/.gitignore
> index 601cc9f..14a4b16 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -23,6 +23,7 @@ Makefile.in
> /spice-streaming-agent.spec
> /spice-streaming-agent.pc
> /data/spice-streaming.desktop
> +/data/spice-streaming-agent.service
> /libtool
> /m4/libtool.m4
> /m4/ltoptions.m4
> diff --git a/Makefile.am b/Makefile.am
> index 32fdaff..21e5797 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -18,9 +18,17 @@ pkgconfig_DATA = spice-streaming-agent.pc
> udevrulesdir = $(UDEVRULESDIR)
> 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)
> +
> 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 fe1bb41..a75f22c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -59,6 +59,9 @@ AC_ARG_WITH(udevrulesdir,
> )
> AC_SUBST(UDEVRULESDIR)
>
> +SYSTEMDSYSTEMUNITDIR=`${PKG_CONFIG} systemd --variable=systemdsystemunitdir`
> +AC_SUBST(SYSTEMDSYSTEMUNITDIR)
> +
> dnl ===========================================================================
> dnl check compiler flags
>
> @@ -112,6 +115,7 @@ AC_DEFINE_DIR([BINDIR], [bindir], [Where binaries are installed.])
> 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 f143c77..2ed27eb 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/org.spice-space.stream.0", MODE="0666"
> -
> +ACTION=="add", SUBSYSTEM=="virtio-ports", ENV{DEVLINKS}=="/dev/virtio-ports/org.spice-space.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..3af2112
> --- /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-org.spice\x2dspace.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 d9323bb..fd10270 100644
> --- a/spice-streaming-agent.spec.in
> +++ b/spice-streaming-agent.spec.in
> @@ -12,6 +12,7 @@ BuildRequires: spice-protocol >= @SPICE_PROTOCOL_MIN_VER@
> BuildRequires: libX11-devel libXfixes-devel
> BuildRequires: libjpeg-turbo-devel
> BuildRequires: catch-devel
> +BuildRequires: systemd-devel
> # we need /usr/sbin/semanage program which is available on different
> # packages depending on distribution
> Requires(post): /usr/sbin/semanage
> @@ -61,6 +62,8 @@ fi
> %{_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 ffc52b2..9b4bb30 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -64,4 +64,6 @@ spice_streaming_agent_SOURCES = \
> stream-port.hpp \
> eventfd.cpp \
> eventfd.hpp \
> + daemon.cpp \
> + daemon.hpp \
> $(NULL)
> diff --git a/src/daemon.cpp b/src/daemon.cpp
> new file mode 100644
> index 0000000..e93329b
> --- /dev/null
> +++ b/src/daemon.cpp
> @@ -0,0 +1,619 @@
> +/* 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.
> + */
> +
> +#include <config.h>
> +#include "daemon.hpp"
> +#include "eventfd.hpp"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <syslog.h>
> +#include <poll.h>
> +#include <errno.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <fcntl.h>
> +#include <pwd.h>
> +#include <grp.h>
> +#include <systemd/sd-daemon.h>
> +#include <string>
> +#include <map>
> +#include <stdexcept>
> +
> +/*
> + * There are 3 "roles" for the agent:
> + * - main agent;
> + * - daemon
> + * - helper
> + * The role of the agent is to handle a given graphical session
> + * capturing and sending video stream to SPICE server.
> + * The role 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. These
> + * information are used to be able to connect to the display server
> + * (like X).
> + * The agent is a child (forked) of the daemon.
> + * 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
> + * switch between sessions;
> + * - running agents directly launched from a graphical session cause
> + * some issue with SELinux, launching outside a Unix session allows
> + * 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 display server.
> + * 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
You're using the word 'client' while you're talking about the helper,
also in the code below. The naming isn't very clear to begin with, this
makes it even harder to understand...
> + * 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).
> + *
> + * 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).
> + */
> +
> +static const char daemon_socket_name[] = "@spice-streaming-agent-daemon";
> +
> +struct TerminalInfo
> +{
> + TerminalInfo() = default;
> + TerminalInfo(const uint8_t *msg, size_t msg_len);
> +
> + std::string display;
> + std::string xauthority;
> + uid_t uid;
> +};
Seems like a better name would be e.g. XorgSessionInfo?
> +/**
> + * Parse a message from a client.
> + * Throw an exception if the message content is invalid.
> + */
> +TerminalInfo::TerminalInfo(const uint8_t *msg, size_t msg_len)
> +{
> + if (msg_len < 1 || msg[0] != 1) {
> + throw std::runtime_error("Invalid message header");
> + }
> +
> + 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) {
> + throw std::runtime_error("Invalid field header");
> + }
> +
> + switch (type) {
> + case 1:
> + display = std::string((const char*) msg, len);
> + break;
> + case 2:
> + xauthority = std::string((const char*) msg, len);
> + break;
> + default:
> + throw std::runtime_error("Invalid field");
> + }
> +
> + msg += len;
> + }
> + if (msg != msg_end) {
> + throw std::runtime_error("Message not terminated correctly");
> + }
> +}
> +
> +static void api_err(const char *msg)
> +{
> + syslog(LOG_ERR, "%s: %m", msg);
> + exit(1);
> +}
What does 'api' mean here? And why not throw exceptions?
> +/**
> + * Get terminal number from PID.
> + * This function is supposed to be quite low level.
> + * We don't log any error to avoid possibles DoS.
> + * The caller can use errno.
> + * @returns terminal or -1 if error
> + */
> +static int get_terminal(pid_t pid)
> +{
> + char fn[128];
> + sprintf(fn, "/proc/%u/stat", pid);
> +
> + // use C style FILE, the kernel document this file syntax using
> + // scanf format specification
> + 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);
> +
> + 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;
> +}
> +
> +static bool check_xauthority(const std::string& 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;
> +}
> +
> +/**
> + * Check if a given file descriptor is the daemon socket we should
> + * accept requests from.
> + * In case the daemon is launched form inetd or systemd the file
> + * descriptor is inherited from the parent.
> + */
> +static bool fd_is_agent(int fd)
> +{
> + // must be a socket
> + struct stat st;
> + if (fstat(fd, &st) != 0) {
> + return false;
> + }
> + 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, daemon_socket_name) != 0) {
> + return false;
> + }
> +
> + // TODO must be in listening, but this is mainly a paranoia check,
> + // the file descriptor can come only from a trusted source
> +
> + return true;
> +}
> +
> +class Daemon
> +{
> +public:
> + Daemon(const char *stream_port_name);
> + ~Daemon();
> + void loop(int &streamfd);
> +private:
> + void remove_client(unsigned n);
> + void data_from_client(unsigned n);
> + void handle_new_fd(int fd);
> + void handle_fd_events(unsigned n, unsigned events);
> + bool check_agent(int &streamfd);
> +
> + enum { max_clients = 63 };
> + enum {
> + LISTEN_FD,
> + WAKEUP_FD,
> + VT_FD,
> + FIXED_FDS
> + };
> + spice::streaming_agent::EventFD wakeup_loop;
> + struct pollfd fds[max_clients+FIXED_FDS];
> + struct pollfd * const client_fds = fds+FIXED_FDS;
I'm still confused by how you handle the "clients" (which are, in fact,
the helpers?) in the Daemon. They are kept in an array but the only
thing you ever do with them is remove them when they (it seems) close
the connection? Can you describe the logic behind it please?
> + unsigned num_clients = 0;
> + pid_t child_pid = -1;
> + int working_tty = -1;
> + const char *stream_port_name;
> +
> + std::map<int, TerminalInfo> terminal_info;
> +
> + static void handle_sigchild(int);
> + static Daemon *current;
> +};
It's unusual to have the sort-of "main class" definition buried in the
cpp file and have a single method the header that instantiates it. Any
reason not to have the class exposed and having a Daemon::run() instead
of run_daemon()?
> +Daemon *Daemon::current = nullptr;
> +
> +Daemon::Daemon(const char *stream_port_name):
> + stream_port_name(stream_port_name)
> +{
> + int ret;
> +
> + fds[WAKEUP_FD].fd = wakeup_loop.raw_fd();
> + fds[WAKEUP_FD].events = POLLIN;
> +
> + int fd;
> + if (fd_is_agent(0)) {
> + // inetd style socket passed
> + fd = 0;
> + } else if (fd_is_agent(SD_LISTEN_FDS_START)) {
> + // systemd style socket passed
> + fd = SD_LISTEN_FDS_START;
> + } else {
> + // open socket
> + fd = socket(PF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0);
> + if (fd < 0) {
> + api_err("Unable to open daemon socket");
> + }
> +
> + struct sockaddr_un address;
> + memset(&address, 0, sizeof(address));
> + address.sun_family = AF_UNIX;
> + strcpy(address.sun_path, daemon_socket_name);
> + int len = SUN_LEN(&address);
> + address.sun_path[0] = 0;
> + ret = bind(fd, (struct sockaddr *)&address, len);
> + if (ret < 0) {
> + api_err("Unable to bind daemon socket");
> + }
> + // listen to socket
> + ret = listen(fd, 5);
> + if (ret < 0) {
> + api_err("Unable to listen to daemon socket");
> + }
> + }
> +
> + fds[LISTEN_FD].fd = fd;
> +
> + // detect TTY changes
> + fd = open("/sys/class/tty/tty0/active", O_RDONLY|O_CLOEXEC);
> + if (fd < 0) {
> + api_err("Unable to open TTY change file");
> + }
> + fds[VT_FD].fd = fd;
> + fds[VT_FD].events = POLLPRI;
> +}
> +
> +Daemon::~Daemon()
> +{
> + // close file descriptors
> + // this is executed also on the main streaming agent
"main streaming agent"... this is hard to understand and a bad naming
scheme. Not sure what you mean by the name and not sure why this is
_also_ executed there?
> + for (unsigned n = 0; n < num_clients+FIXED_FDS; ++n) {
> + if (n == WAKEUP_FD) {
> + continue;
> + }
> + close(fds[n].fd);
> + }
> +}
> +
> +void Daemon::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];
> + }
> + --num_clients;
I know you need a single continuous array for the poll(), but you
shouldn't need to manage an array like this in C++... Still thinking of
a solution.
> +}
> +
> +// check message, should contain a DISPLAY and XAUTHORITY
> +// callback when data are received
> +void Daemon::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));
I think you can use '= {}' to initialize a struct to zeroes.
> + 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)
> + // the uid is used to be able to run the streaming agent with
> + // correct user so we should get it from a secure source
> + 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
> + // Don't trust too much the helper sending information, the
> + // terminal cannot be changed easily from normal accounts, get
> + // this information directly from the kernel
> + int num_terminal = get_terminal(cred.pid);
> +
> + // send an ack to the helper, we got all the information
> + // The helper can now terminate
> + remove_client(n);
> +
> + if (num_terminal < 0) {
> + return;
> + }
> +
> + // parse message, should contain data and credentials
> + TerminalInfo info;
> + try {
> + info = TerminalInfo((const uint8_t *) msg_buffer, msg_len);
> + }
> + catch (std::runtime_error& err) {
> + // avoid DoS on the logs just ignoring the error
It's going to be a hell to debug if we don't log any errors here. How
about some kind of rate-limiting if we're really worried about a DoS?
> + 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;
> +}
> +
> +void Daemon::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;
> + ++num_clients;
> +
> + // TODO timeout for data
> +}
> +
> +void Daemon::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);
> + }
Deciding how to process a fd event based on whether it is a POLLIN is
not explicit enough and not at all clear at first glance why you can do
that.
> +}
> +
> +static int current_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;
> + }
> +}
> +
> +bool Daemon::check_agent(int &streamfd)
> +{
> + syslog(LOG_DEBUG, "tty working %d current %d", working_tty, current_tty);
> +
> + pid_t pid;
> + int status;
> + while ((pid=waitpid(-1, &status, WNOHANG)) != -1 && pid != 0) {
> + if (pid == child_pid) {
> + child_pid = -1;
> + }
> + }
> +
> + // try to open streamfd if not already opened
> + if (child_pid == -1 && streamfd < 0) {
> + // open device as soon as possible to make sure is not busy
> + streamfd = open(stream_port_name, O_RDWR|O_CLOEXEC);
> + if (streamfd < 0) {
> + api_err("Unable to open streaming device");
> + }
> + }
> +
> + // 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_DEBUG, "trace %d", __LINE__);
> + // can we handle a new TTY ?
> + if (terminal_info.find(current_tty) == terminal_info.end()) {
> + return false;
> + }
> +
> + syslog(LOG_DEBUG, "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_DEBUG, "trace %d", __LINE__);
> + // save pid, manage only one agent
> + child_pid = fork();
> + switch (child_pid) {
> + case -1:
> + // TODO
> + return false;
> + case 0:
> + // child
> + child_pid = -1;
> + break;
> + default:
> + // parent
> + close(streamfd);
> + streamfd = -1;
> + return false;
> + }
> +
> + // we are the child here, we return to do the stream work
> + uid_t uid = terminal_info[current_tty].uid;
> + syslog(LOG_DEBUG, "trace %d uid %d", __LINE__, (int) uid);
> + passwd *pw = getpwuid(uid);
> + if (!pw) {
> + api_err("Unable to retrieve user information");
> + }
> + if (setgid(pw->pw_gid) != 0) {
> + api_err("Unable to set group");
> + }
> + if (initgroups(pw->pw_name, pw->pw_gid) != 0) {
> + api_err("Unable to set group list");
> + }
> + if (setuid(uid) != 0) {
> + api_err("Unable to set user");
> + }
> +
> + setenv("DISPLAY", terminal_info[current_tty].display.c_str(), 1);
> + setenv("XAUTHORITY", terminal_info[current_tty].xauthority.c_str(), 1);
> +
> + working_tty = current_tty;
> + return true;
> +}
> +
> +void Daemon::loop(int &streamfd)
> +{
> + // ignore pipe, prevent signal handling data from file descriptors
> + signal(SIGPIPE, SIG_IGN);
> + current = this;
> + signal(SIGCHLD, handle_sigchild);
> +
> + // 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(streamfd)) {
> + 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;
> + }
> + wakeup_loop.ack();
> + if ((fds[LISTEN_FD].revents & POLLIN) != 0) {
> + // accept
> + int new_fd = accept(fds[LISTEN_FD].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);
> + }
> + }
> + }
This code around all the fds is pretty complicated and there's no way
to tell what it's actually supposed to be doing without thoroughly
studying it. I'm quite sure it could be written more clearly, but can't
really make concrete suggestions since I still don't completely
understand it... If not, perhaps some more comments on what's acutally
going on would help?
> +
> + signal(SIGCHLD, SIG_DFL);
> + signal(SIGPIPE, SIG_DFL);
> + current = nullptr;
> +}
> +
> +void Daemon::handle_sigchild(int)
> +{
> + current->wakeup_loop.signal();
> +}
> +
> +void run_daemon(const char *stream_port_name, int &streamfd)
> +{
> + streamfd = -1;
> +
> + Daemon daemon(stream_port_name);
> + daemon.loop(streamfd);
> +}
> diff --git a/src/daemon.hpp b/src/daemon.hpp
> new file mode 100644
> index 0000000..33d09e4
> --- /dev/null
> +++ b/src/daemon.hpp
> @@ -0,0 +1,11 @@
> +/* Declaration for daemon code
> + *
> + * \copyright
> + * Copyright 2017 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_DAEMON_HPP
> +#define SPICE_STREAMING_AGENT_DAEMON_HPP
> +
> +void run_daemon(const char *streamport, int &streamfd);
> +
> +#endif
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 240b9c7..261b693 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -11,6 +11,7 @@
> #include "error.hpp"
> #include "xorg-utils.hpp"
> #include "eventfd.hpp"
> +#include "daemon.hpp"
>
> #include <spice/stream-device.h>
> #include <spice/enums.h>
> @@ -198,7 +199,7 @@ static void read_command(StreamPort &stream_port, bool blocking)
> update_fd.ack();
> bool vt_active = ::vt_active.load(std::memory_order_relaxed);
> if (!vt_active) {
> - throw std::runtime_error("VT disabled");
> + exit(0);
> }
> continue;
> }
> @@ -342,7 +343,12 @@ static void cursor_changes(StreamPort *stream_port, Display *display, int event_
> 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) {
> + // 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);
> update_fd.signal();
> continue;
> @@ -525,13 +531,6 @@ int main(int argc, char* argv[])
> }
> }
>
> - // register built-in plugins
> - MjpegPlugin::Register(&agent);
> -
> - agent.LoadPlugins(pluginsdir);
> -
> - register_interrupts();
> -
> FILE *f_log = NULL;
> if (log_filename) {
> f_log = fopen(log_filename, "wb");
> @@ -547,6 +546,19 @@ int main(int argc, char* argv[])
> }
> old_args.clear();
>
> + int streamfd;
> + run_daemon(stream_port_name, streamfd);
While this is much better than v1 of the series (by having the daemon
clearly separated), this is still upside-down in my head, since you run
the daemon here, but later, when you fork off a child, the control flow
falls through back here and continues with what's below. Here's how I'd
imagine it:
// spice-streaming-agent.cpp
// ...
int main() {
// ...
Daemon daemon;
daemon.run();
// (no agent code here)
}
// daemon.cpp
// ...
Daemon::run() {
// ...
pid_t pid = fork();
if (pid) {
// ...
} else {
StreamingAgent agent;
agent.run();
}
}
Supposing the StreamingAgent class encompasses all the streaming agent
functionality currently in spice-streaming-agent.cpp. That is, all of
the agent is launched from inside the Daemon.
This does require to finish the separation of everything in spice-
streaming-agent.cpp.
> +
> + // this should be done after the fork to avoid duplicating
> + // resources
> +
> + // register built-in plugins
> + MjpegPlugin::Register(&agent);
> +
> + agent.LoadPlugins(pluginsdir);
> +
> + register_interrupts();
> +
> Display *display = XOpenDisplay(NULL);
> if (display == NULL) {
> syslog(LOG_ERR, "failed to open display\n");
> @@ -576,7 +588,7 @@ int main(int argc, char* argv[])
> int ret = EXIT_SUCCESS;
>
> try {
> - StreamPort stream_port(stream_port_name);
> + StreamPort stream_port(streamfd);
Why did you break the RAII and encapsulation again by introducing a fd
constructor?
>
> std::thread cursor_th(cursor_changes, &stream_port, display, event_base);
> cursor_th.detach();
> diff --git a/src/stream-port.cpp b/src/stream-port.cpp
> index 5528854..2ce5854 100644
> --- a/src/stream-port.cpp
> +++ b/src/stream-port.cpp
> @@ -26,6 +26,13 @@ StreamPort::StreamPort(const std::string &port_name) : fd(open(port_name.c_str()
> }
> }
>
> +StreamPort::StreamPort(int fd) : fd(fd)
> +{
> + if (fd < 0) {
> + throw Error("Streaming device not opened");
> + }
> +}
> +
> StreamPort::~StreamPort()
> {
> close(fd);
> diff --git a/src/stream-port.hpp b/src/stream-port.hpp
> index 9187cf5..775d15e 100644
> --- a/src/stream-port.hpp
> +++ b/src/stream-port.hpp
> @@ -18,6 +18,7 @@ namespace streaming_agent {
> class StreamPort {
> public:
> StreamPort(const std::string &port_name);
> + explicit StreamPort(int fd);
> ~StreamPort();
>
> void read(void *buf, size_t len);
I feel like I could have done a better job, but I'm still struggling to
understand how it's supposed to work... So hopefully what I wrote makes
sense and you can help me understand it :)
Cheers,
Lukas
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel