[Spice-devel] [RFC PATCH 1/1] separate and encapsulate the agent business code
Lukáš Hrázký
lhrazky at redhat.com
Thu Feb 15 10:51:02 UTC 2018
On Wed, 2018-02-14 at 22:13 +0100, Christophe de Dinechin wrote:
> I cut quite a bit for clarity. What I cut I agree with ;-)
>
>
> > On 14 Feb 2018, at 14:45, Lukáš Hrázký <lhrazky at redhat.com> 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.
Ok.. I appreciate the patches and I understand your intent to fix
things... but at the moment it adds more work for me to rebase my stuff
and as small as your patches are, there's quite a number of them,
surely creating conflicts for me I'll have to solve. It's not huge, but
since there is no need to send those patches right now, seems to me the
work could have been avoided?
> > 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™
Why don't you want to throw exceptions in a constructor? I find that
completely normal. Initialization can fail. You just need to guarantee,
that for any initialization that you do in a constructor that also
requires some teardown, the teardown gets called even if you get an
exception thrown in the middle of the constructor. The language
guarantees that for any objects that get fully initialized, their
destructor gets called. That's why we want to use RAII where possible
and that's why there's the TODO comment to fix that.
For the agent.LoadPlugins(), though, I don't see a problem currently,
as it is the first call in the constructor and no other initialization
(aside from member constructors) was done.
How to handle plugin exceptions is another question, I would think we
want to catch each plugin's constructor exception and log it, then keep
going with the other plugins. So that one bad plugin doesn't stop the
agent from working.
> >
> > > 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 ;-)
Well, a bunch of small patches is certainly better, but doesn't spare
too much work when rebasing. I would say the best would be not to
create more patches for the same code in parallel. I was reluctant to
do it with there being your outstanding patches, but they are old and
unattended... If you don't have to, please don't create more patches
over mine :)
And I am very glad to work on this together with you... Lets just
serialize our work if we can to prevent the conflicts? :)
> >
> > > > On 8 Feb 2018, at 17:20, Lukáš Hrázký <lhrazky at redhat.com> 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 at redhat.com>
> > > > ---
> > > > 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…
Ok. I didn't even try to fully work this one out... I'm happy to just
use #include "config.h" :)
> >
> > >
> > > > + 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.
For me, using functors with std::thread is normal and natural. I'm
quite sure it was an intended design aspect too. That's why I don't see
why it would deserve a comment... And I don't think putting the thread
inside would be better, this is much more modular. You can run this
functor any way you like. For example you could write some tests and
run it in the main thread, making the tests much simpler.
> >
> > > > + 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.
Yes, I can see why you don't get it, neither really do I :) Sorry again
for posting this unfinished.
> >
> > > > {
> > > > - 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 :-(
In the cover letter, I explained both that this is unfinished work and
that I am unhappy with the name and the not clearly defined bunch of
stuff the class is doing atm...
> >
> > > > +{
> > > > +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”/
Yes! :) As I said, they do clean up the code, but add work for me atm.
Off to write my patches, too many things keep distracting me :)
Thanks,
Lukas
> 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
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
More information about the Spice-devel
mailing list