[Spice-devel] [RFC PATCH 1/1] separate and encapsulate the agent business code
Lukáš Hrázký
lhrazky at redhat.com
Thu Feb 15 15:31:32 UTC 2018
On Thu, 2018-02-15 at 13:03 +0100, Christophe de Dinechin wrote:
> > On 15 Feb 2018, at 11:51, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >
> > 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?
>
> The same is true with your mega-patch and all my current branches ;-) And I assume this is true of others pending branches as well. So it really goes both ways.
>
> So I’m sorry, while I appreciate the direction you want to go, as written earlier, I think it needs to be broken up in smaller individual refactoring operations, not simply rebased. My patches are an attempt to show what I’d like to see.
>
> If you find it too cumbersome, we can work together on this.
I've realized that since. The patch was a first crude attempt, I'll try
to do better in the future, I promise ;)
> >
> > > > 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?
>
> Please read section 2 and 3.8 of https://www.usenix.org/legacy/events/wiess2000/full_papers/dinechin/dinechin.pdf (this exception handling model is what is now used by all modern C++ compilers, including GCC and clang).
As a side note, I find it slightly irritating that you keep referencing
your own publications :P
I did read the suggested sections and while it was very educational,
the takeaway is that using exceptions complicates compiler
optimizations and therefore has overhead for the code in the try block
even when no exception is thrown. There wasn't anything specific to
using exceptions in constructors.
> One of the issues is that it makes it hard to reason about what is going on, since you find yourself having to think about “half-constructed” objects all the time. And since these are rarely exercised code paths, nobody gets them right. The existing code is simply not in good enough shape at the moment.
I find it hard to follow what exactly you are seeing as the problem. If
I'm reading this right, you mean that if ever you want to throw
exceptions from a constructor, you need to realize you are leaving
behind a half-initialized object for which the destructor won't be
called. And therefore you suggest avoiding to throw exceptions from
constructors?
I find this reasoning flawed. There are times when you can't avoid
having an exception thrown in the constructor and therefore you should
know what you are doing. Just one of the so many pitfalls of C++ and
another reason why we all love it so :) And when you know what you are
doing, there is no problem in throwing exceptions from constructors.
It is also core to RAII, since resource acquisition naturally can fail
and the mechanism to handle that in C++ are exceptions. So you
basically can't use RAII without them.
> (and I’m not against throwing from constructors entirely, my own RAII patch has a throw from a constructor)
So maybe if you can pinpoint precisely what is the problem here (if
there really is one?), I would understand your point...
> > 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.
>
> We don’t have RAII for the moment, which is why I’d rather stay away of throwing in a ctor if that can be avoided. Which is why I quickly rebased one of my RAII patches yesterday so that we start with that instead of the other way round.
But if we know what we are doing (and I'm sure between the two of us at
least you certainly do :D), we could risk it? :D
> > 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.
>
> As the paper above may hint, I am well aware of that ;-)
>
> >
> > 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.
>
> That’s precisely the point.
Sure, we should address 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 ;-)
> >
> > 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…
>
> Not *that* old and unattended.
Well, 3 months is very old for me, though maybe it's your workflow here
that I'm still not entirely used to. It's older than the time I've been
on the team ;) And I have never received your patches in the mail, and
I didn't go dig for them in the history (that would be somewhat tedious
I think). So I don't really know what patches you have there...
> > If you don't have to, please don't create more patches
> > over mine :)
>
> Well, if you can avoid making mega-patches, that will make my “attending” my “old” patches much easier, thank you very much ;-)
I'm not sure it would, actually? Regardless of how much I split them,
it's the amount of changes that will make it hard for you, so one
megapatch or a thousand small ones does not make a difference?
But I will split as much as I can ;)
> Note that when you talked about an “agent runner”, I did not realize that this was largely another take at https://patchwork.freedesktop.org/patch/187689. Also, as I pointed out in my review, I have not yet understood what abstraction a “Runner” is, and you have yet to explain it to me (see below). I know what a stream is. I know how to extract the cursor stuff. Once we have done all that, we can see if there is still a need for a “Runner”.
Yes, that's it only partially, but it's a part that I now plan to add
as a separate patch. Since your patch is outdated now, I think I'll
have my take on it, unless you insist I pick up yours? :)
> >
> > And I am very glad to work on this together with you... Lets just
> > serialize our work if we can to prevent the conflicts? :)
>
> I am strongly suggesting we start with my patches (some of which being, as you pointed out, “old”, and therefore taking precedence) and then add whatever remains to be done on top of this, instead of redoing a parallel patch that touches all the same functions, and then complaining when I try to help you split it in smaller chunks ;-)
You did not help to split my patch, you went on to fix other things! :P
Which I agree with, only that the parallel work creates conflicts.
Well, if you're convinced your old patches are better than mine :D :D,
please send a list of what I should pick up and how to actually go
about it... This suggestion to start with your patches makes it really
hard for me, they are outdated, I'd have to go dig for them and they
are not entirely matching my ideas...
> >
> > > >
> > > > > > 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" :)
>
> All I’m saying is that the recommendation you pointed out is contradicted by what autotools actually generate.
>
> >
> > > >
> > > > >
> > > > > > + 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.
>
> But at the point I’m reading operator(), I don’t know yet it’s going to be used in a thread.
>
> So all I’m asking is a comment like:
>
> // Thread entry point for std::thread created in ...
>
> The comment is needed because operator() can be used for so many more things…
>
> > 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.
>
> No problem.
>
> >
> > > >
> > > > > > {
> > > > > > - 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…
>
> I’m sorry to insist. When I write “I still don’t get it”, it’s not to annoy you. Here is the cover letter:
>
> > Create an AgentRunner (TODO: needs a better name)
>
> Bad start: if the name is wrong, it may mean that the abstraction represented by the class is not clear. In my own “parallel” patch, this was a stream. Like you, I tried to minimize code changes at first, but it backfired and Frediano complained that as a result, the abstraction is not correct (he complained again today on the same topic for my revived Stream RAII patch).
>
> > class to encapsulate
> > the streaming and communication with the server.
>
> So this is the definition of what the class does. When I myself look at that communication, the first order of business I see is that there is no encapsulation of the *messages*. We have three kinds of message (format, data, cursor), which are presently dealt with in three completely different ways (in spice_stream_send_format, spice_stream_send_frame and send_cursor respectively). In one case we statically allocate and write all in one go. In the second case we statically allocate only teh header and send the payload with a second call to write_all. In the cursor case we dynamically allocate the whole thing and and copy / reformat pixel data in the copy.
>
> Maybe if we want to encapsulate things, this is where we need to start, don’t you think? Like abstracting header computations so that we don’t have puzzling comments along the same computations that make you scratch your head, like:
>
> msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
> msg.hdr.size = size; /* includes only the body? */
> dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
>
> Now, the proposed patch does not encapsulate that communication. It reorganizes the code and does other stuff, but I see no encapsulation whatsoever.
It encapsulates the whole agent business code from the setup in
main.cpp :)
But really, I see your point, I already said it was unfinished patch
several times, I don't think we need to discuss further. I agree with
you.
I also think that we are each approaching this from different ends. You
go from bottom to top and I go from top to bottom. Both are valid
approaches. (see you in the middle! :))
> > 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
>
> That part, for example, I sort of understand. “CursorThread” is an abstraction that makes sense to me, I see how doing what you did moves us closer to being able to isolate it on the side.
>
> > - 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.
>
>
> Each of these lines should be a separate patch. If they were, you would not complain so much ;-)
Let's see what I can manage :)
>
> >
> > > >
> > > > > > +{
> > > > > > +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 :)
>
> Life 2.0 as we know it.
That, and the amount of ML discussions :)
Cheers,
Lukas
> >
> > 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