[Spice-devel] [PATCH 20/22] Move the X11CursorUpdater and X11CursorThread classes in a separate file
Christophe de Dinechin
cdupontd at redhat.com
Thu Mar 1 16:24:03 UTC 2018
> On 1 Mar 2018, at 17:02, Christophe de Dinechin <christophe.de.dinechin at gmail.com> wrote:
>
>
>
>> On 1 Mar 2018, at 16:12, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>>
>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>> From: Christophe de Dinechin <dinechin at 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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list