On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote: > 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. I did ask, I'm glad for those that are relevant :D thanks. Bonus points for recursive acronym :) > 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. As mentioned in the cover letter, I couldn't figure out proper naming, so I kind of sent it as I had it. Maybe I shouldn't have sent it like this, but I wanted some comments on whether these changes are actually welcome and also for Frediano, in case he liked it, to rebase his daemon patches on. Becase for him, with this patch, calling the agent is like ~3 lines, which is easy to change. I know it's better to make the file rename a separate commit when renaming, but since I'm splitting the code into two parts, I'm not really sure how to do it. Also, splitting this into separate patches is a real challenge and I don't really know how to approach it. This patch as it is was the first step for me, since it only touches the code logic as little as necessary and mostly just moves the code around. That is however not apparent from the patch at all (proven by you pointing out a TON of issues that I only copy-pasted :)) 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 :) 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. > 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 :) > 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. > > 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. 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 > > +#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. Oops, forgot to fix 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. I didn't know :), didn't notice that. Well, now I do :) Still, that is for another 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" Will do. > > +} > > + > > +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 Was in original code. > > + 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 * :-) Was in original code. To be fair, I did it in some previous commit. :D > 2/ Should be configurable someday. That's what I thought as well. Not relevant to this patch... > 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”? This is a very good remark. We really should change that. > > + char opt; > > + string log_filename; > > acceptable use of string ;-) > > > + int log_binary = 0; > > bool? Was in original code. Small enough change, can fix it. > > + 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? Was in original code. I'd return here and do nothing in usage(). I'll fix that. > > + } > > + *p++ = '\0’; > > Since you are building strings, the C++ way would be to make a string from optarg with p-optarg chars. Was in original code. > > + 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() You're right. Will do. > > + } > > + > > + 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. As I said, I did mention this in the cover letter, needs improvement. AFAIK usually directories are used to represent namespaces, at least for public header files. Having all sources in a single directory like src/spice/streaming_agent/ seems unnecessary complication if we only have a single namespace here. Same would be to prefix all the files with the same prefix. > > 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 I was moving some of them elsewhere as integral part of the patch, seemed correct to also reorg them. > > > > 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? Was in original code. Really, don't ask me :) > > + 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? You mean I should split it into a separate file? I agree. > > +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 ;-) What kind of comment do you suggest? I'm genuinely not sure what you mean. > > + { > > + 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]. Was in original code. > > + 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? :) 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. > > { > > - 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. > > +{ > > +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 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 > > 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