[Bug 29218] TpStreamTube - high level stream tube API

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29218

Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |http://git.Collabora.co.uk/
                   |?p=user/danni/telepathy-gli |?p=user/cassidy/telepathy-g
                   |b.git;a=shortlog;h=refs/hea |lib;a=shortlog;h=refs/heads
                   |ds/tp-stream-tube           |/tp-stream-tube-29218

--- Comment #11 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-09-06 05:26:06 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.