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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 1 Mar 2018, at 17:02, Christophe de Dinechin <christophe.de.dinechin@xxxxxxxxx> wrote:
> 
> 
> 
>> On 1 Mar 2018, at 16:12, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>> 
>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>> 
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]