[Spice-devel] [PATCH 8/9] 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.
> ---
>  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 8ed1a2a..7d099a0 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -91,6 +91,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);
>  
> @@ -123,16 +131,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);
> @@ -576,6 +574,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;
> @@ -668,7 +669,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);
>      }
>  }
>  
> @@ -1161,6 +1164,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 14cbe94..127e87e 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -72,10 +72,6 @@ struct RedCharDeviceClass
>                                    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 recommended to remove the client
>       * 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 0d9cef7..98f39e5 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -925,7 +925,7 @@ static void vdi_port_send_tokens_to_client(RedCharDevice
> *self,
>                                            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;
>  
> @@ -3432,6 +3432,10 @@ static int do_spice_init(RedsState *reds,
> SpiceCoreInterface *core_interface)
>      reds->secure_listen_socket = -1;
>      reds->agent_dev = red_char_device_vdi_port_new(reds);
>      reds_update_agent_properties(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);
> @@ -4527,7 +4531,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)

Hi,
  if I remember correctly even Christophe after some comments
propose another implementation (week reference?).

Frediano