[20/22] Move the X11CursorUpdater and X11CursorThread classes in a separate file

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

Details

Message ID 20180228154325.25791-21-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>

This makes it possible to remove X11 dependencies from the main agent file.

Note: it may be unsafe to call XCloseDisplay from the destructor.
Doing some experiments throwing exceptions, I noticed messages such as:

[xcb] Unknown request in queue while dequeuing
[xcb] Most likely this is a multi-threaded client and XInitThreads
      has not been called
[xcb] Aborting, sorry about that.
spice-streaming-agent: xcb_io.c:165: dequeue_pending_request:
Assertion `!xcb_xlib_unknown_req_in_deq' failed.
Aborted (core dumped)

The stack trace looks like this:

    #1  0x00007ffff65cf4da in abort () from /lib64/libc.so.6
    #2  0x00007ffff65c5d67 in __assert_fail_base () from /lib64/libc.so.6
    #3  0x00007ffff65c5e12 in __assert_fail () from /lib64/libc.so.6
    #4  0x00007ffff76b6ecc in dequeue_pending_request () from /lib64/libX11.so.6
    #5  0x00007ffff76b7d20 in _XReply () from /lib64/libX11.so.6
    #6  0x00007ffff76b36cd in XSync () from /lib64/libX11.so.6
    #7  0x00007ffff769494e in XCloseDisplay () from /lib64/libX11.so.6

If we hit this problem in practice, we may need to remove the
XCloseDisplay call from the destructor.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/Makefile.am               |   2 +
 src/message.hpp               |   2 +
 src/spice-streaming-agent.cpp | 121 +-----------------------------------------
 src/x11-cursor.cpp            |  65 +++++++++++++++++++++++
 src/x11-cursor.hpp            |  91 +++++++++++++++++++++++++++++++
 5 files changed, 161 insertions(+), 120 deletions(-)
 create mode 100644 src/x11-cursor.cpp
 create mode 100644 src/x11-cursor.hpp

Patch hide | download patch | download mbox

diff --git a/src/Makefile.am b/src/Makefile.am
index 923a103..4a03e5e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -58,4 +58,6 @@  spice_streaming_agent_SOURCES = \
 	stream.cpp \
 	stream.hpp \
 	errors.cpp \
+	x11-cursor.hpp \
+	x11-cursor.cpp \
 	$(NULL)
diff --git a/src/message.hpp b/src/message.hpp
index 28b3e28..650bd66 100644
--- a/src/message.hpp
+++ b/src/message.hpp
@@ -6,6 +6,8 @@ 
 #ifndef SPICE_STREAMING_AGENT_MESSAGE_HPP
 #define SPICE_STREAMING_AGENT_MESSAGE_HPP
 
+#include "stream.hpp"
+
 #include <spice/stream-device.h>
 
 namespace spice
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index c401a34..2264af2 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -7,6 +7,7 @@ 
 #include "concrete-agent.hpp"
 #include "stream.hpp"
 #include "message.hpp"
+#include "x11-cursor.hpp"
 #include "hexdump.h"
 #include "mjpeg-fallback.hpp"
 
@@ -36,8 +37,6 @@ 
 #include <vector>
 #include <string>
 #include <functional>
-#include <X11/Xlib.h>
-#include <X11/extensions/Xfixes.h>
 
 using namespace spice::streaming_agent;
 
@@ -86,63 +85,6 @@  public:
     }
 };
 
-class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage, STREAM_TYPE_CURSOR_SET>
-{
-public:
-    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
-    static size_t size(XFixesCursorImage *cursor)
-    {
-        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
-    }
-
-    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
-    {
-        StreamMsgCursorSet msg = {
-            .width = cursor->width,
-            .height = cursor->height,
-            .hot_spot_x = cursor->xhot,
-            .hot_spot_y = cursor->yhot,
-            .type = SPICE_CURSOR_TYPE_ALPHA,
-            .padding1 = { },
-            .data = { }
-        };
-
-        size_t pixcount = pixel_count(cursor);
-        size_t pixsize = pixcount * sizeof(uint32_t);
-        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
-        uint32_t *pixbuf = pixels.get();
-        fill_pixels(cursor, pixcount, pixbuf);
-
-        stream.write_all("cursor message", &msg, sizeof(msg));
-        stream.write_all("cursor pixels", pixbuf, pixsize);
-    }
-
-private:
-    static size_t pixel_count(XFixesCursorImage *cursor)
-    {
-        return cursor->width * cursor->height;
-    }
-
-    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
-    {
-        for (unsigned i = 0; i < count; ++i) {
-            pixbuf[i] = cursor->pixels[i];
-        }
-    }
-};
-
-class X11CursorUpdater
-{
-public:
-    X11CursorUpdater(Stream &stream);
-    ~X11CursorUpdater();
-    void send_cursor_changes();
-
-private:
-    Stream &stream;
-    Display *display;
-};
-
 class FrameLog
 {
 public:
@@ -182,67 +124,6 @@  void FrameLog::dump(const void *buffer, size_t length)
     }
 }
 
-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
 
 bool quit_requested = false;
diff --git a/src/x11-cursor.cpp b/src/x11-cursor.cpp
new file mode 100644
index 0000000..6abc258
--- /dev/null
+++ b/src/x11-cursor.cpp
@@ -0,0 +1,65 @@ 
+/* X11 cursor transmission
+ *
+ * \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+#include "x11-cursor.hpp"
+
+#include <syslog.h>
+
+namespace spice
+{
+namespace streaming_agent
+{
+
+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 spic::streaming_agent
diff --git a/src/x11-cursor.hpp b/src/x11-cursor.hpp
new file mode 100644
index 0000000..2038b1d
--- /dev/null
+++ b/src/x11-cursor.hpp
@@ -0,0 +1,91 @@ 
+/* X11 cursor transmission
+ *
+ * \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+#ifndef SPICE_STREAMING_AGENT_X11_CURSOR_HPP
+#define SPICE_STREAMING_AGENT_X11_CURSOR_HPP
+
+#include "message.hpp"
+
+#include <spice-streaming-agent/errors.hpp>
+
+#include <thread>
+#include <X11/Xlib.h>
+#include <X11/extensions/Xfixes.h>
+
+namespace spice {
+namespace streaming_agent {
+
+class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage,
+                                        STREAM_TYPE_CURSOR_SET>
+{
+public:
+    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
+    static size_t size(XFixesCursorImage *cursor)
+    {
+        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
+    }
+
+    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
+    {
+        StreamMsgCursorSet msg = {
+            .width = cursor->width,
+            .height = cursor->height,
+            .hot_spot_x = cursor->xhot,
+            .hot_spot_y = cursor->yhot,
+            .type = SPICE_CURSOR_TYPE_ALPHA,
+            .padding1 = { },
+            .data = { }
+        };
+
+        size_t pixcount = pixel_count(cursor);
+        size_t pixsize = pixcount * sizeof(uint32_t);
+        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
+        uint32_t *pixbuf = pixels.get();
+        fill_pixels(cursor, pixcount, pixbuf);
+
+        stream.write_all("cursor message", &msg, sizeof(msg));
+        stream.write_all("cursor pixels", pixbuf, pixsize);
+    }
+
+private:
+    static size_t pixel_count(XFixesCursorImage *cursor)
+    {
+        return cursor->width * cursor->height;
+    }
+
+    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
+    {
+        for (unsigned i = 0; i < count; ++i) {
+            pixbuf[i] = cursor->pixels[i];
+        }
+    }
+};
+
+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;
+};
+
+}} // namespace spice::streaming_agent
+
+#endif // SPICE_STREAMING_AGENT_X11_CURSOR_HPP

Comments

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> This makes it possible to remove X11 dependencies from the main agent file.
> 
> Note: it may be unsafe to call XCloseDisplay from the destructor.
> Doing some experiments throwing exceptions, I noticed messages such as:
> 
> [xcb] Unknown request in queue while dequeuing
> [xcb] Most likely this is a multi-threaded client and XInitThreads
>       has not been called
> [xcb] Aborting, sorry about that.
> spice-streaming-agent: xcb_io.c:165: dequeue_pending_request:
> Assertion `!xcb_xlib_unknown_req_in_deq' failed.
> Aborted (core dumped)

Hmm, I think it is quite clear. The destructor gets called in the main
thread, meanwhile stuff is happening in the cursor thread. Apparently,
if we wanna do that, we need to either join the thread before
destroying the object (probably a good idea and I think we can do
that), or see if XInitThreads solves the issue (it might introduce some
locking?).

> The stack trace looks like this:
> 
>     #1  0x00007ffff65cf4da in abort () from /lib64/libc.so.6
>     #2  0x00007ffff65c5d67 in __assert_fail_base () from /lib64/libc.so.6
>     #3  0x00007ffff65c5e12 in __assert_fail () from /lib64/libc.so.6
>     #4  0x00007ffff76b6ecc in dequeue_pending_request () from /lib64/libX11.so.6
>     #5  0x00007ffff76b7d20 in _XReply () from /lib64/libX11.so.6
>     #6  0x00007ffff76b36cd in XSync () from /lib64/libX11.so.6
>     #7  0x00007ffff769494e in XCloseDisplay () from /lib64/libX11.so.6
> 
> If we hit this problem in practice, we may need to remove the
> XCloseDisplay call from the destructor.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/Makefile.am               |   2 +
>  src/message.hpp               |   2 +
>  src/spice-streaming-agent.cpp | 121 +-----------------------------------------
>  src/x11-cursor.cpp            |  65 +++++++++++++++++++++++
>  src/x11-cursor.hpp            |  91 +++++++++++++++++++++++++++++++
>  5 files changed, 161 insertions(+), 120 deletions(-)
>  create mode 100644 src/x11-cursor.cpp
>  create mode 100644 src/x11-cursor.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 923a103..4a03e5e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -58,4 +58,6 @@ spice_streaming_agent_SOURCES = \
>  	stream.cpp \
>  	stream.hpp \
>  	errors.cpp \
> +	x11-cursor.hpp \
> +	x11-cursor.cpp \
>  	$(NULL)
> diff --git a/src/message.hpp b/src/message.hpp
> index 28b3e28..650bd66 100644
> --- a/src/message.hpp
> +++ b/src/message.hpp
> @@ -6,6 +6,8 @@
>  #ifndef SPICE_STREAMING_AGENT_MESSAGE_HPP
>  #define SPICE_STREAMING_AGENT_MESSAGE_HPP
>  
> +#include "stream.hpp"
> +
>  #include <spice/stream-device.h>
>  
>  namespace spice
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index c401a34..2264af2 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -7,6 +7,7 @@
>  #include "concrete-agent.hpp"
>  #include "stream.hpp"
>  #include "message.hpp"
> +#include "x11-cursor.hpp"
>  #include "hexdump.h"
>  #include "mjpeg-fallback.hpp"
>  
> @@ -36,8 +37,6 @@
>  #include <vector>
>  #include <string>
>  #include <functional>
> -#include <X11/Xlib.h>
> -#include <X11/extensions/Xfixes.h>
>  
>  using namespace spice::streaming_agent;
>  
> @@ -86,63 +85,6 @@ public:
>      }
>  };
>  
> -class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage, STREAM_TYPE_CURSOR_SET>
> -{
> -public:
> -    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
> -    static size_t size(XFixesCursorImage *cursor)
> -    {
> -        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
> -    }
> -
> -    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
> -    {
> -        StreamMsgCursorSet msg = {
> -            .width = cursor->width,
> -            .height = cursor->height,
> -            .hot_spot_x = cursor->xhot,
> -            .hot_spot_y = cursor->yhot,
> -            .type = SPICE_CURSOR_TYPE_ALPHA,
> -            .padding1 = { },
> -            .data = { }
> -        };
> -
> -        size_t pixcount = pixel_count(cursor);
> -        size_t pixsize = pixcount * sizeof(uint32_t);
> -        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
> -        uint32_t *pixbuf = pixels.get();
> -        fill_pixels(cursor, pixcount, pixbuf);
> -
> -        stream.write_all("cursor message", &msg, sizeof(msg));
> -        stream.write_all("cursor pixels", pixbuf, pixsize);
> -    }
> -
> -private:
> -    static size_t pixel_count(XFixesCursorImage *cursor)
> -    {
> -        return cursor->width * cursor->height;
> -    }
> -
> -    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
> -    {
> -        for (unsigned i = 0; i < count; ++i) {
> -            pixbuf[i] = cursor->pixels[i];
> -        }
> -    }
> -};
> -
> -class X11CursorUpdater
> -{
> -public:
> -    X11CursorUpdater(Stream &stream);
> -    ~X11CursorUpdater();
> -    void send_cursor_changes();
> -
> -private:
> -    Stream &stream;
> -    Display *display;
> -};
> -
>  class FrameLog
>  {
>  public:
> @@ -182,67 +124,6 @@ void FrameLog::dump(const void *buffer, size_t length)
>      }
>  }
>  
> -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
>  
>  bool quit_requested = false;
> diff --git a/src/x11-cursor.cpp b/src/x11-cursor.cpp
> new file mode 100644
> index 0000000..6abc258
> --- /dev/null
> +++ b/src/x11-cursor.cpp
> @@ -0,0 +1,65 @@
> +/* X11 cursor transmission
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#include "x11-cursor.hpp"
> +
> +#include <syslog.h>
> +
> +namespace spice
> +{
> +namespace streaming_agent
> +{
> +
> +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 spic::streaming_agent
> diff --git a/src/x11-cursor.hpp b/src/x11-cursor.hpp
> new file mode 100644
> index 0000000..2038b1d
> --- /dev/null
> +++ b/src/x11-cursor.hpp
> @@ -0,0 +1,91 @@
> +/* X11 cursor transmission
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> +#define SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> +
> +#include "message.hpp"
> +
> +#include <spice-streaming-agent/errors.hpp>
> +
> +#include <thread>
> +#include <X11/Xlib.h>
> +#include <X11/extensions/Xfixes.h>
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage,
> +                                        STREAM_TYPE_CURSOR_SET>
> +{
> +public:
> +    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
> +    static size_t size(XFixesCursorImage *cursor)
> +    {
> +        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
> +    }
> +
> +    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
> +    {
> +        StreamMsgCursorSet msg = {
> +            .width = cursor->width,
> +            .height = cursor->height,
> +            .hot_spot_x = cursor->xhot,
> +            .hot_spot_y = cursor->yhot,
> +            .type = SPICE_CURSOR_TYPE_ALPHA,
> +            .padding1 = { },
> +            .data = { }
> +        };
> +
> +        size_t pixcount = pixel_count(cursor);
> +        size_t pixsize = pixcount * sizeof(uint32_t);
> +        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
> +        uint32_t *pixbuf = pixels.get();
> +        fill_pixels(cursor, pixcount, pixbuf);
> +
> +        stream.write_all("cursor message", &msg, sizeof(msg));
> +        stream.write_all("cursor pixels", pixbuf, pixsize);
> +    }
> +
> +private:
> +    static size_t pixel_count(XFixesCursorImage *cursor)
> +    {
> +        return cursor->width * cursor->height;
> +    }
> +
> +    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
> +    {
> +        for (unsigned i = 0; i < count; ++i) {
> +            pixbuf[i] = cursor->pixels[i];
> +        }
> +    }
> +};
> +
> +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;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> On 1 Mar 2018, at 16:12, 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>
>> 
>> This makes it possible to remove X11 dependencies from the main agent file.
>> 
>> Note: it may be unsafe to call XCloseDisplay from the destructor.
>> Doing some experiments throwing exceptions, I noticed messages such as:
>> 
>> [xcb] Unknown request in queue while dequeuing
>> [xcb] Most likely this is a multi-threaded client and XInitThreads
>>      has not been called
>> [xcb] Aborting, sorry about that.
>> spice-streaming-agent: xcb_io.c:165: dequeue_pending_request:
>> Assertion `!xcb_xlib_unknown_req_in_deq' failed.
>> Aborted (core dumped)
> 
> Hmm, I think it is quite clear. The destructor gets called in the main
> thread, meanwhile stuff is happening in the cursor thread. Apparently,
> if we wanna do that, we need to either join the thread before
> destroying the object (probably a good idea and I think we can do
> that), or see if XInitThreads solves the issue (it might introduce some
> locking?).

Re-reading the code, I think I had just not paid enough attention to the ‘thread.detach()’ call. Why is there? It is causing the problem, because as a result, destroying the thread does not wait for its completion, so the destructor of the updater can be called while it runs.

Frediano, what was the rationale behind the ‘detach()’? I suggest we remove it.

> 
>> The stack trace looks like this:
>> 
>>    #1  0x00007ffff65cf4da in abort () from /lib64/libc.so.6
>>    #2  0x00007ffff65c5d67 in __assert_fail_base () from /lib64/libc.so.6
>>    #3  0x00007ffff65c5e12 in __assert_fail () from /lib64/libc.so.6
>>    #4  0x00007ffff76b6ecc in dequeue_pending_request () from /lib64/libX11.so.6
>>    #5  0x00007ffff76b7d20 in _XReply () from /lib64/libX11.so.6
>>    #6  0x00007ffff76b36cd in XSync () from /lib64/libX11.so.6
>>    #7  0x00007ffff769494e in XCloseDisplay () from /lib64/libX11.so.6
>> 
>> If we hit this problem in practice, we may need to remove the
>> XCloseDisplay call from the destructor.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/Makefile.am               |   2 +
>> src/message.hpp               |   2 +
>> src/spice-streaming-agent.cpp | 121 +-----------------------------------------
>> src/x11-cursor.cpp            |  65 +++++++++++++++++++++++
>> src/x11-cursor.hpp            |  91 +++++++++++++++++++++++++++++++
>> 5 files changed, 161 insertions(+), 120 deletions(-)
>> create mode 100644 src/x11-cursor.cpp
>> create mode 100644 src/x11-cursor.hpp
>> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 923a103..4a03e5e 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -58,4 +58,6 @@ spice_streaming_agent_SOURCES = \
>> 	stream.cpp \
>> 	stream.hpp \
>> 	errors.cpp \
>> +	x11-cursor.hpp \
>> +	x11-cursor.cpp \
>> 	$(NULL)
>> diff --git a/src/message.hpp b/src/message.hpp
>> index 28b3e28..650bd66 100644
>> --- a/src/message.hpp
>> +++ b/src/message.hpp
>> @@ -6,6 +6,8 @@
>> #ifndef SPICE_STREAMING_AGENT_MESSAGE_HPP
>> #define SPICE_STREAMING_AGENT_MESSAGE_HPP
>> 
>> +#include "stream.hpp"
>> +
>> #include <spice/stream-device.h>
>> 
>> namespace spice
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index c401a34..2264af2 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -7,6 +7,7 @@
>> #include "concrete-agent.hpp"
>> #include "stream.hpp"
>> #include "message.hpp"
>> +#include "x11-cursor.hpp"
>> #include "hexdump.h"
>> #include "mjpeg-fallback.hpp"
>> 
>> @@ -36,8 +37,6 @@
>> #include <vector>
>> #include <string>
>> #include <functional>
>> -#include <X11/Xlib.h>
>> -#include <X11/extensions/Xfixes.h>
>> 
>> using namespace spice::streaming_agent;
>> 
>> @@ -86,63 +85,6 @@ public:
>>     }
>> };
>> 
>> -class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage, STREAM_TYPE_CURSOR_SET>
>> -{
>> -public:
>> -    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
>> -    static size_t size(XFixesCursorImage *cursor)
>> -    {
>> -        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
>> -    }
>> -
>> -    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
>> -    {
>> -        StreamMsgCursorSet msg = {
>> -            .width = cursor->width,
>> -            .height = cursor->height,
>> -            .hot_spot_x = cursor->xhot,
>> -            .hot_spot_y = cursor->yhot,
>> -            .type = SPICE_CURSOR_TYPE_ALPHA,
>> -            .padding1 = { },
>> -            .data = { }
>> -        };
>> -
>> -        size_t pixcount = pixel_count(cursor);
>> -        size_t pixsize = pixcount * sizeof(uint32_t);
>> -        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
>> -        uint32_t *pixbuf = pixels.get();
>> -        fill_pixels(cursor, pixcount, pixbuf);
>> -
>> -        stream.write_all("cursor message", &msg, sizeof(msg));
>> -        stream.write_all("cursor pixels", pixbuf, pixsize);
>> -    }
>> -
>> -private:
>> -    static size_t pixel_count(XFixesCursorImage *cursor)
>> -    {
>> -        return cursor->width * cursor->height;
>> -    }
>> -
>> -    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
>> -    {
>> -        for (unsigned i = 0; i < count; ++i) {
>> -            pixbuf[i] = cursor->pixels[i];
>> -        }
>> -    }
>> -};
>> -
>> -class X11CursorUpdater
>> -{
>> -public:
>> -    X11CursorUpdater(Stream &stream);
>> -    ~X11CursorUpdater();
>> -    void send_cursor_changes();
>> -
>> -private:
>> -    Stream &stream;
>> -    Display *display;
>> -};
>> -
>> class FrameLog
>> {
>> public:
>> @@ -182,67 +124,6 @@ void FrameLog::dump(const void *buffer, size_t length)
>>     }
>> }
>> 
>> -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
>> 
>> bool quit_requested = false;
>> diff --git a/src/x11-cursor.cpp b/src/x11-cursor.cpp
>> new file mode 100644
>> index 0000000..6abc258
>> --- /dev/null
>> +++ b/src/x11-cursor.cpp
>> @@ -0,0 +1,65 @@
>> +/* X11 cursor transmission
>> + *
>> + * \copyright
>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>> + */
>> +#include "x11-cursor.hpp"
>> +
>> +#include <syslog.h>
>> +
>> +namespace spice
>> +{
>> +namespace streaming_agent
>> +{
>> +
>> +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 spic::streaming_agent
>> diff --git a/src/x11-cursor.hpp b/src/x11-cursor.hpp
>> new file mode 100644
>> index 0000000..2038b1d
>> --- /dev/null
>> +++ b/src/x11-cursor.hpp
>> @@ -0,0 +1,91 @@
>> +/* X11 cursor transmission
>> + *
>> + * \copyright
>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>> + */
>> +#ifndef SPICE_STREAMING_AGENT_X11_CURSOR_HPP
>> +#define SPICE_STREAMING_AGENT_X11_CURSOR_HPP
>> +
>> +#include "message.hpp"
>> +
>> +#include <spice-streaming-agent/errors.hpp>
>> +
>> +#include <thread>
>> +#include <X11/Xlib.h>
>> +#include <X11/extensions/Xfixes.h>
>> +
>> +namespace spice {
>> +namespace streaming_agent {
>> +
>> +class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage,
>> +                                        STREAM_TYPE_CURSOR_SET>
>> +{
>> +public:
>> +    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
>> +    static size_t size(XFixesCursorImage *cursor)
>> +    {
>> +        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
>> +    }
>> +
>> +    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
>> +    {
>> +        StreamMsgCursorSet msg = {
>> +            .width = cursor->width,
>> +            .height = cursor->height,
>> +            .hot_spot_x = cursor->xhot,
>> +            .hot_spot_y = cursor->yhot,
>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>> +            .padding1 = { },
>> +            .data = { }
>> +        };
>> +
>> +        size_t pixcount = pixel_count(cursor);
>> +        size_t pixsize = pixcount * sizeof(uint32_t);
>> +        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
>> +        uint32_t *pixbuf = pixels.get();
>> +        fill_pixels(cursor, pixcount, pixbuf);
>> +
>> +        stream.write_all("cursor message", &msg, sizeof(msg));
>> +        stream.write_all("cursor pixels", pixbuf, pixsize);
>> +    }
>> +
>> +private:
>> +    static size_t pixel_count(XFixesCursorImage *cursor)
>> +    {
>> +        return cursor->width * cursor->height;
>> +    }
>> +
>> +    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
>> +    {
>> +        for (unsigned i = 0; i < count; ++i) {
>> +            pixbuf[i] = cursor->pixels[i];
>> +        }
>> +    }
>> +};
>> +
>> +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;
>> +};
>> +
>> +}} // namespace spice::streaming_agent
>> +
>> +#endif // SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 1 Mar 2018, at 17:02, Christophe de Dinechin <christophe.de.dinechin@gmail.com> wrote:
> 
> 
> 
>> On 1 Mar 2018, at 16:12, 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>
>>> 
>>> This makes it possible to remove X11 dependencies from the main agent file.
>>> 
>>> Note: it may be unsafe to call XCloseDisplay from the destructor.
>>> Doing some experiments throwing exceptions, I noticed messages such as:
>>> 
>>> [xcb] Unknown request in queue while dequeuing
>>> [xcb] Most likely this is a multi-threaded client and XInitThreads
>>>     has not been called
>>> [xcb] Aborting, sorry about that.
>>> spice-streaming-agent: xcb_io.c:165: dequeue_pending_request:
>>> Assertion `!xcb_xlib_unknown_req_in_deq' failed.
>>> Aborted (core dumped)
>> 
>> Hmm, I think it is quite clear. The destructor gets called in the main
>> thread, meanwhile stuff is happening in the cursor thread. Apparently,
>> if we wanna do that, we need to either join the thread before
>> destroying the object (probably a good idea and I think we can do
>> that), or see if XInitThreads solves the issue (it might introduce some
>> locking?).
> 
> Re-reading the code, I think I had just not paid enough attention to the ‘thread.detach()’ call. Why is there? It is causing the problem, because as a result, destroying the thread does not wait for its completion, so the destructor of the updater can be called while it runs.
> 
> Frediano, what was the rationale behind the ‘detach()’? I suggest we remove it.

Responding to self.

Actually, I will add a patch to the series that addresses this problem. It goes a bit deeper. The cursor thread also needs to terminate on quit_requested. The proposed patch is there: https://gitlab.com/c3d/spice-streaming-agent/commit/07b0e0ea9317fab3867fb29d4367be8d4ad8ba98. Totally untested ATM (following my repeated power outages today, I’m presently doing a RAID array rebuild, and since this array brand can’t rebuild from Linux, I can’t use my VMs for the moment, so doing everything “blind” from my laptop).


> 
>> 
>>> The stack trace looks like this:
>>> 
>>>   #1  0x00007ffff65cf4da in abort () from /lib64/libc.so.6
>>>   #2  0x00007ffff65c5d67 in __assert_fail_base () from /lib64/libc.so.6
>>>   #3  0x00007ffff65c5e12 in __assert_fail () from /lib64/libc.so.6
>>>   #4  0x00007ffff76b6ecc in dequeue_pending_request () from /lib64/libX11.so.6
>>>   #5  0x00007ffff76b7d20 in _XReply () from /lib64/libX11.so.6
>>>   #6  0x00007ffff76b36cd in XSync () from /lib64/libX11.so.6
>>>   #7  0x00007ffff769494e in XCloseDisplay () from /lib64/libX11.so.6
>>> 
>>> If we hit this problem in practice, we may need to remove the
>>> XCloseDisplay call from the destructor.
>>> 
>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>> ---
>>> src/Makefile.am               |   2 +
>>> src/message.hpp               |   2 +
>>> src/spice-streaming-agent.cpp | 121 +-----------------------------------------
>>> src/x11-cursor.cpp            |  65 +++++++++++++++++++++++
>>> src/x11-cursor.hpp            |  91 +++++++++++++++++++++++++++++++
>>> 5 files changed, 161 insertions(+), 120 deletions(-)
>>> create mode 100644 src/x11-cursor.cpp
>>> create mode 100644 src/x11-cursor.hpp
>>> 
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 923a103..4a03e5e 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -58,4 +58,6 @@ spice_streaming_agent_SOURCES = \
>>> 	stream.cpp \
>>> 	stream.hpp \
>>> 	errors.cpp \
>>> +	x11-cursor.hpp \
>>> +	x11-cursor.cpp \
>>> 	$(NULL)
>>> diff --git a/src/message.hpp b/src/message.hpp
>>> index 28b3e28..650bd66 100644
>>> --- a/src/message.hpp
>>> +++ b/src/message.hpp
>>> @@ -6,6 +6,8 @@
>>> #ifndef SPICE_STREAMING_AGENT_MESSAGE_HPP
>>> #define SPICE_STREAMING_AGENT_MESSAGE_HPP
>>> 
>>> +#include "stream.hpp"
>>> +
>>> #include <spice/stream-device.h>
>>> 
>>> namespace spice
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index c401a34..2264af2 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -7,6 +7,7 @@
>>> #include "concrete-agent.hpp"
>>> #include "stream.hpp"
>>> #include "message.hpp"
>>> +#include "x11-cursor.hpp"
>>> #include "hexdump.h"
>>> #include "mjpeg-fallback.hpp"
>>> 
>>> @@ -36,8 +37,6 @@
>>> #include <vector>
>>> #include <string>
>>> #include <functional>
>>> -#include <X11/Xlib.h>
>>> -#include <X11/extensions/Xfixes.h>
>>> 
>>> using namespace spice::streaming_agent;
>>> 
>>> @@ -86,63 +85,6 @@ public:
>>>    }
>>> };
>>> 
>>> -class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage, STREAM_TYPE_CURSOR_SET>
>>> -{
>>> -public:
>>> -    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
>>> -    static size_t size(XFixesCursorImage *cursor)
>>> -    {
>>> -        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
>>> -    }
>>> -
>>> -    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
>>> -    {
>>> -        StreamMsgCursorSet msg = {
>>> -            .width = cursor->width,
>>> -            .height = cursor->height,
>>> -            .hot_spot_x = cursor->xhot,
>>> -            .hot_spot_y = cursor->yhot,
>>> -            .type = SPICE_CURSOR_TYPE_ALPHA,
>>> -            .padding1 = { },
>>> -            .data = { }
>>> -        };
>>> -
>>> -        size_t pixcount = pixel_count(cursor);
>>> -        size_t pixsize = pixcount * sizeof(uint32_t);
>>> -        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
>>> -        uint32_t *pixbuf = pixels.get();
>>> -        fill_pixels(cursor, pixcount, pixbuf);
>>> -
>>> -        stream.write_all("cursor message", &msg, sizeof(msg));
>>> -        stream.write_all("cursor pixels", pixbuf, pixsize);
>>> -    }
>>> -
>>> -private:
>>> -    static size_t pixel_count(XFixesCursorImage *cursor)
>>> -    {
>>> -        return cursor->width * cursor->height;
>>> -    }
>>> -
>>> -    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
>>> -    {
>>> -        for (unsigned i = 0; i < count; ++i) {
>>> -            pixbuf[i] = cursor->pixels[i];
>>> -        }
>>> -    }
>>> -};
>>> -
>>> -class X11CursorUpdater
>>> -{
>>> -public:
>>> -    X11CursorUpdater(Stream &stream);
>>> -    ~X11CursorUpdater();
>>> -    void send_cursor_changes();
>>> -
>>> -private:
>>> -    Stream &stream;
>>> -    Display *display;
>>> -};
>>> -
>>> class FrameLog
>>> {
>>> public:
>>> @@ -182,67 +124,6 @@ void FrameLog::dump(const void *buffer, size_t length)
>>>    }
>>> }
>>> 
>>> -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
>>> 
>>> bool quit_requested = false;
>>> diff --git a/src/x11-cursor.cpp b/src/x11-cursor.cpp
>>> new file mode 100644
>>> index 0000000..6abc258
>>> --- /dev/null
>>> +++ b/src/x11-cursor.cpp
>>> @@ -0,0 +1,65 @@
>>> +/* X11 cursor transmission
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +#include "x11-cursor.hpp"
>>> +
>>> +#include <syslog.h>
>>> +
>>> +namespace spice
>>> +{
>>> +namespace streaming_agent
>>> +{
>>> +
>>> +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 spic::streaming_agent
>>> diff --git a/src/x11-cursor.hpp b/src/x11-cursor.hpp
>>> new file mode 100644
>>> index 0000000..2038b1d
>>> --- /dev/null
>>> +++ b/src/x11-cursor.hpp
>>> @@ -0,0 +1,91 @@
>>> +/* X11 cursor transmission
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +#ifndef SPICE_STREAMING_AGENT_X11_CURSOR_HPP
>>> +#define SPICE_STREAMING_AGENT_X11_CURSOR_HPP
>>> +
>>> +#include "message.hpp"
>>> +
>>> +#include <spice-streaming-agent/errors.hpp>
>>> +
>>> +#include <thread>
>>> +#include <X11/Xlib.h>
>>> +#include <X11/extensions/Xfixes.h>
>>> +
>>> +namespace spice {
>>> +namespace streaming_agent {
>>> +
>>> +class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage,
>>> +                                        STREAM_TYPE_CURSOR_SET>
>>> +{
>>> +public:
>>> +    X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
>>> +    static size_t size(XFixesCursorImage *cursor)
>>> +    {
>>> +        return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
>>> +    }
>>> +
>>> +    void write_message_body(Stream &stream, XFixesCursorImage *cursor)
>>> +    {
>>> +        StreamMsgCursorSet msg = {
>>> +            .width = cursor->width,
>>> +            .height = cursor->height,
>>> +            .hot_spot_x = cursor->xhot,
>>> +            .hot_spot_y = cursor->yhot,
>>> +            .type = SPICE_CURSOR_TYPE_ALPHA,
>>> +            .padding1 = { },
>>> +            .data = { }
>>> +        };
>>> +
>>> +        size_t pixcount = pixel_count(cursor);
>>> +        size_t pixsize = pixcount * sizeof(uint32_t);
>>> +        std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
>>> +        uint32_t *pixbuf = pixels.get();
>>> +        fill_pixels(cursor, pixcount, pixbuf);
>>> +
>>> +        stream.write_all("cursor message", &msg, sizeof(msg));
>>> +        stream.write_all("cursor pixels", pixbuf, pixsize);
>>> +    }
>>> +
>>> +private:
>>> +    static size_t pixel_count(XFixesCursorImage *cursor)
>>> +    {
>>> +        return cursor->width * cursor->height;
>>> +    }
>>> +
>>> +    void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
>>> +    {
>>> +        for (unsigned i = 0; i < count; ++i) {
>>> +            pixbuf[i] = cursor->pixels[i];
>>> +        }
>>> +    }
>>> +};
>>> +
>>> +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;
>>> +};
>>> +
>>> +}} // namespace spice::streaming_agent
>>> +
>>> +#endif // SPICE_STREAMING_AGENT_X11_CURSOR_HPP
>> _______________________________________________
>> 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