WIP implementation.
Somme comments: - The temp file used for the unix file when making the Offer should be deleted when the socket is closed. - There is a risk of infinite loop when creating a unix socket name, if g_socket_listener_add_address() fails for another reason than the file already being used. - I think it is useful to be able to offer my own GSocketAddress. - #ifdef HAVE_GIO_UNIX should be moved inside determine_socket_type(), in case the CM supports unix socket but not the client.
Fixed all of these issues. It would be nice to get a proper review of this branch now.
I'm not convinced by this channel-view approach. That means that in practice any application will have to create and prepare an extra object per channel when receiving them. Th
I'm not convinced by this channel-view approach. That means that in practice any application will have to create and prepare an extra object per channel when receiving them. That makes me kinda sad as we spend lot of time working on TpBaseClient & friends to make it as simple as possible to use by preparing the TpChannel objects we pass to the user. We even considered adding extra API to ask "Please prepare those extra features on the channel". So, why not create subclass of TpChannel for high level API? We could add API to TpBaseClient to say we are interested in those and even API to pass our own factory if we want to create crazy subclass ourself.
(In reply to comment #5) > I'm not convinced by this channel-view approach. That means that in practice > any application will have to create and prepare an extra object per channel > when receiving them. It was initially a subclass; Danielle, Sjoerd and I argued about this in the office for quite some time, and ended up preferring the "view" design. The problem with subclassing TpChannel (as is done in telepathy-qt4, for instance) is that it makes it harder to have application-specific subclasses. If you want to make (say) a MaemoTpChannel with extra stuff bolted on (see librtcom-telepathy-glib for motivation), you end up needing to duplicate the class hierarchy. Because GObject doesn't do multiple inheritance, you'd also end up with MaemoTextChannel and MaemoTubeChannel (or whatever) not having a common ancestor other than TpChannel (mitigated by the fact that you could give them a common GInterface). Another problem with subclassing TpChannel is that everything that constructs a TpChannel of unspecified type behind the scenes will need some sort of channel factory which knows about all the TpChannel subclasses, which damages the ability to add channel types in a loosely-coupled way. If we add a channel factory, we need to be careful to have it not be a piece of global state. If two components of the same app both try to set the channel factory to give them their favourite component-specific subclasses, we need to avoid having one of them be overridden by the other and not get the subclass it asked for. As usual, the nastiest case is two independent Telepathy plugins dlopened by a non-Telepathy app. A further problem is that it's easy to get your assumptions violated by not having enough information at construction time. If you assume that every Text channel will be a TpTextChannel, and you ever create a TpChannel whose type isn't known yet, then you lose. TpChannelView avoids that by requiring that the TpChannel has the core feature prepared, i.e. it knows its own type already. As Danielle described it at the time (I haven't actually reviewed this branch), TpStreamTube doesn't do async preparation anyway, so the "double preparation" situation doesn't arise - you do make an async call to offer/accept the tube, but that doesn't really make sense to turn into a Feature, so it's unavoidable. Some unfortunate cases where we *would* have the double-preparation you mention are when TpTextChannelView downloads the pending message queue, or when TpCallChannelView gets the initial call state. > So, why not create subclass of TpChannel for high level API? We could add API > to TpBaseClient to say we are interested in those and even API to pass our own > factory if we want to create crazy subclass ourself. One interesting approach might be to have each place that creates channels behind the scenes take a GType as a parameter. If you already know you want stream tubes, tell it TP_TYPE_STREAM_TUBE_CHANNEL; if you're handling channels in a generic way, TP_TYPE_CHANNEL should be sufficient. (Or, we could define an object or callback that is a channel factory, and use that, but that requires that the factory knows about every subclass, which makes me sad from a loosely-coupled-objects point of view.) TpBaseClient would have to do this via tp_base_client_set_channel_gtype (TpBC *, GType), but that's not a problem; subclasses of TpBaseClient could call it in constructed(), and TpSimpleHandler etc. could let the caller call it at the same time that they add their channel filters. Where do we make new channels behind the scenes? At the moment I think it's: * TpBaseClient - easy, add another method as above * TpChannelDispatchOperation when it fetches the Channels D-Bus property - type or factory could be passed through from TpBaseClient when using one, but if you create a TpChannelDispatchOperation yourself, you're on your own and we'll soon add: * Guillaume's channel-requesting API from Bug #13422, which is secretly a TpBaseClient internally If we want to go back to subclassing, having weighed up advantages and disadvantages, I wonder whether we need the API in Bug #13422 to be encapsulated in an object (rather than being a standalone async call), so that we have an object on which to tweak "preferences" like which GType you want to end up with (without adding a million parameters to the initial async call, which doesn't scale).
(In reply to comment #6) > (In reply to comment #5) > > I'm not convinced by this channel-view approach. That means that in practice > > any application will have to create and prepare an extra object per channel > > when receiving them. > > It was initially a subclass; Danielle, Sjoerd and I argued about this in the > office for quite some time, and ended up preferring the "view" design. > > The problem with subclassing TpChannel (as is done in telepathy-qt4, for > instance) is that it makes it harder to have application-specific subclasses. > If you want to make (say) a MaemoTpChannel with extra stuff bolted on (see > librtcom-telepathy-glib for motivation), you end up needing to duplicate the > class hierarchy. > > Because GObject doesn't do multiple inheritance, you'd also end up with > MaemoTextChannel and MaemoTubeChannel (or whatever) not having a common > ancestor other than TpChannel (mitigated by the fact that you could give them a > common GInterface). Right, that's indeed an issue but I'm not convinced it's worth making the API more complicate to use for all the "normal" cases. > > So, why not create subclass of TpChannel for high level API? We could add API > > to TpBaseClient to say we are interested in those and even API to pass our own > > factory if we want to create crazy subclass ourself. > > One interesting approach might be to have each place that creates channels > behind the scenes take a GType as a parameter. If you already know you want > stream tubes, tell it TP_TYPE_STREAM_TUBE_CHANNEL; if you're handling channels > in a generic way, TP_TYPE_CHANNEL should be sufficient. That's what I had in mind when I mentioned adding API to let user tweak the channel creation. > (Or, we could define an object or callback that is a channel factory, and use > that, but that requires that the factory knows about every subclass, which > makes me sad from a loosely-coupled-objects point of view.) > > TpBaseClient would have to do this via tp_base_client_set_channel_gtype (TpBC > *, GType), but that's not a problem; subclasses of TpBaseClient could call it > in constructed(), and TpSimpleHandler etc. could let the caller call it at the > same time that they add their channel filters. > > Where do we make new channels behind the scenes? At the moment I think it's: a "git grep tp_channel_new" in telepathy-glib/telepathy-glib seems to confirm that's indeed the only places where tp-glib creates channels for us. > * TpBaseClient - easy, add another method as above Agreed, that seems an elegant solution. > * TpChannelDispatchOperation when it fetches the Channels D-Bus property - type > or factory could be passed through from TpBaseClient when using one, but if you > create a TpChannelDispatchOperation yourself, you're on your own I don't see any reason why you would want to create a CDO without using TpBaseClient. > and we'll soon add: > > * Guillaume's channel-requesting API from Bug #13422, which is secretly a > TpBaseClient internally > > If we want to go back to subclassing, having weighed up advantages and > disadvantages, I wonder whether we need the API in Bug #13422 to be > encapsulated in an object (rather than being a standalone async call), so that > we have an object on which to tweak "preferences" like which GType you want to > end up with (without adding a million parameters to the initial async call, > which doesn't scale). Very good point. An object could indeed be a solution (and we could also use it for "re-handle" notifications so no need of TpHandlerNotifier) but it makes the API more complicate to use... but I guess we can't win on both sides :\
Some quick comments: TpStreamTube is missing documentation (at the moment make check will fail), but I'm sure you knew that already. If we don't drop it, TpChannelView needs documentation too. > +#define GET_PRIV(o) (((TpChannelView *)o)->priv) I really dislike this idiom; it takes the type-safety you could have had by using self->priv (see TpBaseProtocol, for instance) and throws it away by casting o without a check, while being harder to type than self->priv. TpStreamTubeClass, TpChannelViewClass need ABI padding. The GAsyncResult idiom is that tp_stream_tube_offer_existing_async should have its own _finish function, I think? determine_socket_type() looks as though it should check for an access control type we understand, although we should probably declare that (IPv4, Localhost) is mandatory-to-implement, and if you support Unix sockets then you must support (Unix, Localhost). It'd probably be better for this code to prefer abstract Unix sockets on platforms that have them (Linux), and to avoid DoS/connection-stealing on multi-user systems, you should prefer Port (for IPv4) and Credentials (for Unix). Ability to test these is blocked by Bug #12999, though. > + g_set_error (error, TP_ERRORS, > + TP_ERROR_RESOURCE_UNAVAILABLE, > + "No supported socket types"); NotImplemented? Why does _channel_accepted call determine_socket_type() again? Surely the tube should remember which socket type (and access control mechanism) it used?
I opened bug #29973 to discuss channel factory plans.
The general consensus is now to make TpChannel sub-class so this branch should be adapted. Let me know if you want me to take over this bug.
I took over this branch and started to fix comments. http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/tp-stream-tube-29218 TpStreamTube now inherits from TpChannel. (In reply to comment #8) > Some quick comments: > > TpStreamTube is missing documentation (at the moment make check will fail), but > I'm sure you knew that already. I'll start documenting this (assuming we're happy with the API). > If we don't drop it, TpChannelView needs documentation too. dropped. > > +#define GET_PRIV(o) (((TpChannelView *)o)->priv) > > I really dislike this idiom; it takes the type-safety you could have had by > using self->priv (see TpBaseProtocol, for instance) and throws it away by > casting o without a check, while being harder to type than self->priv. removed. > TpStreamTubeClass, TpChannelViewClass need ABI padding. added. > The GAsyncResult idiom is that tp_stream_tube_offer_existing_async should have > its own _finish function, I think? Indeed. I added it. > determine_socket_type() looks as though it should check for an access control > type we understand, although we should probably declare that (IPv4, Localhost) > is mandatory-to-implement, and if you support Unix sockets then you must > support (Unix, Localhost). (IPv4, Localhost) is mandatory according to the spec. I think adding (Unix, Localhost) if we support Unix is fair enough (and fit the reality). I can cook a tp-spec patch if needed. > It'd probably be better for this code to prefer abstract Unix sockets on > platforms that have them (Linux), and to avoid DoS/connection-stealing on > multi-user systems, you should prefer Port (for IPv4) and Credentials (for > Unix). Ability to test these is blocked by Bug #12999, though. Right. I'd say to defer this until it's actually implemented in CM. > > + g_set_error (error, TP_ERRORS, > > + TP_ERROR_RESOURCE_UNAVAILABLE, > > + "No supported socket types"); > > NotImplemented? Changed. > Why does _channel_accepted call determine_socket_type() again? Surely the tube > should remember which socket type (and access control mechanism) it used? Fixed.
So, I implemented Credentials support which works fine. Then I tried to implement (IPv4, Port) support and hit an issue. We are supposed to pass the (ip, port) from where we are going to connect to Accept(). That means we should create the client socket before calling Accept which is not a big deal. But that raises another problem: this API doesn't let the accepter connect more than once to the tube. Maybe we should have _accept_async() and connect_async() with connect_finish() returning the GIOStream. We could have a accept_and_connect_async() as an helper for the most common case. That also means that we shouldn't use Port except if we know for sure that we'll have only one connection. Maybe accept_and_connect_async() should state that you can't use connect_async() later so we know that we'll have one connection and so can use Port?
Another thing it would be good to add is ConnectionClosed support: http://telepathy.freedesktop.org/spec/Channel_Type_Stream_Tube.html#org.freedesktop.Telepathy.Channel.Type.StreamTube.ConnectionClosed We have some options here: A) Expose the connection ID to the user and let it map it to the actual connection. Easy but not very elegant. B) fire something like connection-closed (GSocketConnection, TpError, const gchar *msg) C) Ideally it could be nice to have a new object TubeConnection inheriting from GSocketConnection but the object is actually either a GTcpConnection or a GUnixConnection so we have diamond inheritance problem :\
I added a GObject property and an accessor for the Parameters and Service properties. The remaning properties are: - SupportedSocketTypes: We shouldn't expose it expect if we want to let the user decide of the socket and access control to use. Which we probably don't want, TpStreamTube should choose the "best" available so I wouldn't clutter the API with such technical details. - Tube.State: not sure if we need this one. I'd be tempted to say that applications won't care much about it. Either they receive an incoming tube from a TpBaseClient or requested one using TpAccountChannelRequest. In both cases they already know its state and they just want to offer, accept or decline the tube. Offer() and Accept() will fail if the state is wrong any way. Furthermore, adding it will imply adding a a CORE feature for TpStreamTube and an preparation step. My vote would be to skip it for now, we can always add it later if we need to.
For the record, my tests work fine using glib 2.25.13 but failed with glib master because of those bugs: https://bugzilla.gnome.org/show_bug.cgi?id=629251 https://bugzilla.gnome.org/show_bug.cgi?id=629259
I removed the offer_existing API because it's untested and I think it's wrong. Let's focus on the "simple" use cases for now. Once we are happy with the API, I'll re-think about an API for the "already existing socket" case and use my wormux tube branch to test it. I also implemented the logic making use of the access_control_param arg of NewRemoteConnection to properly identify incoming connections. I have a question though. We can identify connections only when using the Credentials or Port access control. What should we do if those are not available? In that case we can't guaranteed that the TpContact we pass when firing the "incoming" signal is actually the one using the connection. Should we then pass NULL isntead as we can't safely guarantee that this TpContact is the right one?
(In reply to comment #15) > For the record, my tests work fine using glib 2.25.13 but failed with glib > master because of those bugs: > https://bugzilla.gnome.org/show_bug.cgi?id=629251 > https://bugzilla.gnome.org/show_bug.cgi?id=629259 They have been fixed (hurrah for reactive glib maintainers) so it's not longer an issue.
Some random comments looking at the branch as-is to get a feel for the shape of the API: > + [glib-2.0 >= 2.25.16, gobject-2.0 >= 2.25.16, gio-2.0 >= 2.25.16]) I'd rather not merge this until GLib 2.26 is out, but that should be quite soon. > + * stream-tube.h - a view for the TpChannel proxy No longer true. As per comments in Bug #30338 this should be TpStreamTubeChannel? The constructor should perhaps insta-invalidate the channel if Service isn't in the immutable properties? If it doesn't, then there should be a Feature for "Service is guaranteed to be available". > + * tp_stream_tube_offer_finish: This looks very boilerplatey. Can we make a macro for this? (I think Wocky has one?) > + return g_simple_async_result_get_op_res_gboolean (simple); Should always return TRUE, unless a GError has been set. This method is conceptually "void, throws TpError" rather than "returns gboolean, throws TpError". > + /* why doesn't GIO provide a method to create a socket we don't > + * care about? Factor out what this method would do and put it in util.h or util-internal.h? > + /* check there wasn't an error on the final attempt */ > + if (error != NULL) Never reached, I don't think? I think you need to clear the error at the *beginning* of the for loop, so that after the last iteration it'll still be set? > + G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_DEFAULT, Stream tubes over IP are defined to be TCP, so use G_SOCKET_PROTOCOL_TCP.
(In reply to comment #14) > - SupportedSocketTypes: We shouldn't expose it expect if we want to let the > user decide of the socket and access control to use. Which we probably don't I agree. > - Tube.State: not sure if we need this one. I'd be tempted to say that > applications won't care much about it. I agree.
(In reply to comment #12) > We are supposed to pass the (ip, port) from where we are going to connect to > Accept(). That means we should create the client socket before calling Accept > which is not a big deal. > But that raises another problem: this API doesn't let the accepter connect more > than once to the tube. Decisions a client needs to make include: * Am I going to let telepathy-glib make the connection, or hand it over to someone else? Vinagre has been adapted to Tubes, so it makes sense for it to get a GIOStream from Telepathy. A less well-adapted application might expect a GSocketAddress, but it'll never integrate very well like that; I think we'll basically only see this for a thin shim that invokes something with command-line arguments (e.g. an _http._tcp tube handler that runs "firefox http://127.0.0.1:4242/"). * If I'm going to hand the connection over to someone else, what does it support connecting to, and how can we do access-control? In practice, I think the answer will be (IPv4, Localhost). * Am I going to connect once, or repeatedly? HTTP is pseudo-connectionless, so you have to give your HTTP library (or web browser) a host:port and go from there. However, if you're well-integrated enough to be using GIOStream, it seems unlikely that you're using a protocol that works like that (it'd be too painful for a start!) I think we have two cases to deal with here: Fully-integrated Telepathy app ------------------------------ * Call accept_[and_connect_]async() and get a GIOStream * Internally, pass around a data structure containing the GIOStream and the TpStreamTubeChannel * Use whatever socket type telepathy-glib wants (Unix if supported, else IPv4) with the best possible access-control Handover to external process ---------------------------- * You need to know what socket types external processes can use, so nothing telepathy-glib can do will be good enough for you * Get the properties and call the Accept() tp_cli API yourself * You can handle multiple client connections, at your own risk
(In reply to comment #16) > I removed the offer_existing API because it's untested and I think it's wrong. > Let's focus on the "simple" use cases for now. > Once we are happy with the API, I'll re-think about an API for the "already > existing socket" case and use my wormux tube branch to test it. I think that sounds fair enough. Anything that needs to use an existing socket is unlikely to integrate with Tubes very well, in practice - e.g. logs will say "connection from 127.0.0.1:4242" rather than "connection from Guillaume". > I also implemented the logic making use of the access_control_param arg of > NewRemoteConnection to properly identify incoming connections. I have a > question though. We can identify connections only when using the Credentials or > Port access control. What should we do if those are not available? In that case > we can't guaranteed that the TpContact we pass when firing the "incoming" > signal is actually the one using the connection. Should we then pass NULL > isntead as we can't safely guarantee that this TpContact is the right one? Yes, I think so.
(In reply to comment #13) > C) Ideally it could be nice to have a new object TubeConnection inheriting from > GSocketConnection but the object is actually either a GTcpConnection or a > GUnixConnection so we have diamond inheritance problem :\ We could have TpStreamTubeConnection, which has a ref to the GSocketConnection and a ref to the TpStreamTubeChannel?
(In reply to comment #18) > Some random comments looking at the branch as-is to get a feel for the shape of > the API: > > > + [glib-2.0 >= 2.25.16, gobject-2.0 >= 2.25.16, gio-2.0 >= 2.25.16]) > > I'd rather not merge this until GLib 2.26 is out, but that should be quite > soon. nod. > > + * stream-tube.h - a view for the TpChannel proxy > > No longer true. fixed. > As per comments in Bug #30338 this should be TpStreamTubeChannel? renamed. > The constructor should perhaps insta-invalidate the channel if Service isn't in > the immutable properties? If it doesn't, then there should be a Feature for > "Service is guaranteed to be available". Check in constructed. It also checks the channel type. > > + * tp_stream_tube_offer_finish: > > This looks very boilerplatey. Can we make a macro for this? (I think Wocky has > one?) done (and fixed a bug in the macros). > > + return g_simple_async_result_get_op_res_gboolean (simple); > > Should always return TRUE, unless a GError has been set. This method is > conceptually "void, throws TpError" rather than "returns gboolean, throws > TpError". Leftover from Danni's code. removed. > > + /* why doesn't GIO provide a method to create a socket we don't > > + * care about? > > Factor out what this method would do and put it in util.h or util-internal.h? done. > > + /* check there wasn't an error on the final attempt */ > > + if (error != NULL) > > Never reached, I don't think? I think you need to clear the error at the > *beginning* of the for loop, so that after the last iteration it'll still be > set? done. > > + G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_DEFAULT, > > Stream tubes over IP are defined to be TCP, so use G_SOCKET_PROTOCOL_TCP. I tried but for some reason gio isn't happy about that :\ ERROR:stream-tube-chan.c:478:create_local_socket: assertion failed (err == NULL): Unable to create socket: Protocol not supported (g-io-error-quark, 0)
(In reply to comment #20) > * Am I going to let telepathy-glib make the connection, or hand it over to > someone else? > > Vinagre has been adapted to Tubes, so it makes sense for it to get a GIOStream > from Telepathy. A less well-adapted application might expect a GSocketAddress, > but it'll never integrate very well like that; I think we'll basically only see > this for a thin shim that invokes something with command-line arguments (e.g. > an _http._tcp tube handler that runs "firefox http://127.0.0.1:4242/"). Yeah except that Vino/Vinagre are not using GIOStream yet and Wormux for example will never use it even if we add good (optionnal) Telepathy integration. So I think it's important to keep this case in mind as it will probably be the main case actually. > * If I'm going to hand the connection over to someone else, what does it > support connecting to, and how can we do access-control? > > In practice, I think the answer will be (IPv4, Localhost). IPv4 for now and hopefully IPv6 in some future. I'm not sure for LocalHost. If I'm offering a tube, using Port totally makes sense as it doesn't affect the "protocol" and allow us to identify incoming connections. Oth, as you can see in my tp-glib implementation, breaking the races is actually far from being trivial. :\ I suspect the answer here "If you want to use a Muc Stream tube, then you have to use GIOStream and use the 'simple' API to be able to identify connections". > * Am I going to connect once, or repeatedly? > > HTTP is pseudo-connectionless, so you have to give your HTTP library (or web > browser) a host:port and go from there. However, if you're well-integrated > enough to be using GIOStream, it seems unlikely that you're using a protocol > that works like that (it'd be too painful for a start!) Makes sense. > I think we have two cases to deal with here: > > Fully-integrated Telepathy app > ------------------------------ > > * Call accept_[and_connect_]async() and get a GIOStream > * Internally, pass around a data structure containing the GIOStream and the > TpStreamTubeChannel > * Use whatever socket type telepathy-glib wants (Unix if supported, else IPv4) > with the best possible access-control That's what I implemented currently. Does this API looks good to you? > Handover to external process > ---------------------------- > > * You need to know what socket types external processes can use, so nothing > telepathy-glib can do will be good enough for you > * Get the properties and call the Accept() tp_cli API yourself > * You can handle multiple client connections, at your own risk I still think we should have high level API for this case. If only because tp_cli_* is not accessible in other language.
(In reply to comment #18) > > + * tp_stream_tube_offer_finish: > > This looks very boilerplatey. Can we make a macro for this? (I think Wocky has > one?) Anyone feel like proposing Wocky's macros for glib?
(In reply to comment #25) > (In reply to comment #18) > > > + * tp_stream_tube_offer_finish: > > > > This looks very boilerplatey. Can we make a macro for this? (I think Wocky has > > one?) > > Anyone feel like proposing Wocky's macros for glib? I'll have a look into it, this should not block this review though, most sources implements _finish() manually.
(In reply to comment #26) > (In reply to comment #25) > > Anyone feel like proposing Wocky's macros for glib? > > I'll have a look into it, this should not block this review though, most > sources implements _finish() manually. Actually those calls have issues that may prevent them from being included. 1. It's wrong to call g_simple_async_result_propagate_error() before g_simple_async_result_is_valid() 2. A copy_func may not support NULL (e.g. g_object_ref), use case found in g_socket_address_enumerator_next_finish(). Aside those, I don't see any major issues. I'll propose a fix to Wocky before trying to get those into GLib.
(In reply to comment #22) > (In reply to comment #13) > > C) Ideally it could be nice to have a new object TubeConnection inheriting from > > GSocketConnection but the object is actually either a GTcpConnection or a > > GUnixConnection so we have diamond inheritance problem :\ > > We could have TpStreamTubeConnection, which has a ref to the GSocketConnection > and a ref to the TpStreamTubeChannel? Done but I didn't add the ref on the TpStreamTubeChannel yet and report Connection errors.
> Done but I didn't add the ref on the TpStreamTubeChannel yet and report > Connection errors. This is now done. I think all the API is now implemented. Could you review it please? Once we're happy with it I'll finish to document it.
The stuff in gnio-util.c depends on GUnixConnection, which makes it troublesome to put in our API (GUnixConnection only conditionally exists). The functions should be underscore-prefixed and in gnio-util-internal.h, or a new unix-internal.h; they should only be defined if HAVE_GIO_UNIX. If they're needed by something outside telepathy-glib, we'll have to introduce a wrapper function which always exists, like this: gboolean tp_unix_connection_send_credentials_with_byte (GSocketConnection *sock, guchar byte, GCancellable *cancellable, GError **error) { #ifdef HAVE_GIO_UNIX return _tp_unix_connection_send_credentials_with_byte ( G_UNIX_CONNECTION (sock), byte, cancellable, error); #else g_set_error (G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, "Unix sockets not supported"); return FALSE; #endif }
The general shape of the API looks fine. Some notes about things I happened to spot: > +static void > +tp_stream_tube_channel_constructed (GObject *obj) I noticed this doesn't chain up. It should do that first. > + * @stream: (transfer none): the #TpStreamTubeConnection for the connection I don't think (transfer) is applicable to objects in signals: the signal machinery always takes one ref for the duration of the signal emission, and the recipient needs to ref it if they want to keep it. It's worth mentioning it explicitly in the text though, like you did. > + if (n_failed > 0) > + { > + DEBUG ("Failed to prepare TpContact (unspecified error)"); If error == NULL and n_failed > 0, that unambiguously means the failed handles failed with InvalidHandle. Improved docs wording would be welcome. We ought to introduce a signal with contact IDs to avoid the possibility of InvalidHandle, really... > + /* FIXME: set the address of the socket. Gio doesn't seem to have API > + * to get the port before connecting without specifying the whole > + * adress (we can't as we don't know which ports are available). */ You have to do a GIO-flavoured version of this pseudocode: sock = socket (AF_INET, SOCK_STREAM, 0); bind (sock, addr={ AF_INET, sin_port=0, sin_addr=IN_ADDR_ANY }, addr_len); getsockname (sock, &real_addr, real_addr_len); /* time passes */ connect (sock, blah blah blah); I think the GIO version of that might be to make the GSocketClient earlier, call g_socket_client_set_local_address() for IN_ADDR_ANY with port 0, then call g_socket_client_get_local_address() to find out what the kernel actually gave us? > + /* Pass the ref on tube_conn to the callback */ > + > + tp_connection_get_contacts_by_handle ( > + tp_channel_borrow_connection (TP_CHANNEL (self)), > + 1, &handle, 0, NULL, > + _new_remote_connection_with_contact, > + tube_conn, NULL, G_OBJECT (self)); You can't rely on the callback being called if you pass a weak object - if @self dies, the callback won't happen. If you just want to unref tube_conn in the "@self died" case, you can pass g_object_unref as the destructor here instead of releasing the ref in the callback? > + g_assert (self->priv->connection != NULL); TP_IS_CONNECTION? > + * tp_stream_tube_connection_get_connection: (skip) I wonder whether tp_stream_tube_connection_get_socket() would be better, given that TpConnection is a prominent part of our API? :-) _tp_create_temp_unix_socket looks like an ideal candidate for putting in GIO.
(In reply to comment #30) > The stuff in gnio-util.c depends on GUnixConnection, which makes it troublesome > to put in our API (GUnixConnection only conditionally exists). > > The functions should be underscore-prefixed and in gnio-util-internal.h, or a > new unix-internal.h; they should only be defined if HAVE_GIO_UNIX. > > If they're needed by something outside telepathy-glib, we'll have to introduce > a wrapper function which always exists, like this: I added a wrapper as these functions are used by tests as well (stream-tube-chan.c). (In reply to comment #31) > The general shape of the API looks fine. Cool, I will start documenting it then. > > +static void > > +tp_stream_tube_channel_constructed (GObject *obj) > > I noticed this doesn't chain up. It should do that first. done. > > + * @stream: (transfer none): the #TpStreamTubeConnection for the connection > > I don't think (transfer) is applicable to objects in signals: the signal > machinery always takes one ref for the duration of the signal emission, and the > recipient needs to ref it if they want to keep it. It's worth mentioning it > explicitly in the text though, like you did. annotation removed. > > + if (n_failed > 0) > > + { > > + DEBUG ("Failed to prepare TpContact (unspecified error)"); > > If error == NULL and n_failed > 0, that unambiguously means the failed handles > failed with InvalidHandle. Improved docs wording would be welcome. fixed. > > + /* FIXME: set the address of the socket. Gio doesn't seem to have API > > + * to get the port before connecting without specifying the whole > > + * adress (we can't as we don't know which ports are available). */ > > You have to do a GIO-flavoured version of this pseudocode: > > sock = socket (AF_INET, SOCK_STREAM, 0); > bind (sock, addr={ AF_INET, sin_port=0, sin_addr=IN_ADDR_ANY }, addr_len); > getsockname (sock, &real_addr, real_addr_len); > /* time passes */ > connect (sock, blah blah blah); > > I think the GIO version of that might be to make the GSocketClient earlier, > call g_socket_client_set_local_address() for IN_ADDR_ANY with port 0, then call > g_socket_client_get_local_address() to find out what the kernel actually gave > us? Unfortunatelly that doesn't work :( I opened https://bugzilla.gnome.org/show_bug.cgi?id=631316 about this issue. > > + /* Pass the ref on tube_conn to the callback */ > > + > > + tp_connection_get_contacts_by_handle ( > > + tp_channel_borrow_connection (TP_CHANNEL (self)), > > + 1, &handle, 0, NULL, > > + _new_remote_connection_with_contact, > > + tube_conn, NULL, G_OBJECT (self)); > > You can't rely on the callback being called if you pass a weak object - if > @self dies, the callback won't happen. If you just want to unref tube_conn in > the "@self died" case, you can pass g_object_unref as the destructor here > instead of releasing the ref in the callback? fixed. > > + g_assert (self->priv->connection != NULL); > > TP_IS_CONNECTION? G_IS_SOCKET_CONNECTION actually :) > > + * tp_stream_tube_connection_get_connection: (skip) > > I wonder whether tp_stream_tube_connection_get_socket() would be better, given > that TpConnection is a prominent part of our API? :-) I renamed it to socket-connection to avoid confusion with TpConnection and GSocket. > _tp_create_temp_unix_socket looks like an ideal candidate for putting in GIO. I opened https://bugzilla.gnome.org/show_bug.cgi?id=631314
(In reply to comment #32) > > > + /* FIXME: set the address of the socket. Gio doesn't seem to have API > > > + * to get the port before connecting without specifying the whole > > > + * adress (we can't as we don't know which ports are available). */ > > > > You have to do a GIO-flavoured version of this pseudocode: > > > > sock = socket (AF_INET, SOCK_STREAM, 0); > > bind (sock, addr={ AF_INET, sin_port=0, sin_addr=IN_ADDR_ANY }, addr_len); > > getsockname (sock, &real_addr, real_addr_len); > > /* time passes */ > > connect (sock, blah blah blah); > > > > I think the GIO version of that might be to make the GSocketClient earlier, > > call g_socket_client_set_local_address() for IN_ADDR_ANY with port 0, then call > > g_socket_client_get_local_address() to find out what the kernel actually gave > > us? > > Unfortunatelly that doesn't work :( I opened > https://bugzilla.gnome.org/show_bug.cgi?id=631316 about this issue. I managed to do it using a lower level API (GSocket). I added documentation so I think the branch is now ready for review. I have this error when checking the doc but have no idea why: 0 symbols incomplete. 0 not documented. copy_func ^^^ Unused symbols Documentation check failed
gnio-util.c > +#include <gio/gunixcredentialsmessage.h> Does this still exist when we don't have gio-unix? > +G_DEFINE_TYPE (TpStreamTubeChannel, tp_stream_tube_channel, TP_TYPE_CHANNEL); Nitpicking: trailing semicolon here isn't needed, isn't valid C99 and makes some compilers complain. > + g_hash_table_foreach_remove (self->priv->tube_connections, is_connection, > + conn); Nitpicking: could use GHashTableIter and g_hash_table_iter_remove()? > +tp_stream_tube_channel_dispose (GObject *obj) ... > + g_cancellable_cancel (self->priv->cancellable); I think this deserves a comment, at least. Why do you do this? Is the idea that if the channel is disposed while an async call is in flight, it'll automatically cancel itself? (I would have thought that the channel would hold one ref to itself for the duration of an async call?) > + case TP_SOCKET_ADDRESS_TYPE_UNIX: > + family = G_SOCKET_FAMILY_UNIX; > + break; I think this needs an "it's Unix" guard. > +++ b/telepathy-glib/util-internal.h ... > +#ifdef HAVE_GIO_UNIX If you check HAVE_SOMETHING in a file, then that file should include "config.h" at the top, and cannot be installed. (This is an uninstalled header, so that's OK.) One alternative would be to have _tp_create_temp_unix_socket exist even on non-Unix, but always fail with whatever the GLib equivalent of ENOTSUPP is. > + /* FIXME: Actually TP_SOCKET_ACCESS_CONTROL_CREDENTIALS is also able to > + * properly identify connections and our code should be able to. > + * But we can't test it as we currently use sync calls to send and > + * receive credentials. We should change that once bgo #629503 > + * has been fixed. */ Would it be possible to test this by ensuring that the send happens before the receive, and assuming that there's enough send-buffer space for one byte?
(In reply to comment #34) > gnio-util.c > > +#include <gio/gunixcredentialsmessage.h> > > Does this still exist when we don't have gio-unix? Probably not. Fixed. > > +G_DEFINE_TYPE (TpStreamTubeChannel, tp_stream_tube_channel, TP_TYPE_CHANNEL); > > Nitpicking: trailing semicolon here isn't needed, isn't valid C99 and makes > some compilers complain. Removed. > > + g_hash_table_foreach_remove (self->priv->tube_connections, is_connection, > > + conn); > > Nitpicking: could use GHashTableIter and g_hash_table_iter_remove()? done. > > +tp_stream_tube_channel_dispose (GObject *obj) > ... > > + g_cancellable_cancel (self->priv->cancellable); > > I think this deserves a comment, at least. Why do you do this? Is the idea that > if the channel is disposed while an async call is in flight, it'll > automatically cancel itself? Actually we don't use this cancellable, I removed it. > (I would have thought that the channel would hold one ref to itself for the > duration of an async call?) Indeed. > > + case TP_SOCKET_ADDRESS_TYPE_UNIX: > > + family = G_SOCKET_FAMILY_UNIX; > > + break; > > I think this needs an "it's Unix" guard. done. > > +++ b/telepathy-glib/util-internal.h > ... > > +#ifdef HAVE_GIO_UNIX > > If you check HAVE_SOMETHING in a file, then that file should include "config.h" > at the top, and cannot be installed. (This is an uninstalled header, so that's > OK.) done. > One alternative would be to have _tp_create_temp_unix_socket exist even on > non-Unix, but always fail with whatever the GLib equivalent of ENOTSUPP is. > > > + /* FIXME: Actually TP_SOCKET_ACCESS_CONTROL_CREDENTIALS is also able to > > + * properly identify connections and our code should be able to. > > + * But we can't test it as we currently use sync calls to send and > > + * receive credentials. We should change that once bgo #629503 > > + * has been fixed. */ > > Would it be possible to test this by ensuring that the send happens before the > receive, and assuming that there's enough send-buffer space for one byte? done.
Ship it!
Merged to master. Will be in 0.13.2
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.