Hey, On Wed, Apr 20, 2016 at 04:29:56AM -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. > > --- > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel