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

[Hans de Goede]

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