[Spice-devel] [PATCH spice-streaming-agent 4/5] Implement a daemon/agent separation
Christophe de Dinechin
cdupontd at redhat.com
Mon Feb 5 17:05:17 UTC 2018
> On 2 Feb 2018, at 18:34, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> 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 at redhat.com>
>> ---
>> 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.
I agree the code seems to have trouble deciding if it’s C or C++.
Frediano, do you prefer to go “all the way” to C++, or rather stick with C? If we go for C++, can we stick to a C++ way of doing things? But I’m not sure what C++ brings us for that kind of code…
Christophe
>
>> + 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list