[Bug 34931] Add a "meta porter"

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Will Thompson <will.thompson at collabora.co.uk> 2011-03-11 13:38:40 PST ---
+#define wocky_implement_finish_return_pointer(source, tag) \
+    GSimpleAsyncResult *_simple; \
+    _simple = (GSimpleAsyncResult *) result; \
+    if (g_simple_async_result_propagate_error (_simple, error)) \
+      return NULL; \
+    g_return_val_if_fail (g_simple_async_result_is_valid (result, \
+            G_OBJECT (source), tag), \
+        NULL); \
+    return g_simple_async_result_get_op_res_gpointer (_simple);
+

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.

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()).

+  /**
+   * WockyLLContact:jid:
+   *
+   * The contact's ll JID.
+   */

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


+ * 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)"?
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?

+/* 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?

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

“Perhaps this should be called ll-connection-factory, to emphasize the
link-local emphasis?”

I would tend to agree I think.

I think it might be clearer if the connection factory had a GQueue of
GInetSocketAddress and popped each one it tries off the head. 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).

+ */
+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.)

+ * wocky_ll_connector_connect_finish:
...
+ * After this function is called, @connector can be freed using
+ * g_object_unref(). Note that the connection returned from this
+ * function must be reffed using g_object_ref() before freeing the
+ * connector otherwise the connection will be disposed and will close.

No! It should return a ref to the connection. Then none of this documentation
would be needed.


+ * 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”.

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.

+static void
+wocky_ll_connector_constructed (GObject *object)
+{
+  WockyLLConnector *self = WOCKY_LL_CONNECTOR (object);
+  WockyLLConnectorPrivate *priv = self->priv;
+
+  if (priv->connection == NULL)
+    priv->connection = wocky_xmpp_connection_new (priv->stream);
+
+  if (G_OBJECT_CLASS (wocky_ll_connector_parent_class)->constructed)
+    G_OBJECT_CLASS (wocky_ll_connector_parent_class)->constructed (object);


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

+  else
+    {
+      DEBUG ("successfully sent stream open, connection now open");
+
+      g_simple_async_result_complete (priv->simple);
+    }

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


> session: add set_jid function
> This is useful for being able to create a link-local session (and
> therefore the meta porter) and start listening on a port without
> knowing your JID properly yet. E.g.

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

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

+ * wocky_meta_porter_ref:
+ * @porter: a #WockyMetaPorter
+ * @contact: a #WockyContact

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

> Perhaps this should be called WockyLoopbackStream?

yes!

~~~~~

I have not fully reviewed these three patches:

• meta-porter: added files
• loopback-connection: added files
• tests: add a simple test for the loopback connection

I shall come back to them.

-- 
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.