[14/22] Create classes encapsulating the X11 display cursor capture

Submitted by Christophe de Dinechin on Feb. 28, 2018, 3:43 p.m.

Details

Message ID 20180228154325.25791-15-christophe@dinechin.org
State New
Headers show
Series "streaming-agent: C++ refactoring" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 28, 2018, 3:43 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

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@redhat.com>
---
 src/spice-streaming-agent.cpp | 118 +++++++++++++++++++++++++++---------------
 1 file changed, 76 insertions(+), 42 deletions(-)

Patch hide | download patch | download mbox

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;
+};
+
+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) {

Comments

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> 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@redhat.com>
> ---
>  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)

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.)

> +
> +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) {
> On 1 Mar 2018, at 14:56, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> 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@redhat.com>
>> ---
>> 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.

> 
> 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) {
On Thu, 2018-03-01 at 20:01 +0100, Christophe de Dinechin wrote:
> > On 1 Mar 2018, at 14:56, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > 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@redhat.com>
> > > ---
> > > 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.

This also made me realize you should delete the copy constructor of
X11CursorUpdater and implement the move constructor.

> > 
> > 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) {
> 
>
> On 2 Mar 2018, at 11:37, Lukáš Hrázký <lhrazky@redhat.com> 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@redhat.com> wrote:
>>> 
>>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> 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@redhat.com>
>>>> ---
>>>> 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));
    }
}

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.


> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2018-03-02 at 14:28 +0100, Christophe de Dinechin wrote:
> > On 2 Mar 2018, at 11:37, Lukáš Hrázký <lhrazky@redhat.com> 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@redhat.com> wrote:
> > > > 
> > > > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > 
> > > > > 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@redhat.com>
> > > > > ---
> > > > > 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();
> }
> 
> 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...

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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>
> On 2 Mar 2018, at 14:45, Lukáš Hrázký <lhrazky@redhat.com> 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@redhat.com> 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@redhat.com> wrote:
>>>>> 
>>>>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>>>> 
>>>>>> 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@redhat.com>
>>>>>> ---
>>>>>> 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…

> 
> 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@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Fri, 2018-03-02 at 22:10 +0100, Christophe de Dinechin wrote:
> > On 2 Mar 2018, at 14:45, Lukáš Hrázký <lhrazky@redhat.com> 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@redhat.com> 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@redhat.com> wrote:
> > > > > > 
> > > > > > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > > > 
> > > > > > > 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@redhat.com>
> > > > > > > ---
> > > > > > > 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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > > 
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>