Hi Lukas,
You asked for it, so here is my TON (TON stands for TON of nits). Except for what’s just below this, nothing major, just sharing ideas.
Top-comment: since your objective is to “change as little as possible”, maybe it would be possible to start with a patch doing just a file rename? And then change the renamed file? In many tools, that would make it easier to see what you changed and what you inherited.
The commit log does not explain what an AgentRunner is, why it’s a necessary abstraction, and what this is supposed to be helping with. As I see it:
- It does not cleanly encapsulate resources nor enforces / provides RAII for things like streamfd and the like.
- Because we throw hapazardly here and there and because of the previous comment, it actually makes the situation worse in case of error (at the very least, by making the flow of control harder to follow, but I suspect by ending in “terminate()” more often than not).
Also, a number of the changes could have their own individual patch set, which would make it easier to review and understand later.
> On 8 Feb 2018, at 17:20, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> Create an AgentRunner (TODO: needs a better name) class to encapsulate
> the streaming and communication with the server. The basic setup (cmd
> 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@xxxxxxxxxx>
> ---
> src/Makefile.am | 2 +
> src/main.cpp | 127 ++++++++++++
> src/spice-streaming-agent.cpp | 455 +++++++++++++++++-------------------------
> src/spice-streaming-agent.hpp | 56 ++++++
> 4 files changed, 370 insertions(+), 270 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 8d5c5bd..3a6fee7 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 \
> @@ -56,4 +57,5 @@ spice_streaming_agent_SOURCES = \
> mjpeg-fallback.cpp \
> jpeg.cpp \
> jpeg.hpp \
> + main.cpp \
> $(NULL)
> diff --git a/src/main.cpp b/src/main.cpp
> new file mode 100644
> index 0000000..a309011
> --- /dev/null
> +++ b/src/main.cpp
> @@ -0,0 +1,127 @@
> +/* An implementation of a SPICE streaming agent
> + *
> + * \copyright
> + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <config.h>
I’d write “config.h”. No reason to ever look config.h in system headers.
> +#include "spice-streaming-agent.hpp"
> +
> +#include <string.h>
> +#include <getopt.h>
> +#include <unistd.h>
> +#include <syslog.h>
> +#include <signal.h>
> +
> +using namespace std;
> +using namespace SpiceStreamingAgent;
Inherited? I thought we had decided not to use that.
> +
> +
> +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);
> +}
As you know, one of my patches splits that into separate sections. Might be worth doing in a subsequent patch.
> +
> +void handle_interrupt(int intr)
> +{
> + syslog(LOG_INFO, "Got signal %d, exiting", intr);
> + AgentRunner::quit = true;
nit: since it’s a state and not an action, I’d call it “quit_requested"
> +}
> +
> +void register_interrupts(void)
> +{
> + struct sigaction sa;
> +
> + memset(&sa, 0, sizeof(sa));
Every time I see that, I tell Frediano to use C++-style init. Compilers are good at getting rid of memset calls, but still, it’s much shorter:
struct sigaction sa = { 0 };
> + sa.sa_handler = handle_interrupt;
or better yet, with that stuffed in the init
> + 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[])
> +{
> + string stream_port = "/dev/virtio-ports/com.redhat.stream.0”;
1/ Should be const char * :-)
2/ Should be configurable someday.
OT: I always found having “redhat” in the name a bit offensive to other distros :-) Any reason this was not named “com.spice.stream.0”?
> + char opt;
> + string log_filename;
acceptable use of string ;-)
> + int log_binary = 0;
bool?
> + 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]);
add comment that usage() had better exit, or a return as a precaution?
> + }
> + *p++ = '\0’;
Since you are building strings, the C++ way would be to make a string from optarg with p-optarg chars.
> + 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 {
> + 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;
I would rather set a variable with the return code, and return at end of function, e.g. call closelog()
> + }
> +
> + closelog();
> + return EXIT_SUCCESS;
> +}
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
I’d like file names and class names to be related.
For example, spice-streaming-agent.cpp could be for a spice::streaming::Agent class.
> index 94d9d25..8f97489 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -1,44 +1,38 @@
> /* 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>
“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”
Header reorg would make a nice separate patch
>
> using namespace std;
> using namespace SpiceStreamingAgent;
>
> -static ConcreteAgent agent;
> +
> +bool AgentRunner::quit = false;
> +
> +namespace {
>
> struct SpiceStreamFormatMessage
> {
> @@ -52,15 +46,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) {
why test twice for l < 0?
> + 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 have_something_to_read(int *pfd, int timeout)
> +class CursorThread {
Any reason this is in this file?
> +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);
> + }
> +
> + void operator()()
Why make it a functor? (I know the answer, I’m suggesting a comment ;-)
> + {
> + 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) {
The lambda is fancy. But mixed with decade-old X11 junk… Not really to my taste. In particular if it’s to implement *one* “customization” of a *private* function that calls it exactly *once*. Really, just pass the cursor pointer, you’ll make the job of the compiler much easier. It also makes not much sense to decouple the “pixel copy” logic from the rest of the low-level crap like hot spots. If you really want a customization, it would have to deal with the format of the hot spot, etc. Which, oh, just happen to be in the cursor…
If you decide to keep the lambda, I think you want [=] here, you really want the original cursor pointer. Imagine send_cursor becomes some thread, then a loop could cause your lambda to be evaluated with a cursor fetched at the next iteration. Or better yet, [cursor].
> + 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]);
Sigh. I’m starting to _really_ regret malloc()…
> +
> + StreamDevHeader &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> + memset(&dev_hdr, 0, sizeof(dev_hdr));
memset? Super yucky. All that stuff should be three dozen of layers of encapsulation below what we are doing here.
What about replacing these lines with a placement new:
StreamDevHeader &dev_hdr = *new(msg.get()) StreamDevHeader {
.protocol_version = STREAM_DEVICE_PROTOCOL,
.type = STREAM_TYPE_CURSOR_SET,
.size = sizeof(StreamDevHeader)
};
> + 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)));
Combining a reinterpret_cast with low-level uint8_t pointer arithmetic. That’s a winner.
Same recipe as above, but use array indexing to better express intent:
StreamMsgCursorSet &cursor_msg = *new(&dev_hdr[1]) StreamMsgCursorSet {
… fields
};
Also avoids a lot of repetition, and makes it easier for the compiler to initialize in place.
> +
> + 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);
Just pass cursor and init here. Or abstract everything, including hot spot.
> +
> + std::lock_guard<std::mutex> stream_guard(stream_mtx);
> + write_all(streamfd, msg.get(), cursor_size);
Why shouldn’t the locking happen within write_all? As is, it’s practically screaming “please please please make sure someone else calls write_all without the mutex”
> + }
> +
> + Display* display;
= NULL, since you init the rest.
> + int event_base = 0;
> + int streamfd = -1;
> + std::mutex &stream_mtx;
A global mutex for a global function is one case where I would use a global variable. At least until we have made further cleanup.
> +};
> +
> +} // namespace
??? Why are we closing namespaces in the middle?
> +
> +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);
I personally don’t like having things that are likely to fail in a ctor. Cleaning up when a dtor throws an exception is messy. Without going into details, at first sight, I believe it’s not done right here.
> +
> + // TODO proper cleanup on exceptions thrown here - wrap in classes?
Well, obviously, if you go through all this trouble and still don’t have RAII on the stream, it’s annoying ;-)
> + 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() {
> + if (streamfd >= 0) {
That if is a clear sign something is wrong in the design.
> + close(streamfd);
> + streamfd = -1;
Just in case you call the destructor twice. Better safe than sorry. Another sign something is wrong here.
> + }
> +
> + 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 +216,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;
Ugly init and mixed declarators.
>
> @@ -106,7 +240,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 +285,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;
Ugly init.
> int timeout = blocking?-1:0;
> @@ -173,28 +307,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 +327,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 +358,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 +368,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)
> +void AgentRunner::do_capture()
That really confuses me. What is AgentRunner? Running the agent? The agent itself? Why does it have a “do_capture”?
> {
> - 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)
> -{
> - 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 +417,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 +431,22 @@ 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();
> }
> diff --git a/src/spice-streaming-agent.hpp b/src/spice-streaming-agent.hpp
> new file mode 100644
> index 0000000..d6cbfbb
> --- /dev/null
> +++ b/src/spice-streaming-agent.hpp
> @@ -0,0 +1,56 @@
> +/* 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 SpiceStreamingAgent {
> +
> +class AgentRunner
I dislike having the class name differ from the file name. If there is an “AgentRunner”, I expect it to “run” an “agent”. But there is not agent anywhere in your “runner”, and it’s not even clear what it’s running.
> +{
> +public:
> + AgentRunner(const std::string &stream_port,
> + const std::string &log_filename,
> + bool log_binary,
> + bool stdin_ok);
Same comments as Frediano rergarding the ctor.
As I see it, the logger is a resource, I think it deserves its own class. The stream is a resource, it deserves its own class. Option management is a separate thing, also a separate set of classes. And things like the list of codecs or capture belong to the agent itself, I see no reason to defer them here.
In other words, the way I would like this to change is:
- Main creates a “Stream”, a “LogFile” and an “Agent” object
- The cursor class encapsulation is semi-OK, should be split apart in the only place that even remotely knows anything about X11. Just get rid of the overblown lambda, or, if you can prove there will be valid use cases for it, provide a real encapsulation for what a cursor is that includes everything, including hot spot.
- ConcreteAgent is really bad, it should really be AgentInterface and Agent, or IAgent and Agent if you speak Microsoftese).
- Once this is done, I don’t know that there will be much room for a “runner”, although I could be convinced otherwise if it deals with things like threads
Hope that this helps and that you won’t read that as me being overly picky. To my discharge, I’m feeling nauseated, which usually does not improve my mood ;-)
Let’s keep working on this!
Cheers
Christophe
> + ~AgentRunner();
> +
> + void do_capture();
> + 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);
> +
> + std::string stream_port;
> + FILE *log_file = nullptr;
> + ConcreteAgent agent;
> + int streaming_requested = 0;
> + std::set<SpiceVideoCodecType> client_codecs;
> + int streamfd = -1;
> + bool log_binary = false;
> + bool stdin_ok = false;
> + std::mutex stream_mtx;
> +};
> +
> +} // SpiceStreamingAgent
> +
> +#endif // SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
> --
> 2.16.1
>
> _______________________________________________
> 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