I cut quite a bit for clarity. What I cut I agree with ;-)
> On 14 Feb 2018, at 14:45, Lukáš Hrázký <[email protected]> wrote: > > On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote: >> Hi Lukas, >> > > For your comments which are unrelated to my changes, I commended > uniformly with "Was in original code." as I didn't know what else to do > with them :) I understood that. A lot of what I picked up is review of earlier stuff, some of which was written before I ever joined. It’s more to document later improvements I’d like to see happen, in case you get there first. And then I thought that I’d rather send patches, and I sent a first round. > This way it may be easier to go through them in the future > and fix them. I can't really go off fixing all that now, I'd never > finish this and it would be hard to manage the series while trying to > do separate patches for them. Agreed (and I knew that sending the comments out) > >> 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. > > That is correct, and was my concern too when sending it, as mentioned > in the cover letter :) (in case I could have made something more clear, > please let me know) > > I will surely try to do better :) > >> - 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). > > I don't think we throw haphazardly and don't think it makes errors > harder to diagnose, but I may be wrong. If you had a concrete advice > what to improve, please share it :) Well, at the very least, we have a call to “agent.LoadPlugins” in the constructor, which may throw, because in ConcreteAgent::LoadPlugin we call the init function, and if it throws anything but runtime_error, we pass it along. If memory serves me right, bad_alloc for example does not derive from runtime_error but directly from exception. And the driver is allowed to throw 42 if it wants to. So now we have a potential exception from a plugin within the AgentRunner constructor. That smells funny. Please Don’t Do That™ > >> Also, a number of the changes could have their own individual patch set, >> which would make it easier to review and understand later. > > As I said, not really sure I can do that :( I'll try though. Ideas > welcome. Since I need to rebase several of my patch series, we might work together on getting all this done. But that won’t work with mega patches on either side ;-) > >>> On 8 Feb 2018, at 17:20, Lukáš Hrázký <[email protected]> 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ý <[email protected]> >>> --- >>> 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. > > The reason for the <> is described in [1], 4th paragraph. I've > mentioned it during the previous discussion and didn't get any comment > on it IIRC. Either is fine by me, I don't plan to introduce another > file named 'config.h' anywhere in the source tree. > > [1] > https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html That rationale is remarkably inconsistent with the generated makefiles, which build for example with: -I. -I.. -I.. -I.. -I/usr/include/pixman-1 -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -pthread -I/usr/include/opus -DG_LOG_DOMAIN=\"Spice\" -I/usr/local/include/spice-1 or for the streaming agent: -I. -I.. -DSPICE_STREAMING_AGENT_PROGRAM -I../include -DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" -I/usr/local/include/spice-1 So you -I. first anyway, and you would prefer the local config.h in any case. “config.h” just lets compiler find it without looking up at the command-line options. This is all automake-generated, not in our Makefile.am or autoconf.ac AFAICT. Let me add this to my long list of autotools WTF… > >> >>> + int log_binary = 0; >> >> bool? > > Was in original code. > > Small enough change, can fix it. Sent a patch. >> Header reorg would make a nice separate patch > > I was moving some of them elsewhere as integral part of the patch, > seemed correct to also reorg them. Understood, but you can probably split that as a separate patch. git add —patch helps a lot with this kind of refactoring. > >> >> Any reason this is in this file? > > You mean I should split it into a separate file? I agree. Yes. Have the X11 stuff on the side is a bonus. >>> >> >> Why make it a functor? (I know the answer, I’m suggesting a comment ;-) > > What kind of comment do you suggest? I'm genuinely not sure what you > mean. Because we invoke that operator to start a thread, and it’s the thread interface that requires this to look like a function. An alternative that might be clearer would be to have the thread creation within the class. > >>> + 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]. > > Was in original code. Yes. That probably needs fixing, though, the capture by reference worries me. Subtle bugs ahead. > >>> + 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()… > > Was in original code. > >>> + >>> + 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) >> }; > > Was in original code. > >>> + 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. > > Was in original code. > >>> + >>> + 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. > > Was in original code. > >>> + >>> + 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” > > Was in original code. > > I'll quite surely fix this one though. > >>> + } >>> + >>> + Display* display; >> >> = NULL, since you init the rest. > > Was in original code. > >>> + 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. > > I'll encapsulate the communication layer properly and this should > become a non-issue. >> >>> +}; >>> + >>> +} // namespace >> >> ??? Why are we closing namespaces in the middle? > > It's the unnamed namespace. AFAIK that's how you use it, do suggest a > better way? > >>> + >>> +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. > > Hey.. don't mix stuff here.. Destructors should never throw, right? :) Sorry, that was a typo, I was obviously talking about ctor, not dtor. > Constructors can throw, but then the object is not costructed and the > destructor is not called. So you have to ensure proper cleanup in the > constructor if it's needed. Hence the comment below :) > >>> + >>> + // 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 ;-) > > Right :) > >>> + 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. > > Indeed :) > >>> + } >>> + >>> + 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. > > Was in original code. > >>> >>> @@ -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. > > Was in original code. > >>> 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”? > > It's the original method... Further refactor needed. I understand it is, but I still don’t really get what a “runner” is supposed to be. > >>> { >>> - 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. > > Explained already. ??? Not sure where. If it’s in the cover letter, I still don’t get it :-( > >>> +{ >>> +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 > > That is more or less what I have in mind now. > >> - 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. > > Not sure if I'll be doing anything about the Cursor internals right > now... > >> - ConcreteAgent is really bad, it should really be AgentInterface and Agent, >> or IAgent and Agent if you speak Microsoftese). > > Wholeheartedly agree :) I was having some time to see if I can come up > with a better naming than AgentInterface and Agent, and didn't. No > Microsoftese please! :D > >> - 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 > > That is actually kind of my hope... > >> 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 ;-) > > No worries at all, as I said, I'm grateful for all the relevant > remarks! :P Let’s keep pushing on this. Hoping that today’s patches for the nits help cleaning up in the spirit of “small patches”/ Thanks Christophe > > Thanks! > Lukas > >> 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 >>> [email protected] >>> https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
