[Spice-devel] [PATCH spice 1/5] spicec: Move setting of clipboard_owner to guest to platform code

[Hans de Goede]

Hi,

Thanks for the reviews!

On 10/07/2010 01:51 PM, Arnon Gilboa wrote:
> ack. can't we use the lock for the other 2 win/x11 events as well?

We could, but it is not needed as all the checks for there matching
states are done in red_client.cpp, and thus synchronized to
the main channel loop, rather then the main process loop (where X11
events are handled), so no locking is needed.

Regards,

Hans



>
> Hans de Goede wrote:
>> Atleast under x11 there is a race condition when setting the clipboard
>> owner to guest from the RedClient code rather then doing it in Platform.
>>
>> After the XSetSelectionOwner() in Platform::on_clipboard_grab(), which runs
>> from the main message loop thread, the x11 event thread can receive a
>> SelectionRequest event from another x11 app, before the RedClient code
>> has set the clipboard owner, which will trigger the owner != guest
>> check in the SelectionRequest event handling code.
>>
>> By moving the setting of the owner in to Platform::on_clipboard_grab() it
>> gets protected by the clipboard lock, which closes this tiny race.
>> ---
>> client/platform.h | 2 ++
>> client/red_client.cpp | 1 -
>> client/windows/platform.cpp | 7 +++++++
>> client/x11/platform.cpp | 10 ++++++++--
>> 4 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/client/platform.h b/client/platform.h
>> index af4a0f6..a9a1715 100644
>> --- a/client/platform.h
>> +++ b/client/platform.h
>> @@ -132,6 +132,8 @@ public:
>> static int get_clipboard_owner() { return _clipboard_owner; }
>>
>> private:
>> + static void set_clipboard_owner_unlocked(int new_owner);
>> +
>> static int _clipboard_owner;
>> };
>>
>> diff --git a/client/red_client.cpp b/client/red_client.cpp
>> index 8e7dfe0..0650e35 100644
>> --- a/client/red_client.cpp
>> +++ b/client/red_client.cpp
>> @@ -1118,7 +1118,6 @@ void RedClient::dispatch_agent_message(VDAgentMessage* msg, void* data)
>> case VD_AGENT_CLIPBOARD_GRAB:
>> Platform::on_clipboard_grab((uint32_t *)data,
>> msg->size / sizeof(uint32_t));
>> - Platform::set_clipboard_owner(Platform::owner_guest);
>> break;
>> case VD_AGENT_CLIPBOARD_REQUEST:
>> if (Platform::get_clipboard_owner() != Platform::owner_client) {
>> diff --git a/client/windows/platform.cpp b/client/windows/platform.cpp
>> index 580a40a..075d269 100644
>> --- a/client/windows/platform.cpp
>> +++ b/client/windows/platform.cpp
>> @@ -859,6 +859,11 @@ void WinPlatform::exit_modal_loop()
>>
>> int Platform::_clipboard_owner = Platform::owner_none;
>>
>> +void Platform::set_clipboard_owner_unlocked(int new_owner)
>> +{
>> + set_clipboard_owner(new_owner);
>> +}
>> +
>> void Platform::set_clipboard_owner(int new_owner)
>> {
>> if (new_owner == owner_none) {
>> @@ -885,6 +890,8 @@ bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
>> EmptyClipboard();
>> SetClipboardData(format, NULL);
>> CloseClipboard();
>> +
>> + set_clipboard_owner(owner_guest);
>> return true;
>> }
>>
>> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
>> index 29b5f75..8f0665c 100644
>> --- a/client/x11/platform.cpp
>> +++ b/client/x11/platform.cpp
>> @@ -3355,6 +3355,8 @@ bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
>>
>> XSetSelectionOwner(x_display, clipboard_prop, platform_win, CurrentTime);
>> XFlush(x_display);
>> +
>> + set_clipboard_owner_unlocked(owner_guest);
>> return true;
>> }
>>
>> @@ -3362,12 +3364,16 @@ int Platform::_clipboard_owner = Platform::owner_none;
>>
>> void Platform::set_clipboard_owner(int new_owner)
>> {
>> + Lock lock(clipboard_lock);
>> + set_clipboard_owner_unlocked(new_owner);
>> +}
>> +
>> +void Platform::set_clipboard_owner_unlocked(int new_owner)
>> +{
>> const char * const owner_str[] = { "none", "guest", "client" };
>>
>> /* Clear pending requests and clipboard data */
>> {
>> - Lock lock(clipboard_lock);
>> -
>> if (next_selection_request) {
>> LOG_INFO("selection requests pending upon clipboard owner change, clearing");
>> while (next_selection_request)
>