From patchwork Mon Dec 10 11:02:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,1/7] gtk-session: prefer early check to agent connectivity From: Victor Toso X-Patchwork-Id: 267293 Message-Id: <20181210110256.20115-2-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:50 +0100 From: Victor Toso In case the agent is disconnected, we we don't need to create the struct RunInfo, GMainLoop and add handlers to some signals. Before this patch, spice_main_channel_clipboard_selection_request() was always called even if agent was not connected, which would log a critical message in agent_clipboard_request() and unnecessary call to spice_channel_wakeup(). This patch also removes one goto and related cleanup label. Changes in v2: - Improved commit log based on a function call that is now avoided: spice_main_channel_clipboard_selection_request (Frediano) Signed-off-by: Victor Toso --- src/spice-gtk-session.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c index 1ccae07..373d11e 100644 --- a/src/spice-gtk-session.c +++ b/src/spice-gtk-session.c @@ -728,6 +728,12 @@ static void clipboard_get(GtkClipboard *clipboard, g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent)); g_return_if_fail(s->main != NULL); + g_object_get(s->main, "agent-connected", &agent_connected, NULL); + if (!agent_connected) { + SPICE_DEBUG("Request to guest failed as agent is not running"); + return; + } + ri.selection_data = selection_data; ri.info = info; ri.loop = g_main_loop_new(NULL, FALSE); @@ -744,13 +750,6 @@ static void clipboard_get(GtkClipboard *clipboard, spice_main_channel_clipboard_selection_request(s->main, selection, atom2agent[info].vdagent); - - g_object_get(s->main, "agent-connected", &agent_connected, NULL); - if (!agent_connected) { - SPICE_DEBUG("canceled clipboard_get, before running loop"); - goto cleanup; - } - /* This is modeled on the implementation of gtk_dialog_run() even though * these thread functions are deprecated and appears to be needed to avoid * dead-lock from gtk_dialog_run(). @@ -761,7 +760,6 @@ static void clipboard_get(GtkClipboard *clipboard, gdk_threads_enter(); G_GNUC_END_IGNORE_DEPRECATIONS -cleanup: g_clear_pointer(&ri.loop, g_main_loop_unref); g_signal_handler_disconnect(s->main, clipboard_handler); g_signal_handler_disconnect(s->main, agent_handler); From patchwork Mon Dec 10 11:02:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,2/7] channel-main: clipboard request: wakeup only when needed From: Victor Toso X-Patchwork-Id: 267294 Message-Id: <20181210110256.20115-3-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:51 +0100 From: Victor Toso This patch makes agent_clipboard_request() to return true only when the message request to the agent is successfully queued to be sent. By checking the return value, we can avoid wakeup the channel unnecessarily at spice_main_channel_clipboard_selection_request() Signed-off-by: Victor Toso --- src/channel-main.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index 4c6bc70..fb7175e 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -1419,23 +1419,24 @@ static void agent_clipboard_notify(SpiceMainChannel *self, guint selection, } /* any context: the message is not flushed immediately, - you can wakeup() the channel coroutine or send_msg_queue() */ -static void agent_clipboard_request(SpiceMainChannel *channel, guint selection, guint32 type) + you can wakeup() the channel coroutine or send_msg_queue() + Returns true if message was queued */ +static bool agent_clipboard_request(SpiceMainChannel *channel, guint selection, guint32 type) { SpiceMainChannelPrivate *c = channel->priv; VDAgentClipboardRequest *request; guint8 *msg; size_t msgsize; - g_return_if_fail(c->agent_connected); - g_return_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)); + g_return_val_if_fail(c->agent_connected, false); + g_return_val_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false); msgsize = sizeof(VDAgentClipboardRequest); if (test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_SELECTION)) { msgsize += 4; } else if (selection != VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD) { SPICE_DEBUG("Ignoring clipboard request"); - return; + return false; } msg = g_alloca(msgsize); @@ -1451,6 +1452,7 @@ static void agent_clipboard_request(SpiceMainChannel *channel, guint selection, request->type = type; agent_msg_queue(channel, VD_AGENT_CLIPBOARD_REQUEST, msgsize, msg); + return true; } /* any context: the message is not flushed immediately, @@ -2936,8 +2938,9 @@ void spice_main_channel_clipboard_selection_request(SpiceMainChannel *channel, g g_return_if_fail(channel != NULL); g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel)); - agent_clipboard_request(channel, selection, type); - spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + if (agent_clipboard_request(channel, selection, type)) { + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + } } /** From patchwork Mon Dec 10 11:02:52 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,3/7] channel-main: clipboard grab: wakeup only when needed From: Victor Toso X-Patchwork-Id: 267295 Message-Id: <20181210110256.20115-4-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:52 +0100 From: Victor Toso This patch makes agent_clipboard_grab() to return true only when the message request to the agent is successfully queued to be sent. By checking the return value, we can avoid wakup the channel unnecessarily at spice_main_channel_clipboard_selection_request() Signed-off-by: Victor Toso --- src/channel-main.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index fb7175e..21afd87 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -1341,8 +1341,9 @@ static void agent_announce_caps(SpiceMainChannel *channel) } /* any context: the message is not flushed immediately, - you can wakeup() the channel coroutine or send_msg_queue() */ -static void agent_clipboard_grab(SpiceMainChannel *channel, guint selection, + you can wakeup() the channel coroutine or send_msg_queue() + Returns true if message was queued. */ +static bool agent_clipboard_grab(SpiceMainChannel *channel, guint selection, guint32 *types, int ntypes) { SpiceMainChannelPrivate *c = channel->priv; @@ -1352,16 +1353,16 @@ static void agent_clipboard_grab(SpiceMainChannel *channel, guint selection, int i; if (!c->agent_connected) - return; + return false; - g_return_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)); + g_return_val_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false); size = sizeof(VDAgentClipboardGrab) + sizeof(uint32_t) * ntypes; if (test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_SELECTION)) { size += 4; } else if (selection != VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD) { CHANNEL_DEBUG(channel, "Ignoring clipboard grab"); - return; + return false; } msg = g_alloca(size); @@ -1379,6 +1380,7 @@ static void agent_clipboard_grab(SpiceMainChannel *channel, guint selection, } agent_msg_queue(channel, VD_AGENT_CLIPBOARD_GRAB, size, msg); + return true; } /* any context: the message is not flushed immediately, @@ -2771,8 +2773,9 @@ void spice_main_channel_clipboard_selection_grab(SpiceMainChannel *channel, guin g_return_if_fail(channel != NULL); g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel)); - agent_clipboard_grab(channel, selection, types, ntypes); - spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + if (agent_clipboard_grab(channel, selection, types, ntypes)) { + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + } } /** From patchwork Mon Dec 10 11:02:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,4/7] channel-main: clipboard release: wakeup only when needed From: Victor Toso X-Patchwork-Id: 267297 Message-Id: <20181210110256.20115-5-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:53 +0100 From: Victor Toso This patch makes agent_clipboard_release() to return true only when the message request to the agent is successfully queued to be sent. By checking the return value, we can avoid wakeup the channel unnecessarily at spice_main_channel_clipboard_selection_release() Signed-off-by: Victor Toso --- src/channel-main.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index 21afd87..5bcd149 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -1458,25 +1458,27 @@ static bool agent_clipboard_request(SpiceMainChannel *channel, guint selection, } /* any context: the message is not flushed immediately, - you can wakeup() the channel coroutine or send_msg_queue() */ -static void agent_clipboard_release(SpiceMainChannel *channel, guint selection) + you can wakeup() the channel coroutine or send_msg_queue() + Returns true if message was queued */ +static bool agent_clipboard_release(SpiceMainChannel *channel, guint selection) { SpiceMainChannelPrivate *c = channel->priv; guint8 msg[4] = { 0, }; guint8 msgsize = 0; - g_return_if_fail(c->agent_connected); - g_return_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)); + g_return_val_if_fail(c->agent_connected, false); + g_return_val_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false); if (test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_SELECTION)) { msg[0] = selection; msgsize += 4; } else if (selection != VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD) { SPICE_DEBUG("Ignoring clipboard release"); - return; + return false; } agent_msg_queue(channel, VD_AGENT_CLIPBOARD_RELEASE, msgsize, msg); + return true; } static gboolean any_display_has_dimensions(SpiceMainChannel *channel) @@ -2828,8 +2830,9 @@ void spice_main_channel_clipboard_selection_release(SpiceMainChannel *channel, g if (!c->agent_connected) return; - agent_clipboard_release(channel, selection); - spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + if (agent_clipboard_release(channel, selection)) { + spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); + } } /** From patchwork Mon Dec 10 11:02:54 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,5/7] channel-main: clipboard release: don't fail silently From: Victor Toso X-Patchwork-Id: 267296 Message-Id: <20181210110256.20115-6-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:54 +0100 From: Victor Toso Spice client should listen to SpiceMainChannel::agent-connected notification and avoid calling any clipboard related functions such as spice_gtk_session_paste_from_guest() from client-gtk library. This patch removes the silent return of spice_main_channel_clipboard_selection_release() in order to catch bugs with the proper check in agent_clipboard_release(). Signed-off-by: Victor Toso --- src/channel-main.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index 5bcd149..223043a 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -2825,11 +2825,6 @@ void spice_main_channel_clipboard_selection_release(SpiceMainChannel *channel, g g_return_if_fail(channel != NULL); g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel)); - SpiceMainChannelPrivate *c = channel->priv; - - if (!c->agent_connected) - return; - if (agent_clipboard_release(channel, selection)) { spice_channel_wakeup(SPICE_CHANNEL(channel), FALSE); } From patchwork Mon Dec 10 11:02:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,6/7] channel-main: clipboard grab: don't fail silently From: Victor Toso X-Patchwork-Id: 267298 Message-Id: <20181210110256.20115-7-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:55 +0100 From: Victor Toso Spice client should listen to SpiceMainChannel::agent-connected notification and avoid calling any clipboard related functions such as spice_gtk_session_paste_from_guest() from client-gtk library. This patch removes the silent return of agent_clipboard_grab() in order to properly catch bugs with critical messages. Signed-off-by: Victor Toso --- src/channel-main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index 223043a..aa563d2 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -1352,9 +1352,7 @@ static bool agent_clipboard_grab(SpiceMainChannel *channel, guint selection, size_t size; int i; - if (!c->agent_connected) - return false; - + g_return_val_if_fail(c->agent_connected, false); g_return_val_if_fail(test_agent_cap(channel, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false); size = sizeof(VDAgentClipboardGrab) + sizeof(uint32_t) * ntypes; From patchwork Mon Dec 10 11:02:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [spice-gtk,v2,7/7] gtk-session: remove extra clipboard selection check From: Victor Toso X-Patchwork-Id: 267299 Message-Id: <20181210110256.20115-8-victortoso@redhat.com> To: spice-devel@lists.freedesktop.org Date: Mon, 10 Dec 2018 12:02:56 +0100 From: Victor Toso Commit 284c1f2d switched to spice_main_channel_clipboard_selection_release() which does check if agent is connected and does the right thing (expected) in regards to releasing the clipboard by calling agent_clipboard_release() which does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed code). So this patch removes redundant check. Same goes for spice_main_channel_clipboard_selection_grab() function. Signed-off-by: Victor Toso --- src/spice-gtk-session.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c index 373d11e..c7e6e24 100644 --- a/src/spice-gtk-session.c +++ b/src/spice-gtk-session.c @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard, } s->clip_grabbed[selection] = TRUE; - - if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) - spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types); + spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types); /* Sending a grab causes the agent to do an implicit release */ s->nclip_targets[selection] = 0; @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard *clipboard, if (s->clip_grabbed[selection]) { s->clip_grabbed[selection] = FALSE; - if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) - spice_main_channel_clipboard_selection_release(s->main, selection); + spice_main_channel_clipboard_selection_release(s->main, selection); } switch (event->reason) {