[Spice-devel] [RFC PATCH spice-streaming-agent v2] separate and encapsulate the agent business code
Lukáš Hrázký
lhrazky at redhat.com
Mon Feb 12 12:36:58 UTC 2018
On Mon, 2018-02-12 at 03:53 -0500, Frediano Ziglio wrote:
> >
> > Create an AgentRunner (TODO: needs a better name) class to encapsulate
> > the streaming and communication with the server. The basic setup (cmd
>
> Also encapsulate command line interface (quite odd, does it make still
> much sense?)
Well, yes.. if I understand what you mean, that is the other half that
is separated... Want me to mention it explicitly in the commit message?
> > arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
> > functions is moved to the AgentRunner class and modified as little as
> > possible:
> > - The cursor updating code is moved into a functor called CursorThread
> > - Some initialization and cleanup is moved to AgentRunner's constructor
> > and destructor
> > - Some error handling moved over to exceptions, mainly what was in
> > main() and do_capture()
> > - A couple of variables gently renamed.
> >
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> > Changes since v1:
> > - update according to Frediano's namespace changes
> >
> > src/Makefile.am | 2 +
> > src/main.cpp | 126 ++++++++++++
> > src/spice-streaming-agent.cpp | 458
> > +++++++++++++++++-------------------------
> > src/spice-streaming-agent.hpp | 57 ++++++
> > 4 files changed, 372 insertions(+), 271 deletions(-)
> > create mode 100644 src/main.cpp
> > create mode 100644 src/spice-streaming-agent.hpp
> >
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 6d37274..42bb10f 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
> >
> > spice_streaming_agent_SOURCES = \
> > spice-streaming-agent.cpp \
> > + spice-streaming-agent.hpp \
> > static-plugin.cpp \
> > static-plugin.hpp \
> > concrete-agent.cpp \
> > @@ -57,4 +58,5 @@ spice_streaming_agent_SOURCES = \
> > mjpeg-fallback.hpp \
> > jpeg.cpp \
> > jpeg.hpp \
> > + main.cpp \
> > $(NULL)
> > diff --git a/src/main.cpp b/src/main.cpp
> > new file mode 100644
> > index 0000000..46eda85
> > --- /dev/null
> > +++ b/src/main.cpp
> > @@ -0,0 +1,126 @@
> > +/* An implementation of a SPICE streaming agent
> > + *
> > + * \copyright
> > + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#include <config.h>
> > +#include "spice-streaming-agent.hpp"
> > +
> > +#include <string.h>
> > +#include <getopt.h>
> > +#include <unistd.h>
> > +#include <syslog.h>
> > +#include <signal.h>
> > +
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +static void usage(const char *progname)
> > +{
> > + printf("usage: %s <options>\n", progname);
> > + printf("options are:\n");
> > + printf("\t-p portname -- virtio-serial port to use\n");
> > + printf("\t-i accept commands from stdin\n");
> > + printf("\t-l file -- log frames to file\n");
> > + printf("\t--log-binary -- log binary frames (following -l)\n");
> > + printf("\t-d -- enable debug logs\n");
> > + printf("\t-c variable=value -- change settings\n");
> > + printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> > + printf("\n");
> > + printf("\t-h or --help -- print this help message\n");
> > +
> > + exit(1);
> > +}
> > +
> > +void handle_interrupt(int intr)
> > +{
> > + syslog(LOG_INFO, "Got signal %d, exiting", intr);
> > + ssa::AgentRunner::quit = true;
> > +}
> > +
> > +void register_interrupts(void)
> > +{
> > + struct sigaction sa;
> > +
> > + memset(&sa, 0, sizeof(sa));
> > + sa.sa_handler = handle_interrupt;
> > + if ((sigaction(SIGINT, &sa, NULL) != 0) &&
> > + (sigaction(SIGTERM, &sa, NULL) != 0)) {
> > + syslog(LOG_WARNING, "failed to register signal handler %m");
> > + }
> > +}
> > +
> > +int main(int argc, char* argv[])
> > +{
> > + std::string stream_port = "/dev/virtio-ports/com.redhat.stream.0";
> > + char opt;
> > + std::string log_filename;
> > + int log_binary = 0;
> > + bool stdin_ok = false;
> > + int logmask = LOG_UPTO(LOG_WARNING);
> > + struct option long_options[] = {
> > + { "log-binary", no_argument, &log_binary, 1},
> > + { "help", no_argument, NULL, 'h'},
> > + { 0, 0, 0, 0}
> > + };
> > +
> > + if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
> > + stdin_ok = true;
> > + }
> > +
> > + openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) :
> > LOG_PID, LOG_USER);
> > + setlogmask(logmask);
> > +
> > + std::vector<std::pair<std::string, std::string>> options;
> > +
> > + while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL))
> > != -1) {
> > + switch (opt) {
> > + case 0:
> > + /* Handle long options if needed */
> > + break;
> > + case 'i':
> > + stdin_ok = true;
> > + openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
> > + break;
> > + case 'p':
> > + stream_port = optarg;
> > + break;
> > + case 'c': {
> > + char *p = strchr(optarg, '=');
> > + if (p == NULL) {
> > + syslog(LOG_ERR, "wrong 'c' argument %s\n", optarg);
> > + usage(argv[0]);
> > + }
> > + *p++ = '\0';
> > + options.push_back(std::make_pair(optarg, p));
> > + break;
> > + }
> > + case 'l':
> > + log_filename = optarg;
> > + break;
> > + case 'd':
> > + logmask = LOG_UPTO(LOG_DEBUG);
> > + setlogmask(logmask);
> > + break;
> > + case 'h':
> > + usage(argv[0]);
> > + break;
> > + }
> > + }
> > +
> > + register_interrupts();
> > +
> > + try {
> > + ssa::AgentRunner runner(stream_port, log_filename, log_binary != 0,
> > stdin_ok);
> > + // TODO fix overcomplicated passing of options to the agent
> > + runner.add_options(options);
> > + runner.run();
> > + } catch (const std::exception &e) {
> > + syslog(LOG_ERR, "Error: %s\n", e.what());
> > + return EXIT_FAILURE;
> > + }
> > +
> > + closelog();
> > + return EXIT_SUCCESS;
> > +}
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 0e7641e..f062ebe 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -1,44 +1,37 @@
> > /* An implementation of a SPICE streaming agent
> > *
> > * \copyright
> > - * Copyright 2016-2017 Red Hat Inc. All rights reserved.
> > + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> > */
> >
> > -#include <stdio.h>
> > -#include <stdlib.h>
> > -#include <stdint.h>
> > +#include <config.h>
> > +#include "spice-streaming-agent.hpp"
> > +
> > +#include "concrete-agent.hpp"
> > +#include "hexdump.h"
> > +
> > +#include <spice-streaming-agent/frame-capture.hpp>
> > +#include <spice-streaming-agent/plugin.hpp>
> > +#include <spice/stream-device.h>
> > +#include <spice/enums.h>
> > +#include <X11/Xlib.h>
> > +#include <X11/extensions/Xfixes.h>
> > #include <string.h>
> > -#include <getopt.h>
> > #include <unistd.h>
> > -#include <errno.h>
> > #include <fcntl.h>
> > #include <sys/time.h>
> > #include <poll.h>
> > #include <syslog.h>
> > -#include <signal.h>
> > -#include <exception>
> > -#include <stdexcept>
> > -#include <memory>
> > -#include <mutex>
> > #include <thread>
> > -#include <vector>
> > #include <functional>
> > -#include <X11/Xlib.h>
> > -#include <X11/extensions/Xfixes.h>
> > -
> > -#include <spice/stream-device.h>
> > -#include <spice/enums.h>
> >
> > -#include <spice-streaming-agent/frame-capture.hpp>
> > -#include <spice-streaming-agent/plugin.hpp>
> >
> > -#include "hexdump.h"
> > -#include "concrete-agent.hpp"
> > +namespace spice {
> > +namespace streaming_agent {
> >
> > -using namespace std;
> > -using namespace spice::streaming_agent;
> > +bool AgentRunner::quit = false;
> >
> > -static ConcreteAgent agent;
> > +namespace {
> >
> > struct SpiceStreamFormatMessage
> > {
> > @@ -52,15 +45,155 @@ struct SpiceStreamDataMessage
> > StreamMsgData msg;
> > };
> >
> > -static int streaming_requested;
> > -static std::set<SpiceVideoCodecType> client_codecs;
> > -static bool quit;
> > -static int streamfd = -1;
> > -static bool stdin_ok;
> > -static int log_binary = 0;
> > -static std::mutex stream_mtx;
> > +size_t write_all(int fd, const void *buf, const size_t len)
> > +{
> > + size_t written = 0;
> > + while (written < len) {
> > + int l = write(fd, (const char *) buf + written, len - written);
> > + if (l < 0 && errno == EINTR) {
> > + continue;
> > + }
> > + if (l < 0) {
> > + syslog(LOG_ERR, "write failed - %m");
> > + return l;
> > + }
> > + written += l;
> > + }
> > + syslog(LOG_DEBUG, "write_all -- %u bytes written\n", (unsigned)written);
> > + return written;
> > +}
> > +
> > +class CursorThread {
> > +public:
> > + CursorThread(int streamfd, std::mutex &stream_mtx) :
> > + streamfd(streamfd),
> > + stream_mtx(stream_mtx)
> > + {
> > + display = XOpenDisplay(NULL);
> > + if (display == NULL) {
> > + throw std::runtime_error("Failed to open display.");
> > + }
> > + int error_base;
> > + if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> > + throw std::runtime_error("XFixesQueryExtension failed");
> > + }
> > + Window rootwindow = DefaultRootWindow(display);
> > + XFixesSelectCursorInput(display, rootwindow,
> > XFixesDisplayCursorNotifyMask);
> > + }
> > +
>
> I personally prefer long function to be defined outside the class
> so is easier to see the interface. Also this hint the compiler
> to inline long functions.
Does it really hint the compiler? I find it slightly better visually to
have the methods inline, because you don't need to jump around the code
to find the definitions. Although The class would perhaps deserve it's
own source file + header, which would solve this issue :)
> > + void operator()()
> > + {
> > + unsigned long last_serial = 0;
> > +
> > + while (1) {
> > + syslog(LOG_ERR, "CURSOR LOOP");
> > + XEvent event;
> > + XNextEvent(display, &event);
> > + if (event.type != event_base + 1)
> > + continue;
> > +
> > + XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> > + if (!cursor)
> > + continue;
> > +
> > + if (cursor->cursor_serial == last_serial)
> > + continue;
> > +
> > + last_serial = cursor->cursor_serial;
> > + auto fill_cursor = [&](uint32_t *pixels) {
> > + for (unsigned i = 0; i < cursor->width * cursor->height;
> > ++i)
> > + pixels[i] = cursor->pixels[i];
> > + };
> > + send_cursor(cursor->width, cursor->height, cursor->xhot,
> > cursor->yhot,
> > + fill_cursor, streamfd, stream_mtx);
> > + }
> > + }
> > +
> > +private:
> > + void send_cursor(unsigned width, unsigned height, int hotspot_x, int
> > hotspot_y,
> > + std::function<void(uint32_t *)> fill_cursor,
> > + int streamfd, std::mutex &stream_mtx)
> > + {
> > + if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > + height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > + return;
> > +
> > + size_t cursor_size =
> > + sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > + width * height * sizeof(uint32_t);
> > + std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > +
> > + StreamDevHeader
> > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > + memset(&dev_hdr, 0, sizeof(dev_hdr));
> > + dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > + dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> > + dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> > +
> > + StreamMsgCursorSet &cursor_msg(
> > + *reinterpret_cast<StreamMsgCursorSet *>(msg.get() +
> > sizeof(StreamDevHeader)));
> > +
> > + memset(&cursor_msg, 0, sizeof(cursor_msg));
> > +
> > + cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > + cursor_msg.width = width;
> > + cursor_msg.height = height;
> > + cursor_msg.hot_spot_x = hotspot_x;
> > + cursor_msg.hot_spot_y = hotspot_y;
> > +
> > + uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > + fill_cursor(pixels);
> > +
> > + std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > + write_all(streamfd, msg.get(), cursor_size);
> > + }
> > +
> > + Display* display;
> > + int event_base = 0;
> > + int streamfd = -1;
> > + std::mutex &stream_mtx;
> > +};
> > +
> > +} // namespace
> >
> > -static int have_something_to_read(int *pfd, int timeout)
> > +AgentRunner::AgentRunner(const std::string &stream_port,
> > + const std::string &log_filename,
> > + bool log_binary,
> > + bool stdin_ok)
> > +:
> > + stream_port(stream_port),
> > + log_binary(log_binary),
> > + stdin_ok(stdin_ok)
> > +{
> > + agent.LoadPlugins(PLUGINSDIR);
> > +
> > + // TODO proper cleanup on exceptions thrown here - wrap in classes?
> > + streamfd = open(stream_port.c_str(), O_RDWR);
> > + if (streamfd < 0)
> > + throw std::runtime_error("failed to open the streaming device (" +
> > + stream_port + "): " + strerror(errno));
> > +
> > + if (!log_filename.empty()) {
> > + log_file = fopen(log_filename.c_str(), "wb");
> > + if (!log_file) {
> > + throw std::runtime_error("Failed to open log file '" +
> > log_filename +
> > + "': " + strerror(errno) + "'");
> > + }
> > + }
> > +}
> > +
> > +AgentRunner::~AgentRunner() {
>
> minor: bracket on next line
Indeed. Old habits.
> > + if (streamfd >= 0) {
> > + close(streamfd);
> > + streamfd = -1;
> > + }
> > +
> > + if (log_file) {
> > + fclose(log_file);
> > + log_file = nullptr;
> > + }
> > +}
> > +
> > +int AgentRunner::have_something_to_read(int *pfd, int timeout)
> > {
> > int nfds;
> > struct pollfd pollfds[2] = {
> > @@ -82,7 +215,7 @@ static int have_something_to_read(int *pfd, int timeout)
> > return *pfd != -1;
> > }
> >
> > -static int read_command_from_stdin(void)
> > +int AgentRunner::read_command_from_stdin()
> > {
> > char buffer[64], *p, *save = NULL;
> >
> > @@ -106,7 +239,7 @@ static int read_command_from_stdin(void)
> > return 1;
> > }
> >
> > -static int read_command_from_device(void)
> > +int AgentRunner::read_command_from_device()
> > {
> > StreamDevHeader hdr;
> > uint8_t msg[64];
> > @@ -151,7 +284,7 @@ static int read_command_from_device(void)
> > return 1;
> > }
> >
> > -static int read_command(bool blocking)
> > +int AgentRunner::read_command(bool blocking)
> > {
> > int fd, n=1;
> > int timeout = blocking?-1:0;
> > @@ -173,28 +306,8 @@ static int read_command(bool blocking)
> > return n;
> > }
> >
> > -static size_t
> > -write_all(int fd, const void *buf, const size_t len)
> > -{
> > - size_t written = 0;
> > - while (written < len) {
> > - int l = write(fd, (const char *) buf + written, len - written);
> > - if (l < 0 && errno == EINTR) {
> > - continue;
> > - }
> > - if (l < 0) {
> > - syslog(LOG_ERR, "write failed - %m");
> > - return l;
> > - }
> > - written += l;
> > - }
> > - syslog(LOG_DEBUG, "write_all -- %u bytes written\n", (unsigned)written);
> > - return written;
> > -}
> > -
> > -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> > +int AgentRunner::spice_stream_send_format(unsigned w, unsigned h, unsigned
> > c)
> > {
> > -
> > SpiceStreamFormatMessage msg;
> > const size_t msgsize = sizeof(msg);
> > const size_t hdrsize = sizeof(msg.hdr);
> > @@ -213,7 +326,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> > h, unsigned c)
> > return EXIT_SUCCESS;
> > }
> >
> > -static int spice_stream_send_frame(const void *buf, const unsigned size)
> > +int AgentRunner::spice_stream_send_frame(const void *buf, const unsigned
> > size)
> > {
> > SpiceStreamDataMessage msg;
> > const size_t msgsize = sizeof(msg);
> > @@ -244,7 +357,7 @@ static int spice_stream_send_frame(const void *buf, const
> > unsigned size)
> > }
> >
> > /* returns current time in micro-seconds */
> > -static uint64_t get_time(void)
> > +uint64_t AgentRunner::get_time(void)
> > {
> > struct timeval now;
> >
> > @@ -254,116 +367,13 @@ static uint64_t get_time(void)
> >
> > }
> >
> > -static void handle_interrupt(int intr)
> > -{
> > - syslog(LOG_INFO, "Got signal %d, exiting", intr);
> > - quit = true;
> > -}
> > -
> > -static void register_interrupts(void)
> > -{
> > - struct sigaction sa;
> > -
> > - memset(&sa, 0, sizeof(sa));
> > - sa.sa_handler = handle_interrupt;
> > - if ((sigaction(SIGINT, &sa, NULL) != 0) &&
> > - (sigaction(SIGTERM, &sa, NULL) != 0)) {
> > - syslog(LOG_WARNING, "failed to register signal handler %m");
> > - }
> > -}
> > -
> > -static void usage(const char *progname)
> > -{
> > - printf("usage: %s <options>\n", progname);
> > - printf("options are:\n");
> > - printf("\t-p portname -- virtio-serial port to use\n");
> > - printf("\t-i accept commands from stdin\n");
> > - printf("\t-l file -- log frames to file\n");
> > - printf("\t--log-binary -- log binary frames (following -l)\n");
> > - printf("\t-d -- enable debug logs\n");
> > - printf("\t-c variable=value -- change settings\n");
> > - printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> > - printf("\n");
> > - printf("\t-h or --help -- print this help message\n");
> > -
> > - exit(1);
> > -}
> > -
> > -static void
> > -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> > - std::function<void(uint32_t *)> fill_cursor)
> > -{
> > - if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > - height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > - return;
> > -
> > - size_t cursor_size =
> > - sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > - width * height * sizeof(uint32_t);
> > - std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > -
> > - StreamDevHeader
> > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > - memset(&dev_hdr, 0, sizeof(dev_hdr));
> > - dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > - dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> > - dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> > -
> > - StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet
> > *>(msg.get() + sizeof(StreamDevHeader)));
> > - memset(&cursor_msg, 0, sizeof(cursor_msg));
> > -
> > - cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > - cursor_msg.width = width;
> > - cursor_msg.height = height;
> > - cursor_msg.hot_spot_x = hotspot_x;
> > - cursor_msg.hot_spot_y = hotspot_y;
> > -
> > - uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > - fill_cursor(pixels);
> > -
> > - std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > - write_all(streamfd, msg.get(), cursor_size);
> > -}
> > -
> > -static void cursor_changes(Display *display, int event_base)
> > -{
> > - unsigned long last_serial = 0;
> > -
> > - while (1) {
> > - XEvent event;
> > - XNextEvent(display, &event);
> > - if (event.type != event_base + 1)
> > - continue;
> > -
> > - XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> > - if (!cursor)
> > - continue;
> > -
> > - if (cursor->cursor_serial == last_serial)
> > - continue;
> > -
> > - last_serial = cursor->cursor_serial;
> > - auto fill_cursor = [&](uint32_t *pixels) {
> > - for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > - pixels[i] = cursor->pixels[i];
> > - };
> > - send_cursor(cursor->width, cursor->height, cursor->xhot,
> > cursor->yhot, fill_cursor);
> > - }
> > -}
> > -
> > -static void
> > -do_capture(const string &streamport, FILE *f_log)
> > +void AgentRunner::do_capture()
> > {
> > - streamfd = open(streamport.c_str(), O_RDWR);
> > - if (streamfd < 0)
> > - throw std::runtime_error("failed to open the streaming device (" +
> > - streamport + "): " + strerror(errno));
> > -
> > unsigned int frame_count = 0;
> > while (! quit) {
> > while (!quit && !streaming_requested) {
> > if (read_command(true) < 0) {
> > - syslog(LOG_ERR, "FAILED to read command\n");
> > - goto done;
> > + throw std::runtime_error("FAILED to read command");
> > }
> > }
> >
> > @@ -406,12 +416,12 @@ do_capture(const string &streamport, FILE *f_log)
> > if (spice_stream_send_format(width, height, codec) ==
> > EXIT_FAILURE)
> > throw std::runtime_error("FAILED to send format
> > message");
> > }
> > - if (f_log) {
> > + if (log_file) {
> > if (log_binary) {
> > - fwrite(frame.buffer, frame.buffer_size, 1, f_log);
> > + fwrite(frame.buffer, frame.buffer_size, 1, log_file);
> > } else {
> > - fprintf(f_log, "%lu: Frame of %zu bytes:\n", get_time(),
> > frame.buffer_size);
> > - hexdump(frame.buffer, frame.buffer_size, f_log);
> > + fprintf(log_file, "%lu: Frame of %zu bytes:\n",
> > get_time(), frame.buffer_size);
> > + hexdump(frame.buffer, frame.buffer_size, log_file);
> > }
> > }
> > if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
> > EXIT_FAILURE) {
> > @@ -420,118 +430,24 @@ do_capture(const string &streamport, FILE *f_log)
> > }
> > //usleep(1);
> > if (read_command(false) < 0) {
> > - syslog(LOG_ERR, "FAILED to read command\n");
> > - goto done;
> > + throw std::runtime_error("FAILED to read command");
> > }
> > }
> > }
> > +}
> >
> > -done:
> > - if (streamfd >= 0) {
> > - close(streamfd);
> > - streamfd = -1;
> > +void AgentRunner::add_options(const std::vector<std::pair<std::string,
> > std::string>> &options) {
> > + for (const auto& option : options) {
> > + agent.AddOption(option.first.c_str(), option.second.c_str());
> > }
> > }
> >
> > -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> > -
> > -int main(int argc, char* argv[])
> > +void AgentRunner::run()
> > {
> > - string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> > - char opt;
> > - const char *log_filename = NULL;
> > - int logmask = LOG_UPTO(LOG_WARNING);
> > - struct option long_options[] = {
> > - { "log-binary", no_argument, &log_binary, 1},
> > - { "help", no_argument, NULL, 'h'},
> > - { 0, 0, 0, 0}
> > - };
> > -
> > - if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
> > - stdin_ok = true;
> > - }
> > -
> > - openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) :
> > LOG_PID, LOG_USER);
> > - setlogmask(logmask);
> > -
> > - while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL))
> > != -1) {
> > - switch (opt) {
> > - case 0:
> > - /* Handle long options if needed */
> > - break;
> > - case 'i':
> > - stdin_ok = true;
> > - openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
> > - break;
> > - case 'p':
> > - streamport = optarg;
> > - break;
> > - case 'c': {
> > - char *p = strchr(optarg, '=');
> > - if (p == NULL) {
> > - arg_error("wrong 'c' argument %s\n", optarg);
> > - usage(argv[0]);
> > - }
> > - *p++ = '\0';
> > - agent.AddOption(optarg, p);
> > - break;
> > - }
> > - case 'l':
> > - log_filename = optarg;
> > - break;
> > - case 'd':
> > - logmask = LOG_UPTO(LOG_DEBUG);
> > - setlogmask(logmask);
> > - break;
> > - case 'h':
> > - usage(argv[0]);
> > - break;
> > - }
> > - }
> > -
> > - agent.LoadPlugins(PLUGINSDIR);
> > -
> > - register_interrupts();
> > -
> > - FILE *f_log = NULL;
> > - if (log_filename) {
> > - f_log = fopen(log_filename, "wb");
> > - if (!f_log) {
> > - syslog(LOG_ERR, "Failed to open log file '%s': %s\n",
> > - log_filename, strerror(errno));
> > - return EXIT_FAILURE;
> > - }
> > - }
> > -
> > - Display *display = XOpenDisplay(NULL);
> > - if (display == NULL) {
> > - syslog(LOG_ERR, "failed to open display\n");
> > - return EXIT_FAILURE;
> > - }
> > - int event_base, error_base;
> > - if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> > - syslog(LOG_ERR, "XFixesQueryExtension failed\n");
> > - return EXIT_FAILURE;
> > - }
> > - Window rootwindow = DefaultRootWindow(display);
> > - XFixesSelectCursorInput(display, rootwindow,
> > XFixesDisplayCursorNotifyMask);
> > -
> > - std::thread cursor_th(cursor_changes, display, event_base);
> > + std::thread cursor_th(CursorThread(streamfd, stream_mtx));
> > cursor_th.detach();
> >
> > - int ret = EXIT_SUCCESS;
> > - try {
> > - do_capture(streamport, f_log);
> > - }
> > - catch (std::runtime_error &err) {
> > - syslog(LOG_ERR, "%s\n", err.what());
> > - ret = EXIT_FAILURE;
> > - }
> > -
> > - if (f_log) {
> > - fclose(f_log);
> > - f_log = NULL;
> > - }
> > - closelog();
> > - return ret;
> > + do_capture();
> > }
> > +
> > +}} // namespace spice::streaming_agent
> > diff --git a/src/spice-streaming-agent.hpp b/src/spice-streaming-agent.hpp
> > new file mode 100644
> > index 0000000..1740a02
> > --- /dev/null
> > +++ b/src/spice-streaming-agent.hpp
> > @@ -0,0 +1,57 @@
> > +/* An implementation of a SPICE streaming agent
> > + *
> > + * \copyright
> > + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#ifndef SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
> > +#define SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
> > +
> > +#include "concrete-agent.hpp"
> > +
> > +#include <cstdint>
> > +#include <mutex>
> > +#include <set>
> > +
> > +
> > +namespace spice {
> > +namespace streaming_agent {
> > +
> > +class AgentRunner
> > +{
>
> bracket
The bracket is on a new line, what am I missing?
> > +public:
> > + AgentRunner(const std::string &stream_port,
> > + const std::string &log_filename,
> > + bool log_binary,
> > + bool stdin_ok);
>
> It does not look so extensible, every option require an additional
> argument.
Yes. It is bordering on warranting a config class to wrap the
constructor arguments. I was thinking about it. Such a change is local
and can be done anytime though. I can do it straight away if you wish.
> > + ~AgentRunner();
> > +
> > + void do_capture();
>
> Why this is public?
I've overlooked this one, will make it private.
> What is responsible for this class?
I do not follow here.
> > + void add_options(const std::vector<std::pair<std::string, std::string>>
> > &options);
> > + void run();
> > +
> > + static bool quit;
> > +
> > +private:
> > + int have_something_to_read(int *pfd, int timeout);
> > + int read_command_from_stdin();
> > + int read_command_from_device();
> > + int read_command(bool blocking);
> > + int spice_stream_send_format(unsigned w, unsigned h, unsigned c);
> > + int spice_stream_send_frame(const void *buf, const unsigned size);
> > + uint64_t get_time(void);
>
> why to get the time you need an AgentRunner?
Again, overlooked, I'll make this one into a standalone function in the
.cpp file.
> > +
> > + std::string stream_port;
> > + FILE *log_file = nullptr;
> > + ConcreteAgent agent;
> > + int streaming_requested = 0;
>
> OT: why not bool?
I could ask you this question :) Even though I was aiming for minimal
changes, I thought to change this one, but it is assigned an uint8_t in
the code. I half expected to get a compiler warning for that and didn't
want to think about it at the time.
> > + std::set<SpiceVideoCodecType> client_codecs;
> > + int streamfd = -1;
> > + bool log_binary = false;
> > + bool stdin_ok = false;
> > + std::mutex stream_mtx;
> > +};
> > +
> > +}} // namespace spice::streaming_agent
> > +
> > +#endif // SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
>
> Frediano
More information about the Spice-devel
mailing list