[Spice-devel] [PATCH spice 13/17] spicec-x11: Add XFlush calls were needed

[Arnon Gilboa]

Hans de Goede wrote:
> Hi,
>
> On 10/04/2010 06:00 PM, Arnon Gilboa wrote:
>> Hans de Goede wrote:
>>> Since we do not always "pump" libX11's event loop by calling
>>> XPending (we only call XPending when there is data to read from the
>>> display fd), we must always explictly flush any outstanding requests.
>>>
>>> This patch adds a whole bunch of missing XFlush calls.
>>> ---
>>> client/x11/platform.cpp | 10 ++++++++++
>>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
>>> index 75b34c6..82660a4 100644
>>> --- a/client/x11/platform.cpp
>>> +++ b/client/x11/platform.cpp
>>> @@ -506,6 +506,7 @@ void Platform::error_beep()
>>> }
>>>
>>> XBell(x_display, 0);
>>> + XFlush(x_display);
>>> }
>>>
>>> void Platform::msleep(unsigned int millisec)
>>> @@ -2028,6 +2029,7 @@ void XMonitor::disable()
>>> for (; iter != _clones.end(); iter++) {
>>> (*iter)->disable();
>>> }
>>> + XFlush(x_display);
>>> X_DEBUG_SYNC(display);
>>> }
>>>
>>> @@ -2049,6 +2051,7 @@ void XMonitor::enable()
>>> for (; iter != _clones.end(); iter++) {
>>> (*iter)->enable();
>>> }
>>> + XFlush(x_display);
>>> X_DEBUG_SYNC(display);
>>> }
>>>
>>> @@ -2975,11 +2978,13 @@ static void set_keyboard_led(XLed led, int set)
>>> case X11_CAPS_LOCK_LED:
>>> if (caps_lock_mask) {
>>> XkbLockModifiers(x_display, XkbUseCoreKbd, caps_lock_mask, set ? 
>>> caps_lock_mask : 0);
>>> + XFlush(x_display);
>>> }
>>> return;
>>> case X11_NUM_LOCK_LED:
>>> if (num_lock_mask) {
>>> XkbLockModifiers(x_display, XkbUseCoreKbd, num_lock_mask, set ? 
>>> num_lock_mask : 0);
>>> + XFlush(x_display);
>>> }
>>> return;
>>> case X11_SCROLL_LOCK_LED:
>>> @@ -2987,6 +2992,7 @@ static void set_keyboard_led(XLed led, int set)
>>> keyboard_control.led_mode = set ? LedModeOn : LedModeOff;
>>> keyboard_control.led = led;
>>> XChangeKeyboardControl(x_display, KBLed | KBLedMode, 
>>> &keyboard_control);
>>> + XFlush(x_display);
>>> return;
>>> }
>>> }
>>> @@ -3034,6 +3040,7 @@ void Platform::reset_cursor_pos()
>>> SpicePoint size = primary_monitor->get_size();
>>> Window root_window = RootWindow(x_display, DefaultScreen(x_display));
>>> XWarpPointer(x_display, None, root_window, 0, 0, 0, 0, pos.x + 
>>> size.x / 2, pos.y + size.y / 2);
>>> + XFlush(x_display);
>>> }
>>>
>>> WaveRecordAbstract* Platform::create_recorder(RecordClient& client,
>>> @@ -3080,6 +3087,7 @@ void XBaseLocalCursor::set(Window window)
>>> {
>>> if (_handle) {
>>> XDefineCursor(x_display, window, _handle);
>>> + XFlush(x_display);
>>> }
>>> }
>>>
>>> @@ -3258,6 +3266,7 @@ bool Platform::on_clipboard_grab(uint32_t 
>>> *types, uint32_t type_count)
>>> return false;
>>> }
>>> XSetSelectionOwner(x_display, clipboard_prop, platform_win, 
>>> CurrentTime);
>>> + XFlush(x_display);
>>> return true;
>>> }
>>>
>>> @@ -3337,6 +3346,7 @@ bool Platform::on_clipboard_request(uint32_t 
>>> type)
>>> }
>>> clipboard_request_target = format;
>>> XConvertSelection(x_display, clipboard_prop, format, clipboard_prop, 
>>> platform_win, CurrentTime);
>>> + XFlush(x_display);
>>> return true;
>>> }
>>>
>> I did not understand this one :(
>
> No problem, let me try and explain.
>
>> are we now syncing all x11 calls?
>
> No not syncing, there is a XSync call and an XFlush call,
> which are different. XFlush ensures that any outstanding requests
> are written to the server.
>
> Normal libX11 programs don't need to call XFlush all that often
> as they call XPending (or related functions) which do an XFlush
> internally (as it wants to make sure any requests causing events
> are written to the server).
>
> But we do not call XPending unless there is data to read from
> the server (we poll on the socket fd). So if we do not
> explicitly flush, requests may be left in the buffer until
> some unrelated event gets reported to us, rather then be
> written to the server immediately.
>
> > is it the right way to do it?
>
> I believe so :) I had issues with this in the agent which also
> does a select on the server fd for read (as it needs to monitor
> the fd for vdagent messages as well). I had a case where I would
> not get an answer to a XConvertSelection, as I did not flush it.
>
> And then much later got the answer when some other X communications
> happened as well.
>
> Basically all the XFlush does is write any requests buffered in
> client side ram to the socket, it does not wait for them to
> actually be processed or anything (as XSync does).
>
> Regards,
>
> Hans
Now that I better understand,
Ack