On Fri, 2018-03-02 at 22:10 +0100, Christophe de Dinechin wrote:
> > On 2 Mar 2018, at 14:45, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > On Fri, 2018-03-02 at 14:28 +0100, Christophe de Dinechin wrote:
> > > > On 2 Mar 2018, at 11:37, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> > > >
> > > > On Thu, 2018-03-01 at 20:01 +0100, Christophe de Dinechin wrote:
> > > > > > On 1 Mar 2018, at 14:56, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > > > > > From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> > > > > > >
> > > > > > > There are two classes:
> > > > > > > - The X11CursorUpdater class sends the messages. It can be used when
> > > > > > > no thread is required.
> > > > > > > - The X11CursorThread spawns an X11CursorUpdater in a new thread.
> > > > > > >
> > > > > > > These classes will be moved to a separate file in a later patch, but
> > > > > > > are kept in the same file at this stage to make it easier to track
> > > > > > > changes and bisect.
> > > > > > >
> > > > > > > Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > src/spice-streaming-agent.cpp | 118 +++++++++++++++++++++++++++---------------
> > > > > > > 1 file changed, 76 insertions(+), 42 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > > > > > index 8bbd457..d1996fd 100644
> > > > > > > --- a/src/spice-streaming-agent.cpp
> > > > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > > > @@ -206,6 +206,80 @@ private:
> > > > > > > }
> > > > > > > };
> > > > > > >
> > > > > > > +class X11CursorUpdater
> > > > > > > +{
> > > > > > > +public:
> > > > > > > + X11CursorUpdater(Stream &stream);
> > > > > > > + ~X11CursorUpdater();
> > > > > > > + void send_cursor_changes();
> > > > > > > +
> > > > > > > +private:
> > > > > > > + Stream &stream;
> > > > > > > + Display *display;
> > > > > > > +};
> > > > > > > +
> > > > > > > +class X11CursorThread
> > > > > > > +{
> > > > > > > +public:
> > > > > > > + X11CursorThread(Stream &stream);
> > > > > > > + static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
> > > > > > > +
> > > > > > > +private:
> > > > > > > + X11CursorUpdater updater;
> > > > > > > + std::thread thread;
> > > > > > > +};
> > > > > >
> > > > > > Ok, I was wondering what you had in mind when you said you'll probably
> > > > > > make two classes. What is the X11CursorThread useful for, though? It is
> > > > > > basically a glue, which isn't necessary, if you turn X11CursorUpdater
> > > > > > into a functor? (as per my original "AgentRunner" patch, let me know if
> > > > > > I need to clarify here)
> > > > >
> > > > > It’s to have RAII on the updater. The thread owns the updater.
> > > >
> > > > If you pass a functor to std::thread, it will be copied or moved inside it, so that it will also own it, so from this point of view, it's the same, you get RAII too.
> > >
> > > I think I am confused by your suggestion. How do we control the ‘join’ in that case? If we did not join, we terminate(). If we call ‘detach’, we no longer own it.
> > >
> > > Here is an example to explain what I mean:
> > >
> > > #include <thread>
> > > #include <memory>
> > > #include <iostream>
> > >
> > > struct foo
> > > {
> > > foo() { std::cerr << "foo::foo()\n"; }
> > > foo(const foo &o) { std::cerr << "foo::foo(&)\n"; }
> > > foo(const foo &&o) { std::cerr << "foo::foo(&&)\n"; }
> > > ~foo() { std::cerr << "foo::~foo()\n"; }
> > >
> > > void operator()(char c) {
> > > std::cerr << "operator() with '" << c << "'\n";
> > > for (int i = 0; i < 100; i++)
> > > {
> > > fputc(c, stderr);
> > > std::this_thread::sleep_for(std::chrono::milliseconds(65));
> > > }
> > > std::cerr << "operator() completed\n";
> > > }
> > > };
> > >
> > > int main()
> > > {
> > > std::cerr << "Start\n";
> > > foo f;
> > > std::thread t(f, 'f');
> > >
> > > for (int i = 0; i < 100; i++)
> > > {
> > > fputc('m', stderr);
> > > std::this_thread::sleep_for(std::chrono::milliseconds(65));
> > > }
> >
> > t.join();
>
> I put that join in the destructor. If you put it in line like this, it’s not RAII anymore, since an exception in the main thread will not call join.
>
> > > }
> > >
> > > At end, this program terminates. And in ~foo or in operator(), I cannot join, there’s no thread object there. So I’m not sure what you want to do.
> >
> > I've added the join to your program, hope it's clear. I am also not sure how you mean it…
>
> Well, precisely, the whole reason for adding a wrapper class was that this join was in the destructor, so called in case of exception.
>
> I’m not sure if we are talking past one another once more…
I think not, this time :) I am not sure if you mentioned a patch
showing how you put the join() in the destructor somewhere (getting a
bit lost in the growing series, and after the weekend..).
There was no join() in the original series...
So thinking about it now, the wrapper is probably a good idea. One
thing to be aware of (as you well may be), if you join() in the
destructor, you also need to ensure that you signal the thread to quit
if an exception is thrown and your thread wrapper would be destroyed,
otherwise the join() in the destructor would block indefinitely.
The best way to do it in our case is I think set a local quit flag in
the destructor before calling the join(). Then we don't even need any
external signaling and can let the destructor take care of everything,
if I'm not mistaken.
One other thing (slowly realizing all the thread interactions :)), you
need to catch all exceptions in the thread function, throwing an
exception from a thread calls terminate().
> >
> > In this case, neither main thread nor 'foo' have an endless loop, they
> > both end by themselves, so the situation is simple. You only need to
> > ensure you wait in the main thread for the 't' thread to finish.
> >
> > That is, the way you use join is in the main thread to wait for the
> > thread on which you are calling the join, in case that's what's unclear
> > here.
> >
> > I may be missing your point with the terminate(), in case it's still
> > outstanding, can you elaborate?
> >
> > >
> > >
> > > > This also made me realize you should delete the copy constructor of X11CursorUpdater and implement the move constructor.
> > >
> > > Done, although not needing it for the moment, I deleted the move ctor too.
> > >
> > > >
> > > > > >
> > > > > > The only other practical thing I see it doing is wrapping these two
> > > > > > lines into a single call:
> > > > > >
> > > > > > std::thread cursor_th(X11CursorUpdater(...));
> > > > > > cursor_th.detach();
> > > > > >
> > > > > > I think these two calls would be fine in main() below and we could
> > > > > > leave out the glue class...
> > > > > >
> > > > > > (I also have my doubts about the detach, perhaps there should be a
> > > > > > proper join, haven't looked into it yet. Might be causing the Ctrl-C
> > > > > > not working.)
> > > > >
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +X11CursorUpdater::X11CursorUpdater(Stream &stream)
> > > > > > > + : stream(stream),
> > > > > > > + display(XOpenDisplay(NULL))
> > > > > > > +{
> > > > > > > + if (display == NULL) {
> > > > > > > + throw Error("failed to open display").syslog();
> > > > > > > + }
> > > > > > > +}
> > > > > > > +
> > > > > > > +X11CursorUpdater::~X11CursorUpdater()
> > > > > > > +{
> > > > > > > + XCloseDisplay(display);
> > > > > > > +}
> > > > > > > +
> > > > > > > +void X11CursorUpdater::send_cursor_changes()
> > > > > > > +{
> > > > > > > + unsigned long last_serial = 0;
> > > > > > > +
> > > > > > > + int event_base, error_base;
> > > > > > > + if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> > > > > > > + syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor changes\n");
> > > > > > > + return; // also terminates the X11CursorThread if that's how we were launched
> > > > > > > + }
> > > > > > > + Window rootwindow = DefaultRootWindow(display);
> > > > > > > + XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
> > > > > > > +
> > > > > > > + while (true) {
> > > > > > > + XEvent event;
> > > > > > > + XNextEvent(display, &event);
> > > > > > > + if (event.type != event_base + 1) {
> > > > > > > + continue;
> > > > > > > + }
> > > > > > > +
> > > > > > > + XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> > > > > > > + if (!cursor || cursor->cursor_serial == last_serial) {
> > > > > > > + continue;
> > > > > > > + }
> > > > > > > +
> > > > > > > + last_serial = cursor->cursor_serial;
> > > > > > > + stream.send<X11CursorMessage>(cursor);
> > > > > > > + }
> > > > > > > +}
> > > > > > > +
> > > > > > > +X11CursorThread::X11CursorThread(Stream &stream)
> > > > > > > + : updater(stream),
> > > > > > > + thread(record_cursor_changes, this)
> > > > > > > +{
> > > > > > > + thread.detach();
> > > > > > > +}
> > > > > > > +
> > > > > > > +
> > > > > > > }} // namespace spice::streaming_agent
> > > > > > >
> > > > > > > static bool quit_requested = false;
> > > > > > > @@ -377,31 +451,6 @@ static void usage(const char *progname)
> > > > > > > exit(1);
> > > > > > > }
> > > > > > >
> > > > > > > -static void cursor_changes(Stream *stream, Display *display, int event_base)
> > > > > > > -{
> > > > > > > - 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;
> > > > > > > - stream->send<X11CursorMessage>(cursor);
> > > > > > > - }
> > > > > > > -}
> > > > > > > -
> > > > > > > static void
> > > > > > > do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > > > > > > {
> > > > > > > @@ -554,25 +603,10 @@ int main(int argc, char* argv[])
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > - 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);
> > > > > > > -
> > > > > > > - Stream stream(streamport);
> > > > > > > - std::thread cursor_th(cursor_changes, &stream, display, event_base);
> > > > > > > - cursor_th.detach();
> > > > > > > -
> > > > > > > int ret = EXIT_SUCCESS;
> > > > > > > try {
> > > > > > > + Stream stream(streamport);
> > > > > > > + X11CursorThread cursor_thread(stream);
> > > > > > > do_capture(stream, streamport, f_log);
> > > > > > > }
> > > > > > > catch (Error &err) {
> > > > >
> > > > >
> > > >
> > > > _______________________________________________
> > > > 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
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel