[spice-gtk] gtk-session: prefer early check to agent connectivity

Submitted by Victor Toso on Jan. 10, 2019, 3:39 p.m.

Details

Message ID 20190110153957.2033-1-victortoso@redhat.com
State New
Headers show
Series "gtk-session: prefer early check to agent connectivity" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Jan. 10, 2019, 3:39 p.m.
From: Victor Toso <me@victortoso.com>

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.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-gtk-session.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

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);

Comments

This patch looks reasonable to me, but I wonder if you could expand the
commit log to indicate the steps required to trigger the warning?  In
what situation is this function (clipboard_get()) being called when the
agent is not connected? 

Jonathon




On Thu, 2019-01-10 at 16:39 +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> 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.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  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);
Hi,

On Thu, Jan 10, 2019 at 01:55:37PM -0600, Jonathon Jongsma wrote:
> This patch looks reasonable to me, but I wonder if you could expand the
> commit log to indicate the steps required to trigger the warning?  In
> what situation is this function (clipboard_get()) being called when the
> agent is not connected? 

Sorry if I did not put it well in the commit log, I did not hit
it in git master (or I not recall doing so). I meant that
spice_main_channel_clipboard_selection_request() is being called
before checking if agent is connected which would trigger the
critical message.

Should be safe to remove the check of agent_connected in
clipboard_get()

> Jonathon
> 
> On Thu, 2019-01-10 at 16:39 +0100, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > 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.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  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);
>