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

[Christophe Fergeau]

Hey,

On Wed, Apr 20, 2016 at 04:29:56AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

The reasoning behind this coupling comment is that reds.c wants to do
some housekeeping when a token is released in a char device. It seems
odd to have the notification needs of something external to the
RedCharDevice mandate the addition of a vfunc in the base RedCharDevice
class.
And moving the on_free_self_token assignment to a g_signal_connect gives
us more flexibility, it can be done by external code, it no longer has
to be done in the implementation of the class we are interested in.

With that said, I looked a bit more closely at that code, and
"on_free_self_token" is not about getting resource released, but more
about feeding more data to the char device now that it's ok to do
so. So yeah, maybe it would make sense to do things a bit differently.

Maybe a more explicit API for token handling (grouping all the token
stuff in a struct, having a few methods to consume/release a token, ...)
would make more sense, in which case we could register a handler to
feed more data into the device when needed.


But really, a vfunc called on_xxx sounds to me like some code external
to the class needed to be notified of something, and in the absence of a
non-invasive mechanism to do these notifications, we fell back on
hardcoding this requirement on the class itself. I prefer to have the
class notifies in a generic way about this, and interested consumer to
be able to catch it if needed.


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

Probably for symmetry even if it's unused. The additional signal can be
dropped.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160420/10d0207c/attachment.sig>