On Tue, 2018-02-06 at 06:08 -0500, Frediano Ziglio 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...
> >
>
> Is to limit the diff, consider adding a file:
>
> LIST = \
> this.cpp
>
> to
>
> LIST = \
> this.cpp \
> that.cpp
>
> this is 2 line diff while
>
> LIST = \
> this.cpp \
> $(NULL)
>
> to
>
> LIST = \
> this.cpp \
> that.cpp \
> $(NULL)
>
> is just one.
Oh... I wouldn't have thought of that... Ok, but deciding between
better diffs or better code, I'd pick better code.
> > > 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.
> >
>
> Believe it or not this was discussed on IRC!
You mean the naming? Hmm... :)
> > > + * 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.
> >
>
> described below
It is, actually.. I would add maybe the daemon listens for messages on
an open socket, to make it clear (for noobs like me ;))
> > - 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 :)
> >
>
> For SPICE protocol (server <-> client) we already generate code but
> a network protocol and a local protocol usually have different
> requirements.
>
> > 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
> >
>
> Well... the number of RFCs and protocols out there are quite massive,
> I won't say that.
>
> > [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.
> >
>
> "StreamingWorker" for a class that does not stream at all does not
> seem a great name, "Daemon" seems better.
Right. StreamingWorker was meant as new name for the Agent, not for the
Daemon. See how confused I was? :) The problem is neither daemon nor
agent really describe what the process is doing...
> Yes, this is mainly C code renamed as C++, the original (not that
> long ago) implementation was plain C, that's the reason for the
> "extern C" in the header.
>
> > > +#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'? :)
> >
>
> Changing, as I said in my previous e-mail this was a "usually" for me
> meaning that I prefer no bracket for the simple case, but seems that
> there are more people liking the always bracket rule, I'll change.
>
> > > + 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 :)
> >
>
> I'll do to. Personally I prefer static const char[] for this
> specific case.
>
> > > + 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?
> >
>
> I'll do. And also err output in the wrong place.
>
> > > +
> > > + // 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...).
> >
>
> I prefer this style, the format is specified for the file is expressed
> in scanf format.
>
> > Again, no error handling.
> >
>
> error handling is implemented here, what you mean is error reporting,
Right. I consider returning the same error code and no message on all
errors almost the same though :)
> I'll write the choice about why not reporting.
This I don't understand.
> > > + 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.
> >
>
> yes, make sense.
>
> > > + 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.
> >
>
> fork is more a problem than it seems.
> I agree that the code is usually "if (fork() == 0) { exit(run_worker()); }" but
> if you want to free resources before calling the worker usually you want some
> kind of "reset" that free them which is kind of ugly too (usually these stuff
> are in the destructor). The fork usually is a bit of "pain" as you don't want
> a full cleanup but just to close the file descriptors (for instance you don't
> want to flush software buffers on child and parent or use shutdown for sockets).
>
> Using "return on child" allows easily if these stuff is encapsulated to
> just use the destructor instead of a "reset" function.
Ok, I'm not sure this is a reply to what I meant by my comment. It may
be that I just don't understand it, I'm not too experienced with
forking.
But the issue I'm having is that the daemon should be the parent
forking off children. So the logical code structure (for me) would be
to call the daemon from main(), and in the daemon's loop fork off a
process in which you call the agent code.
What you do instead is you call daemon and then the agent code from
main(), and in the daemon loop you fork() and break out of the loop
when you are forking off and agent. And then let the code get back to
main and continue with the main() code for the forked agent.
This is what I consider upside down and I think it could be structured
better?
> > > + 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?
> >
>
> Yes, maybe better run_daemon or daemon_mode ?
It's hard, because 'daemon' is a generic name for and operating system
process running in the background doing an arbitrary task... The
daemons of old forked themselves to the background, which we don't do
anymore, but every time you mention a daemon in the code, I think
everyone will think something like this? That's why I don't like the
name. But if you must, 'run_daemon' seems better :)
Thanks,
Lukas
> > > + // 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.
> >
>
> Yes, specifically error reporting.
>
> > > + 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?
> >
>
> Was plain C.
>
> > > +
> > > +#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");
>
> I'll remove this.
>
> > > + // 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);
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel