>
> 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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel