[spice-gtk,v2,2/7] channel-main: clipboard request: wakeup only when needed

Submitted by Victor Toso on Dec. 10, 2018, 11:02 a.m.

Details

Message ID 20181210110256.20115-3-victortoso@redhat.com
State New
Headers show
Series "small code improvements on clipboard" ( rev: 1 ) in Spice

Commit Message

Victor Toso Dec. 10, 2018, 11:02 a.m.
From: Victor Toso <me@victortoso.com>

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 <victortoso@redhat.com>
---
 src/channel-main.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

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);
+    }
 }
 
 /**

Comments

Hi,

On Mon, Dec 10, 2018 at 12:03 PM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@victortoso.com>
>
> 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 <victortoso@redhat.com>
> ---
>  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)

Might be better to use gboolean and TRUE/FALSE (instead of bool with
true/false) to match the rest of the file?
Apart from that, it looks good to me.

Cheers,
Jakub

>  {
>      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);
> +    }
>  }
>
>  /**
> --
> 2.19.2
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Tue, Dec 11, 2018 at 07:04:10PM +0100, Jakub Janku wrote:
> Hi,
> 
> On Mon, Dec 10, 2018 at 12:03 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > From: Victor Toso <me@victortoso.com>
> >
> > 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 <victortoso@redhat.com>
> > ---
> >  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)
> 
> Might be better to use gboolean and TRUE/FALSE (instead of bool with
> true/false) to match the rest of the file?

Indeed, I was not considering the rest of the file while using
bool. I'll change it.

> Apart from that, it looks good to me.
> 
> Cheers,
> Jakub

Thanks!

> 
> >  {
> >      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);
> > +    }
> >  }
> >
> >  /**
> > --
> > 2.19.2
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel