[Bug 34931] Add a "meta porter"

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-03-14 07:42:29 PDT ---
(In reply to comment #2)
> All the other such macros in this file use G_STMT_START and G_STMT_END. (They
> didn't when I first wrote them, but they do now, apparently.) It'd be nice to
> be consistent.

Done.

> Should wocky_stanza_set_contact() override one of from/to? I wonder if there
> should be two separate contact objects, one from-contact and one to-contact.
> Then we could have the invariant that, if from-contact is defined,
> wocky_stanza_get_from() ==
> wocky_contact_get_jid(wocky_stanza_get_from_contact()).

Done.

> It would be nice to spell out the abbreviation in docstrings.

Done.

> + * Returns a #GList of #GInetSocketAddress<!-- -->es which relate to
> + * @self. Note that the #GInetSocketAddresses should be unreffed by
> + * calling g_object_unref() on each list member and the list freed
> + * using g_list_free() when the caller is finished.
> + *
> + * Returns: a new #GList of #GInetSocketAddress<!-- -->es.
> 
> I think this is spelled "(element-type GInetSocketAddress) (transfer full)"?

Done.

> And what does “which relate to @self” mean? Obviously they relate to @self
> since I can get them out of it. Does it mean “at which we can communicate with
> @self” or similar?

Fixed.

> +/* Called when a WockyBareContact, WockyResourceContact or
> + * WockyLLContact has been disposed so we can remove it from his hash
> + * table. */
> 
> *this? But I think this typo (which you've preserved) is funny so maybe you
> left it in intentionally?

Haha, yeah I didn't do this intentionally but I agree it should be left. :-)

> Why does the *same* factory know about LL contacts and normal contacts?

The contact factory is so boring and abstract; it only holds a few objects and
a couple of weak refs. I was going to write my own but found there was one
there already managing both resource contacts and bare contacts. I think it's
fine. :-)

> “Perhaps this should be called ll-connection-factory, to emphasize the
> link-local emphasis?”
> 
> I would tend to agree I think.

Renamed.

> I think it might be clearer if the connection factory had a GQueue of
> GInetSocketAddress and popped each one it tries off the head.

Done.

> Also I think it
> should check whether the cancellable has been cancelled: right now, if you
> cancel, it'll still go on to try connecting to each remaining
> GInetSocketAddress (which will fail immediately).

Done.

> + */
> +WockyXmppConnection *
> +wocky_connection_factory_make_connection_finish (
> +    WockyConnectionFactory *self,
> +    GAsyncResult *result,
> +    GError **error)
> +{
> +  wocky_implement_finish_return_pointer (self,
> +      wocky_connection_factory_make_connection_async);
> +}
> 
> No, the GSimpleAsyncResult should own a ref on the WockyXmppConnection, and
> this function should return a ref to it. It's completely legitimate for the
> caller to never actually call _make_connection_finish(), in which case you'll
> leak the connection. (I was wondering why you wanted the new macro.)

That's not quite right. The connection returned actually belongs to the
connection factory. If you call make_connection_finish you get the connection,
but if you don't ref it before unreffing the connection factory it will be
lost. This is the exact same behaviour as the connector.

So, you want this changed? And in the connector too?

> + * Requests an asynchronous connect using the stream or connection
> + * passed to one of the new functions.
> 
> Cite “the new functions” explicitly. Right now it sounds like you're saying “to
> one of an unnamed set of functions which were recently added”.

Haha yes that is somewhat confusing. Fixed.

> Can't these three:
> 
> +WockyLLConnector * wocky_ll_connector_new (GIOStream *stream);
> +
> +WockyLLConnector * wocky_ll_connector_new_from_connection (
> +    WockyXmppConnection *connection,
> +    const gchar *jid);
> +
> +void wocky_ll_connector_connect_async (WockyLLConnector *connector,
> +    const gchar *remote_jid,
> +    GCancellable *cancellable,
> +    GAsyncReadyCallback cb,
> +    gpointer user_data);
> 
> be replaced by a GAsyncInitable implementation, and two wrappers around
> g_async_initable_init_async, namely wocky_ll_connector_new_outgoing_async
> (WockyXmppConnection *, const gchar *local_jid, const gchar *remote_jid, ...)
> and wocky_ll_connector_new_incoming_async (GIOStream *stream, ...)? This would
> make their usage more self-documenting.

Done.

> You should chain up as the first thing you do. It won't make any difference
> here probably, though.

Done.

> do we not send our own stream features in the incoming case?

Good catch, fixed.

> What happens if an incoming connection arrives in the gap? A valid answer is
> “it can't happen” :)

Yeah it can't happen. Contacts can only get your address once you've published
it in the appropriate avahi record which is when you call set_jid.

> What happens if you change the JID after the loopback porter has been created?

A new loopback porter is created when set_jid is called. I'm not sure about
this because in the worst case you call set_jid n times, there will be n extra
loopback porters in the hash table (which will all be closed and cleaned up
when the porter is closed). On the other hand, if it is processing stanzas,
these could be lost if we bin the porter when changing JID. At the moment,
nothing will be lost. What do you think?

> I think this is a poor name for the function. It sounds like a synonym for
> g_object_ref. Maybe _hold() and _unhold()?

OK, done.

> > Perhaps this should be called WockyLoopbackStream?
> 
> yes!

Done.

Updated branch and pushed.

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