Re: [PATCH 12/20] Replace RedCharDevice::on_free_self_token with a signal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2016-04-15 at 06:18 -0400, Frediano Ziglio wrote:
> > 
> > From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> > 
> > 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.
> 
> This make no sense to me. Once you connect the signal you know the same
> detail on the other class.
> Personally I prefer the static binding that was in previous code.

Hmm, I don't feel strongly, but I think I prefer the signal version. To me,
class-level 'opaque' variables don't make much sense, and GObject signals allow
you to bind user data specific to the signal callback. That seems to make more
sense to me. 

I don't know why the second signal is defined, however, since it doesn't appear
to be used anywhere.

> 
> 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);
> >  
> >  }
> 
> Where these signals are destroyed ?

g_signal_new just returns an integer ID of the signal. So there's not really
anything for us to free or destroy. I don't know whether glib frees the internal
structure somehow or just lets the OS release it at process exit, but there's
not really anything we can do about it.

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]