On Mon, 2018-02-05 at 18:05 +0100, Christophe de Dinechin wrote:
> > On 2 Feb 2018, at 18:34, Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@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.
>
> 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…
Although it seems to me you've picked a wrong remark of mine to reply
to (I was asking if the code structure around fork could be improved,
should apply to C as well as C++), good question. :)
I think C++ still brings benefits, although smaller than they would be
in higher-level code.
But better to answer this so that I know I should stop trying :)
Lukas
> >
> > > + 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
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel