[Spice-devel] [spice-gtk v1 1/6] Avoid accessing SpiceChannel's internals
Victor Toso
victortoso at redhat.com
Tue Sep 24 12:55:20 UTC 2019
On Tue, Sep 24, 2019 at 06:27:55AM -0400, Frediano Ziglio wrote:
> >
> > From: Victor Toso <me at victortoso.com>
> >
> > SpiceChannel provides some internal utility functions that this patch
> > takes advantage of:
> >
> > * spice_channel_get_channel_type()
> > * spice_channel_get_channel_id()
> > * spice_channel_get_state()
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
>
> The question is also why all these files have access to "private" fields?
> How much NOT private they are? Looks like is more an "exported"
> than a private.
Considering that SpiceMainChannel and family are also
SpiceChannel (base class), perhaps this is choice of
implementation.
I often consider that the private structure should really be
accessed (read/write) with functions only.
> > ---
> > src/channel-inputs.c | 65 +++++++++++++++++++++++++++++---------------
> > src/channel-main.c | 28 +++++++++----------
> > src/spice-audio.c | 6 ++--
> > src/spice-session.c | 3 +-
> > 4 files changed, 62 insertions(+), 40 deletions(-)
> >
> > diff --git a/src/channel-inputs.c b/src/channel-inputs.c
> > index a8cdd23..73cbb0f 100644
> > --- a/src/channel-inputs.c
> > +++ b/src/channel-inputs.c
> > @@ -304,14 +304,20 @@ void spice_inputs_channel_motion(SpiceInputsChannel
> > *channel, gint dx, gint dy,
> > gint button_state)
> > {
> > SpiceInputsChannelPrivate *c;
> > + gint channel_state;
>
> Why not enum spice_channel_state ?
> Here and in different hunks
Fixed
>
> >
> > g_return_if_fail(channel != NULL);
> > - g_return_if_fail(SPICE_CHANNEL(channel)->priv->state !=
> > SPICE_CHANNEL_STATE_UNCONNECTED);
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > +
> > + channel_state = spice_channel_get_state(SPICE_CHANNEL(channel));
> > + g_return_if_fail(channel_state != SPICE_CHANNEL_STATE_UNCONNECTED);
> > +
> > + if (channel_state != SPICE_CHANNEL_STATE_READY) {
> > return;
> > + }
> >
> > - if (dx == 0 && dy == 0)
> > + if (dx == 0 && dy == 0) {
> > return;
> > + }
> >
> > c = channel->priv;
> > c->bs = button_state;
> > @@ -360,8 +366,9 @@ void spice_inputs_channel_position(SpiceInputsChannel
> > *channel, gint x, gint y,
> >
> > g_return_if_fail(channel != NULL);
> >
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > + if (spice_channel_get_state(SPICE_CHANNEL(channel)) !=
> > SPICE_CHANNEL_STATE_READY) {
> > return;
> > + }
> >
> > c = channel->priv;
> > c->bs = button_state;
> > @@ -411,10 +418,12 @@ void
> > spice_inputs_channel_button_press(SpiceInputsChannel *channel, gint button,
> >
> > g_return_if_fail(channel != NULL);
> >
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > + if (spice_channel_get_state(SPICE_CHANNEL(channel)) !=
> > SPICE_CHANNEL_STATE_READY) {
> > return;
> > - if (spice_channel_get_read_only(SPICE_CHANNEL(channel)))
> > + }
> > + if (spice_channel_get_read_only(SPICE_CHANNEL(channel))) {
> > return;
> > + }
> >
> > c = channel->priv;
> > switch (button) {
> > @@ -476,10 +485,12 @@ void
> > spice_inputs_channel_button_release(SpiceInputsChannel *channel, gint butto
> >
> > g_return_if_fail(channel != NULL);
> >
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > + if (spice_channel_get_state(SPICE_CHANNEL(channel)) !=
> > SPICE_CHANNEL_STATE_READY) {
> > return;
> > - if (spice_channel_get_read_only(SPICE_CHANNEL(channel)))
> > + }
> > + if (spice_channel_get_read_only(SPICE_CHANNEL(channel))) {
> > return;
> > + }
> >
> > c = channel->priv;
> > switch (button) {
> > @@ -535,13 +546,18 @@ void spice_inputs_channel_key_press(SpiceInputsChannel
> > *channel, guint scancode)
> > {
> > SpiceMsgcKeyDown down;
> > SpiceMsgOut *msg;
> > + gint channel_state;
> >
> > g_return_if_fail(channel != NULL);
> > - g_return_if_fail(SPICE_CHANNEL(channel)->priv->state !=
> > SPICE_CHANNEL_STATE_UNCONNECTED);
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > +
> > + channel_state = spice_channel_get_state(SPICE_CHANNEL(channel));
> > + g_return_if_fail(channel_state != SPICE_CHANNEL_STATE_UNCONNECTED);
> > + if (channel_state != SPICE_CHANNEL_STATE_READY) {
> > return;
> > - if (spice_channel_get_read_only(SPICE_CHANNEL(channel)))
> > + }
> > + if (spice_channel_get_read_only(SPICE_CHANNEL(channel))) {
> > return;
> > + }
> >
> > down.code = spice_make_scancode(scancode, FALSE);
> > msg = spice_msg_out_new(SPICE_CHANNEL(channel),
> > SPICE_MSGC_INPUTS_KEY_DOWN);
> > @@ -578,13 +594,18 @@ void
> > spice_inputs_channel_key_release(SpiceInputsChannel *channel, guint scancod
> > {
> > SpiceMsgcKeyUp up;
> > SpiceMsgOut *msg;
> > + gint channel_state;
> >
> > g_return_if_fail(channel != NULL);
> > - g_return_if_fail(SPICE_CHANNEL(channel)->priv->state !=
> > SPICE_CHANNEL_STATE_UNCONNECTED);
> > - if (SPICE_CHANNEL(channel)->priv->state != SPICE_CHANNEL_STATE_READY)
> > +
> > + channel_state = spice_channel_get_state(SPICE_CHANNEL(channel));
> > + g_return_if_fail(channel_state != SPICE_CHANNEL_STATE_UNCONNECTED);
> > + if (channel_state != SPICE_CHANNEL_STATE_READY) {
> > return;
> > - if (spice_channel_get_read_only(SPICE_CHANNEL(channel)))
> > + }
> > + if (spice_channel_get_read_only(SPICE_CHANNEL(channel))) {
> > return;
> > + }
> >
> > up.code = spice_make_scancode(scancode, TRUE);
> > msg = spice_msg_out_new(SPICE_CHANNEL(channel),
> > SPICE_MSGC_INPUTS_KEY_UP);
> > @@ -624,13 +645,14 @@ void
> > spice_inputs_channel_key_press_and_release(SpiceInputsChannel *input_channe
> > SpiceChannel *channel = SPICE_CHANNEL(input_channel);
> >
> > g_return_if_fail(channel != NULL);
> > - g_return_if_fail(channel->priv->state !=
> > SPICE_CHANNEL_STATE_UNCONNECTED);
> > + g_return_if_fail(spice_channel_get_state(channel) !=
> > SPICE_CHANNEL_STATE_UNCONNECTED);
> >
> > - if (channel->priv->state != SPICE_CHANNEL_STATE_READY)
> > + if (spice_channel_get_state(channel) != SPICE_CHANNEL_STATE_READY) {
> > return;
> > - if (spice_channel_get_read_only(channel))
> > + }
> > + if (spice_channel_get_read_only(channel)) {
> > return;
> > -
> > + }
> > if (spice_channel_test_capability(channel,
> > SPICE_INPUTS_CAP_KEY_SCANCODE)) {
> > SpiceMsgOut *msg;
> > guint16 code;
> > @@ -664,16 +686,15 @@ static SpiceMsgOut* set_key_locks(SpiceInputsChannel
> > *channel, guint locks)
> > SpiceMsgcKeyModifiers modifiers;
> > SpiceMsgOut *msg;
> > SpiceInputsChannelPrivate *ic;
> > - SpiceChannelPrivate *c;
> >
> > g_return_val_if_fail(SPICE_IS_INPUTS_CHANNEL(channel), NULL);
> >
> > ic = channel->priv;
> > - c = SPICE_CHANNEL(channel)->priv;
> > -
> > ic->locks = locks;
> > - if (c->state != SPICE_CHANNEL_STATE_READY)
> > +
> > + if (spice_channel_get_state(SPICE_CHANNEL(channel)) !=
> > SPICE_CHANNEL_STATE_READY) {
> > return NULL;
> > + }
> >
> > msg = spice_msg_out_new(SPICE_CHANNEL(channel),
> > SPICE_MSGC_INPUTS_KEY_MODIFIERS);
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 334be7d..ac0d408 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -2204,14 +2204,13 @@ static void migrate_channel_event_cb(SpiceChannel
> > *channel, SpiceChannelEvent ev
> > gpointer data)
> > {
> > spice_migrate *mig = data;
> > - SpiceChannelPrivate *c = SPICE_CHANNEL(channel)->priv;
> >
> > g_return_if_fail(mig->nchannels > 0);
> > g_signal_handlers_disconnect_by_func(channel, migrate_channel_event_cb,
> > data);
> >
> > switch (event) {
> > case SPICE_CHANNEL_OPENED:
> > - if (c->channel_type == SPICE_CHANNEL_MAIN) {
> > + if (spice_channel_get_channel_type(channel) == SPICE_CHANNEL_MAIN) {
> > SpiceSession *session =
> > spice_channel_get_session(mig->src_channel);
> > if (mig->do_seamless) {
> > SpiceMainChannelPrivate *main_priv =
> > SPICE_MAIN_CHANNEL(channel)->priv;
> > @@ -2227,11 +2226,15 @@ static void migrate_channel_event_cb(SpiceChannel
> > *channel, SpiceChannelEvent ev
> > GList *channels, *l;
> > l = channels = spice_session_get_channels(session);
> > while (l != NULL) {
> > - SpiceChannelPrivate *curc = SPICE_CHANNEL(l->data)->priv;
> > + SpiceChannel *it = SPICE_CHANNEL(l->data);
> > +
> > l = l->next;
> > - if (curc->channel_type == SPICE_CHANNEL_MAIN)
> > + if (spice_channel_get_channel_type(it) ==
> > SPICE_CHANNEL_MAIN) {
> > continue;
> > - migrate_channel_connect(mig, curc->channel_type,
> > curc->channel_id);
> > + }
> > + migrate_channel_connect(mig,
> > + spice_channel_get_channel_type(it),
> > + spice_channel_get_channel_id(it));
> > }
> > g_list_free(channels);
> > } else {
> > @@ -2254,10 +2257,10 @@ static void migrate_channel_event_cb(SpiceChannel
> > *channel, SpiceChannelEvent ev
> > static gboolean main_migrate_handshake_done(gpointer data)
> > {
> > spice_migrate *mig = data;
> > - SpiceChannelPrivate *c = SPICE_CHANNEL(mig->dst_channel)->priv;
> > + SpiceChannel *channel = SPICE_CHANNEL(mig->dst_channel);
> >
> > - g_return_val_if_fail(c->channel_type == SPICE_CHANNEL_MAIN, FALSE);
> > - g_return_val_if_fail(c->state ==
> > SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE, FALSE);
> > + g_return_val_if_fail(spice_channel_get_channel_type(channel) ==
> > SPICE_CHANNEL_MAIN, FALSE);
> > + g_return_val_if_fail(spice_channel_get_state(channel) ==
> > SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE, FALSE);
> >
> > c->state = SPICE_CHANNEL_STATE_MIGRATING;
> > mig->nchannels--;
> > @@ -2407,20 +2410,18 @@ static void
> > main_handle_migrate_begin_seamless(SpiceChannel *channel, SpiceMsgIn
> >
> > static void main_handle_migrate_dst_seamless_ack(SpiceChannel *channel,
> > SpiceMsgIn *in)
> > {
> > - SpiceChannelPrivate *c = SPICE_CHANNEL(channel)->priv;
> > SpiceMainChannelPrivate *main_priv = SPICE_MAIN_CHANNEL(channel)->priv;
> >
> > - g_return_if_fail(c->state == SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
> > + g_return_if_fail(spice_channel_get_state(channel) ==
> > SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
> > main_priv->migrate_data->do_seamless = true;
> > g_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
> > }
> >
> > static void main_handle_migrate_dst_seamless_nack(SpiceChannel *channel,
> > SpiceMsgIn *in)
> > {
> > - SpiceChannelPrivate *c = SPICE_CHANNEL(channel)->priv;
> > SpiceMainChannelPrivate *main_priv = SPICE_MAIN_CHANNEL(channel)->priv;
> >
> > - g_return_if_fail(c->state == SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
> > + g_return_if_fail(spice_channel_get_state(channel) ==
> > SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
> > main_priv->migrate_data->do_seamless = false;
> > g_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
> > }
> > @@ -2551,11 +2552,10 @@ static void spice_main_handle_msg(SpiceChannel
> > *channel, SpiceMsgIn *msg)
> > {
> > int type = spice_msg_in_type(msg);
> > SpiceChannelClass *parent_class;
> > - SpiceChannelPrivate *c = SPICE_CHANNEL(channel)->priv;
> >
> > parent_class = SPICE_CHANNEL_CLASS(spice_main_channel_parent_class);
> >
> > - if (c->state == SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE) {
> > + if (spice_channel_get_state(channel) ==
> > SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE) {
> > if (type != SPICE_MSG_MAIN_MIGRATE_DST_SEAMLESS_ACK &&
> > type != SPICE_MSG_MAIN_MIGRATE_DST_SEAMLESS_NACK) {
> > g_critical("unexpected msg (%d)."
> > diff --git a/src/spice-audio.c b/src/spice-audio.c
> > index daf62df..10b31d7 100644
> > --- a/src/spice-audio.c
> > +++ b/src/spice-audio.c
> > @@ -145,11 +145,13 @@ static void spice_audio_init(SpiceAudio *self)
> >
> > static void connect_channel(SpiceAudio *self, SpiceChannel *channel)
> > {
> > - if (channel->priv->state != SPICE_CHANNEL_STATE_UNCONNECTED)
> > + if (spice_channel_get_state(channel) != SPICE_CHANNEL_STATE_UNCONNECTED)
> > {
> > return;
> > + }
> >
> > - if (SPICE_AUDIO_GET_CLASS(self)->connect_channel(self, channel))
> > + if (SPICE_AUDIO_GET_CLASS(self)->connect_channel(self, channel)) {
> > spice_channel_connect(channel);
> > + }
> > }
> >
> > static void update_audio_channels(SpiceAudio *self, SpiceSession *session)
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index d0d9e54..afeb724 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -2203,7 +2203,6 @@ GSocketConnection*
> > spice_session_channel_open_host(SpiceSession *session, SpiceC
> > g_return_val_if_fail(SPICE_IS_SESSION(session), NULL);
> >
> > SpiceSessionPrivate *s = session->priv;
> > - SpiceChannelPrivate *c = channel->priv;
> > spice_open_host open_host = { 0, };
> > gchar *port, *endptr;
> >
> > @@ -2212,7 +2211,7 @@ GSocketConnection*
> > spice_session_channel_open_host(SpiceSession *session, SpiceC
> > open_host.session = session;
> > open_host.channel = channel;
> >
> > - const char *name = spice_channel_type_to_string(c->channel_type);
> > + const char *name =
> > spice_channel_type_to_string(spice_channel_get_channel_type(channel));
> > if (spice_strv_contains(s->secure_channels, "all") ||
> > spice_strv_contains(s->secure_channels, name))
> > *use_tls = TRUE;
>
> By the way, the patch does not compile.
My bad in splitting up this and following patch, sorry.
I sent v2, in my rebased version, the follow up patch removes
SpiceChannelPrivate *c; from these two functions that are still
using them in this patch.
Thanks for review and checking it al.
Cheers,
>
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190924/cc6a07b0/attachment.sig>
More information about the Spice-devel
mailing list