[Spice-devel] [PATCH 02/16] Replace RedCharDevice::on_free_self_token with a signal

[Frediano Ziglio]

> 
> From: Christophe Fergeau <cfergeau at redhat.com>
> 
> It's more natural to do things this way with glib, and this allows to
> remove some coupling between Reds and RedCharDeviceVDIPort. Before this
> commit, the RedCharDeviceVDIPort has to provide a on_free_self_token()
> because Reds needs to know about it. With a signal, RedCharDeviceVDIPort
> does not have to do anything special, and Reds can just connect to the
> signal it's interested in.
> ---
> Frediano didn't like the signal approach. I think it's OK. Still needs
> resolution.

Let me clarify this, I think there are some pending issues/comments

1) signal and callback
This patch change the callback and use a glib signal instead. I don't
like the dynamic binding and I prefer previous static one but Christophe
and Jonathon prefer the signal (which is dynamic) and we are moving to
the unsafe world of glib so it's not a problem

2) reasoning
Moving from callback to signal does not remove the dependency, the
dependency is just changed to a signal, specifically
" this allows to remove some coupling between Reds and RedCharDeviceVDIPort"
no, you just changed char_dev_class->on_free_self_token assignment
to a g_signal_connect.
"With a signal, RedCharDeviceVDIPort does not have to do anything special"
no, RedCharDeviceVDIPort has to register a signal and emit it.
I would just change the comment to

  It's more natural to do things this way with glib.
  With a signal Reds can just connect to the signal it's interested in.

this was my strong argument

3) why the single callback was changed to multiple signals?
(this was raised by Jonathon)

Frediano

> 
>  server/char-device.c | 47 ++++++++++++++++++++++++++++++++++++-----------
>  server/char-device.h |  4 ----
>  server/reds.c        |  7 +++++--
>  3 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index ebe7633..f3e16da 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -94,6 +94,14 @@ enum {
>      PROP_OPAQUE
>  };
>  
> +enum {
> +   RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED,
> +   RED_CHAR_DEVICE_SELF_TOKEN_RELEASED,
> +   RED_CHAR_DEVICE_LAST_SIGNAL
> +};
> +
> +static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> +
>  static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
>  *write_buf);
>  static void red_char_device_write_retry(void *opaque);
>  
> @@ -131,16 +139,6 @@ red_char_device_send_tokens_to_client(RedCharDevice
> *dev,
>  }
>  
>  static void
> -red_char_device_on_free_self_token(RedCharDevice *dev)
> -{
> -   RedCharDeviceClass *klass = RED_CHAR_DEVICE_GET_CLASS(dev);
> -
> -   if (klass->on_free_self_token != NULL) {
> -       klass->on_free_self_token(dev->priv->opaque);
> -   }
> -}
> -
> -static void
>  red_char_device_remove_client(RedCharDevice *dev, RedClient *client)
>  {
>     RedCharDeviceClass *klass = RED_CHAR_DEVICE_GET_CLASS(dev);
> @@ -624,6 +622,9 @@ static RedCharDeviceWriteBuffer
> *__red_char_device_write_buffer_get(
>          }
>      } else if (origin == WRITE_BUFFER_ORIGIN_SERVER) {
>          dev->priv->num_self_tokens--;
> +        g_signal_emit(G_OBJECT(dev),
> +                      signals[RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED],
> +                      0);
>      }
>  
>      ret->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
> @@ -711,7 +712,9 @@ void red_char_device_write_buffer_release(RedCharDevice
> *dev,
>          red_char_device_client_tokens_add(dev, dev_client, buf_token_price);
>      } else if (buf_origin == WRITE_BUFFER_ORIGIN_SERVER) {
>          dev->priv->num_self_tokens++;
> -        red_char_device_on_free_self_token(dev);
> +        g_signal_emit(G_OBJECT(dev),
> +                      signals[RED_CHAR_DEVICE_SELF_TOKEN_RELEASED],
> +                      0);
>      }
>  }
>  
> @@ -1215,6 +1218,28 @@ red_char_device_class_init(RedCharDeviceClass *klass)
>                                                           "User data to pass
>                                                           to callbacks",
>                                                        G_PARAM_STATIC_STRINGS
>                                                        |
>                                                        G_PARAM_READWRITE));
> +    signals[RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED] =
> +       g_signal_new("self-token-consumed",
> +                    G_OBJECT_CLASS_TYPE(object_class),
> +                    G_SIGNAL_RUN_FIRST,
> +                    0, NULL, NULL,
> +                    g_cclosure_marshal_VOID__VOID,
> +                    G_TYPE_NONE,
> +                    0);
> +
> +    /* FIXME: find a better name for the signal? It replaces a
> +     * on_free_self_token vfunc whose description was:
> +     * « The cb is called when a server (self) message that was addressed to
> the device,
> +     *   has been completely written to it »
> +     */
> +    signals[RED_CHAR_DEVICE_SELF_TOKEN_RELEASED] =
> +       g_signal_new("self-token-released",
> +                    G_OBJECT_CLASS_TYPE(object_class),
> +                    G_SIGNAL_RUN_FIRST,
> +                    0, NULL, NULL,
> +                    g_cclosure_marshal_VOID__VOID,
> +                    G_TYPE_NONE,
> +                    0);
>  
>  }
>  
> diff --git a/server/char-device.h b/server/char-device.h
> index f793c54..c18ce66 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -67,10 +67,6 @@ struct RedCharDeviceClass
>       * device */
>      void (*send_tokens_to_client)(RedClient *client, uint32_t tokens, void
>      *opaque);
>  
> -    /* The cb is called when a server (self) message that was addressed to
> the device,
> -     * has been completely written to it */
> -    void (*on_free_self_token)(void *opaque);
> -
>      /* This cb is called if it is recommanded that a client will be removed
>       * due to slow flow or due to some other error.
>       * The called instance should disconnect the client, or at least the
>       corresponding channel */
> diff --git a/server/reds.c b/server/reds.c
> index 6e4fee4..7e5626a 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -876,7 +876,7 @@ static void vdi_port_send_tokens_to_client(RedClient
> *client, uint32_t tokens, v
>                                            tokens);
>  }
>  
> -static void vdi_port_on_free_self_token(void *opaque)
> +static void vdi_port_on_free_self_token(RedCharDevice *device, gpointer
> opaque)
>  {
>      RedsState *reds = opaque;
>  
> @@ -3377,6 +3377,10 @@ static int do_spice_init(RedsState *reds,
> SpiceCoreInterface *core_interface)
>      reds->listen_socket = -1;
>      reds->secure_listen_socket = -1;
>      reds->agent_dev = red_char_device_vdi_port_new(reds);
> +    g_signal_connect(G_OBJECT(reds->agent_dev),
> +                     "self-token-released",
> +                     (GCallback)vdi_port_on_free_self_token,
> +                     reds);
>      ring_init(&reds->clients);
>      reds->num_clients = 0;
>      reds->main_dispatcher = main_dispatcher_new(reds, reds->core);
> @@ -4322,7 +4326,6 @@
> red_char_device_vdi_port_class_init(RedCharDeviceVDIPortClass *klass)
>      char_dev_class->send_msg_to_client = vdi_port_send_msg_to_client;
>      char_dev_class->send_tokens_to_client = vdi_port_send_tokens_to_client;
>      char_dev_class->remove_client = vdi_port_remove_client;
> -    char_dev_class->on_free_self_token = vdi_port_on_free_self_token;
>  }
>  
>  static RedCharDeviceVDIPort *red_char_device_vdi_port_new(RedsState *reds)