[spice-server,3/3] red-channel: Allows to retrieve client callback saves pointer

Submitted by Frediano Ziglio on Aug. 25, 2017, 10:24 a.m.

Details

Message ID 20170825102441.6258-3-fziglio@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 25, 2017, 10:24 a.m.
The data pointer for client callbacks was not used.
The code was saving this information using callback registration
and g_object_set_data. Remove the g_object_set_data using
the data registered.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-channel.c |  5 +++++
 server/red-channel.h |  2 ++
 server/red-qxl.c     | 12 ++++++------
 server/red-worker.c  |  2 --
 4 files changed, 13 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red-channel.c b/server/red-channel.c
index 9ff3474a..3c4d236b 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -372,6 +372,11 @@  void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs *clien
     channel->priv->data = cbs_data;
 }
 
+void *red_channel_get_registered_client_cbs_data(RedChannel *channel)
+{
+    return channel->priv->data;
+}
+
 static void add_capability(uint32_t **caps, int *num_caps, uint32_t cap)
 {
     int nbefore, n;
diff --git a/server/red-channel.h b/server/red-channel.h
index e65eea1e..0068294a 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -136,6 +136,8 @@  void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc);
 void red_channel_init_stat_node(RedChannel *channel, const RedStatNode *parent, const char *name);
 
 void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs *client_cbs, gpointer cbs_data);
+// retrieve data pointer stored with red_channel_register_client_cbs
+void *red_channel_get_registered_client_cbs_data(RedChannel *channel);
 // caps are freed when the channel is destroyed
 void red_channel_set_common_cap(RedChannel *channel, uint32_t cap);
 void red_channel_set_cap(RedChannel *channel, uint32_t cap);
diff --git a/server/red-qxl.c b/server/red-qxl.c
index ba869e54..0eaf0e83 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -82,7 +82,7 @@  static void red_qxl_set_display_peer(RedChannel *channel, RedClient *client,
     Dispatcher *dispatcher;
 
     spice_debug("%s", "");
-    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
     payload.client = client;
     payload.stream = stream;
     payload.migration = migration;
@@ -99,7 +99,7 @@  static void red_qxl_disconnect_display_peer(RedChannelClient *rcc)
     Dispatcher *dispatcher;
     RedChannel *channel = red_channel_client_get_channel(rcc);
 
-    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
 
     spice_printerr("");
     payload.rcc = rcc;
@@ -119,7 +119,7 @@  static void red_qxl_display_migrate(RedChannelClient *rcc)
     uint32_t type, id;
 
     g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
     spice_printerr("channel type %u id %u", type, id);
     payload.rcc = rcc;
     dispatcher_send_message(dispatcher,
@@ -132,7 +132,7 @@  static void red_qxl_set_cursor_peer(RedChannel *channel, RedClient *client, Reds
                                     RedChannelCapabilities *caps)
 {
     RedWorkerMessageCursorConnect payload = {0,};
-    Dispatcher *dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    Dispatcher *dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
     spice_printerr("");
     payload.client = client;
     payload.stream = stream;
@@ -150,7 +150,7 @@  static void red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
     Dispatcher *dispatcher;
     RedChannel *channel = red_channel_client_get_channel(rcc);
 
-    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
     spice_printerr("");
     payload.rcc = rcc;
 
@@ -167,7 +167,7 @@  static void red_qxl_cursor_migrate(RedChannelClient *rcc)
     uint32_t type, id;
 
     g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
+    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);
     spice_printerr("channel type %u id %u", type, id);
     payload.rcc = rcc;
     dispatcher_send_message(dispatcher,
diff --git a/server/red-worker.c b/server/red-worker.c
index 03a409cd..6e2f9b59 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -1347,7 +1347,6 @@  RedWorker* red_worker_new(QXLInstance *qxl,
     channel = RED_CHANNEL(worker->cursor_channel);
     red_channel_init_stat_node(channel, &worker->stat, "cursor_channel");
     red_channel_register_client_cbs(channel, client_cursor_cbs, dispatcher);
-    g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
     reds_register_channel(reds, channel);
 
     // TODO: handle seemless migration. Temp, setting migrate to FALSE
@@ -1358,7 +1357,6 @@  RedWorker* red_worker_new(QXLInstance *qxl,
     channel = RED_CHANNEL(worker->display_channel);
     red_channel_init_stat_node(channel, &worker->stat, "display_channel");
     red_channel_register_client_cbs(channel, client_display_cbs, dispatcher);
-    g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
     reds_register_channel(reds, channel);
 
     return worker;

Comments

On Fri, Aug 25, 2017 at 11:24:41AM +0100, Frediano Ziglio wrote:
> The data pointer for client callbacks was not used.
> The code was saving this information using callback registration
> and g_object_set_data. Remove the g_object_set_data using
> the data registered.

Ah, hmm, I actually had different plans for this 'data' stuff, which is
to just kill it ;) (and do some more reworking of the client_cbs code).
Yet another patch series I have lying around but never got around to
testing/sending. Yes this means to keep living with the
g_object_set_data() use for 'dispatcher', I did not really pay attention
to it until now, so don't know if there are alternative ways of getting
rid of it.

Some comments below if we go this way:

> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/red-channel.c |  5 +++++
>  server/red-channel.h |  2 ++
>  server/red-qxl.c     | 12 ++++++------
>  server/red-worker.c  |  2 --
>  4 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 9ff3474a..3c4d236b 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -372,6 +372,11 @@ void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs *clien
>      channel->priv->data = cbs_data;
>  }
>  
> +void *red_channel_get_registered_client_cbs_data(RedChannel *channel)

I'd just name this "red_channel_get_client_cbs_data()

> +{
> +    return channel->priv->data;

This could be channel->priv->client_cbs.data I think? I really prefer
when the user data is where it belongs, rather than a generic 'data'
member in the bigger Channel class.

> +}
> +
>  static void add_capability(uint32_t **caps, int *num_caps, uint32_t cap)
>  {
>      int nbefore, n;
> diff --git a/server/red-channel.h b/server/red-channel.h
> index e65eea1e..0068294a 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -136,6 +136,8 @@ void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc);
>  void red_channel_init_stat_node(RedChannel *channel, const RedStatNode *parent, const char *name);
>  
>  void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs *client_cbs, gpointer cbs_data);
> +// retrieve data pointer stored with red_channel_register_client_cbs
> +void *red_channel_get_registered_client_cbs_data(RedChannel *channel);
>  // caps are freed when the channel is destroyed
>  void red_channel_set_common_cap(RedChannel *channel, uint32_t cap);
>  void red_channel_set_cap(RedChannel *channel, uint32_t cap);
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index ba869e54..0eaf0e83 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -82,7 +82,7 @@ static void red_qxl_set_display_peer(RedChannel *channel, RedClient *client,
>      Dispatcher *dispatcher;
>  
>      spice_debug("%s", "");
> -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel), "dispatcher");
> +    dispatcher = (Dispatcher *)red_channel_get_registered_client_cbs_data(channel);

It might be more consistent to do
 typedef void (*channel_client_connect_proc)(RedChannel *channel, RedClient *client, RedsStream *stream,
-                                              int migration, RedChannelCapabilities *caps);
+                                              int migration, RedChannelCapabilities *caps, gpointer user_data);
instead, and to not have a get_client_cbs_data() method at all.

Maybe wait a bit until I take a closer look at the work I had done
before reworking this series?

Christophe
> 
> On Fri, Aug 25, 2017 at 11:24:41AM +0100, Frediano Ziglio wrote:
> > The data pointer for client callbacks was not used.
> > The code was saving this information using callback registration
> > and g_object_set_data. Remove the g_object_set_data using
> > the data registered.
> 
> Ah, hmm, I actually had different plans for this 'data' stuff, which is
> to just kill it ;) (and do some more reworking of the client_cbs code).
> Yet another patch series I have lying around but never got around to
> testing/sending. Yes this means to keep living with the
> g_object_set_data() use for 'dispatcher', I did not really pay attention
> to it until now, so don't know if there are alternative ways of getting
> rid of it.
> 
> Some comments below if we go this way:
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  server/red-channel.c |  5 +++++
> >  server/red-channel.h |  2 ++
> >  server/red-qxl.c     | 12 ++++++------
> >  server/red-worker.c  |  2 --
> >  4 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 9ff3474a..3c4d236b 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -372,6 +372,11 @@ void red_channel_register_client_cbs(RedChannel
> > *channel, const ClientCbs *clien
> >      channel->priv->data = cbs_data;
> >  }
> >  
> > +void *red_channel_get_registered_client_cbs_data(RedChannel *channel)
> 
> I'd just name this "red_channel_get_client_cbs_data()
> 

done

> > +{
> > +    return channel->priv->data;
> 
> This could be channel->priv->client_cbs.data I think? I really prefer
> when the user data is where it belongs, rather than a generic 'data'
> member in the bigger Channel class.
> 

done. I think this was suggested by Francois (at least for encoding side).

> > +}
> > +
> >  static void add_capability(uint32_t **caps, int *num_caps, uint32_t cap)
> >  {
> >      int nbefore, n;
> > diff --git a/server/red-channel.h b/server/red-channel.h
> > index e65eea1e..0068294a 100644
> > --- a/server/red-channel.h
> > +++ b/server/red-channel.h
> > @@ -136,6 +136,8 @@ void red_channel_remove_client(RedChannel *channel,
> > RedChannelClient *rcc);
> >  void red_channel_init_stat_node(RedChannel *channel, const RedStatNode
> >  *parent, const char *name);
> >  
> >  void red_channel_register_client_cbs(RedChannel *channel, const ClientCbs
> >  *client_cbs, gpointer cbs_data);
> > +// retrieve data pointer stored with red_channel_register_client_cbs
> > +void *red_channel_get_registered_client_cbs_data(RedChannel *channel);
> >  // caps are freed when the channel is destroyed
> >  void red_channel_set_common_cap(RedChannel *channel, uint32_t cap);
> >  void red_channel_set_cap(RedChannel *channel, uint32_t cap);
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index ba869e54..0eaf0e83 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -82,7 +82,7 @@ static void red_qxl_set_display_peer(RedChannel *channel,
> > RedClient *client,
> >      Dispatcher *dispatcher;
> >  
> >      spice_debug("%s", "");
> > -    dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > "dispatcher");
> > +    dispatcher = (Dispatcher
> > *)red_channel_get_registered_client_cbs_data(channel);
> 
> It might be more consistent to do
>  typedef void (*channel_client_connect_proc)(RedChannel *channel, RedClient
>  *client, RedsStream *stream,
> -                                              int migration,
> RedChannelCapabilities *caps);
> +                                              int migration,
> RedChannelCapabilities *caps, gpointer user_data);
> instead, and to not have a get_client_cbs_data() method at all.
> 

Most of the uses do not need this (actually only channels running
in a different thread) so would be maybe a bit overkilling.

> Maybe wait a bit until I take a closer look at the work I had done
> before reworking this series?
> 
> Christophe
> 

Sent a new version replacing 2/3 and 3/3.

What about 1/3 ? Not much dependent actually, just touching same area
(OT: how to mark this patch is part of this series but not related
to the subject??)

Frediano