[spice-gtk,v1,2/4] gtk-session: prefer early check to agent connectivity

Submitted by Victor Toso on Dec. 5, 2018, 3:52 p.m.

Details

Message ID 20181205155244.19933-3-victortoso@redhat.com
State New
Headers show
Series "gtk-session misc changes" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 5, 2018, 3:52 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.

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 8d31045..1615172 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -725,6 +725,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);
@@ -741,13 +747,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().
@@ -758,7 +757,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

> 
> 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.
> 
> 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 8d31045..1615172 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -725,6 +725,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);
> @@ -741,13 +747,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().
> @@ -758,7 +757,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);

Before spice_main_channel_clipboard_selection_request was always called so
spice_channel_wakeup was also called. Honestly I don't have enough knowledge
to understand if this could be an issue or not. Not clear which events
could be possible pending here, the function looks quite an hack, looking
at the signals set there's "notify::agent-connected" which seems to mean
that meanwhile the "agent-connected" value is expected to change during this
function.

Frediano
Hi,

On Thu, Dec 06, 2018 at 04:15:14AM -0500, Frediano Ziglio 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.
> > 
> > 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 8d31045..1615172 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -725,6 +725,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);
> > @@ -741,13 +747,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().
> > @@ -758,7 +757,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);
> 
> Before spice_main_channel_clipboard_selection_request was
> always called so spice_channel_wakeup was also called. Honestly
> I don't have enough knowledge to understand if this could be an
> issue or not.

Ah, you are right. Well:
fn spice_main_channel_clipboard_selection_request() calls
fn agent_clipboard_request() which will fail on check
g_return_if_fail(c->agent_connected)

So, if we don't want to have the failure (we want to send the
agent message only if agent is connected) moving this check up is
not just a cleanup but should avoid this kind of issue.

At this moment, we don't have this check so _should_ be possible
to triggered this critical by calling
spice_gtk_session_paste_from_guest while agent is not
connected. Spicy tool listens to agent-connect and does not allow
it to happen but we shouldn't rely on the library's user, I
think.

> Not clear which events could be possible pending here, the
> function looks quite an hack, looking
> at the signals set there's "notify::agent-connected" which
> seems to mean that meanwhile the "agent-connected" value is
> expected to change during this function.
> 
> Frediano

Yeah, perhaps utility function would be better
(..)_agent_is_connected();

Victor