> On 2 Mar 2018, at 11:37, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> On Thu, 2018-03-01 at 20:01 +0100, Christophe de Dinechin wrote:
>>> On 1 Mar 2018, at 14:56, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>>>
>>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>>
>>>> There are two classes:
>>>> - The X11CursorUpdater class sends the messages. It can be used when
>>>> no thread is required.
>>>> - The X11CursorThread spawns an X11CursorUpdater in a new thread.
>>>>
>>>> These classes will be moved to a separate file in a later patch, but
>>>> are kept in the same file at this stage to make it easier to track
>>>> changes and bisect.
>>>>
>>>> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>> ---
>>>> src/spice-streaming-agent.cpp | 118 +++++++++++++++++++++++++++---------------
>>>> 1 file changed, 76 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index 8bbd457..d1996fd 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -206,6 +206,80 @@ private:
>>>> }
>>>> };
>>>>
>>>> +class X11CursorUpdater
>>>> +{
>>>> +public:
>>>> + X11CursorUpdater(Stream &stream);
>>>> + ~X11CursorUpdater();
>>>> + void send_cursor_changes();
>>>> +
>>>> +private:
>>>> + Stream &stream;
>>>> + Display *display;
>>>> +};
>>>> +
>>>> +class X11CursorThread
>>>> +{
>>>> +public:
>>>> + X11CursorThread(Stream &stream);
>>>> + static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
>>>> +
>>>> +private:
>>>> + X11CursorUpdater updater;
>>>> + std::thread thread;
>>>> +};
>>>
>>> Ok, I was wondering what you had in mind when you said you'll probably
>>> make two classes. What is the X11CursorThread useful for, though? It is
>>> basically a glue, which isn't necessary, if you turn X11CursorUpdater
>>> into a functor? (as per my original "AgentRunner" patch, let me know if
>>> I need to clarify here)
>>
>> It’s to have RAII on the updater. The thread owns the updater.
>
> If you pass a functor to std::thread, it will be copied or moved inside it, so that it will also own it, so from this point of view, it's the same, you get RAII too.
I think I am confused by your suggestion. How do we control the ‘join’ in that case? If we did not join, we terminate(). If we call ‘detach’, we no longer own it.
Here is an example to explain what I mean:
#include <thread>
#include <memory>
#include <iostream>
struct foo
{
foo() { std::cerr << "foo::foo()\n"; }
foo(const foo &o) { std::cerr << "foo::foo(&)\n"; }
foo(const foo &&o) { std::cerr << "foo::foo(&&)\n"; }
~foo() { std::cerr << "foo::~foo()\n"; }
void operator()(char c) {
std::cerr << "operator() with '" << c << "'\n";
for (int i = 0; i < 100; i++)
{
fputc(c, stderr);
std::this_thread::sleep_for(std::chrono::milliseconds(65));
}
std::cerr << "operator() completed\n";
}
};
int main()
{
std::cerr << "Start\n";
foo f;
std::thread t(f, 'f');
for (int i = 0; i < 100; i++)
{
fputc('m', stderr);
std::this_thread::sleep_for(std::chrono::milliseconds(65));
}
}
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel