[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