Bug 34931

The salut bug is bug #34932.

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

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

- * 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 #GList of #GInetSocketAddress<!-- -->es which are
+ * advertised by the contact @self as addresses to connect on. 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: (element-type GInetSocketAddress) (transfer full): a new
  *   #GList of #GInetSocketAddress<!-- -->es.

I really meant to add the annotation and remove the longhand description of it from the docstring; but okay.


-  data->addresses = wocky_ll_contact_get_addresses (contact);
+  data->addresses = g_queue_new ();
+
+  addr = wocky_ll_contact_get_addresses (contact);
+  g_list_foreach (addr, add_to_queue, data->addresses);
+  g_list_free (addr);

Would it be too evil to manually initialize the queue's members? :p (I'm disappointed that GQueue doesn't have an append_list() method which takes ownership of an existing GList. Oh well.) You actually don't need to use g_queue_new() — if you're into this kind of thing you can put a GQueue (not a GQueue *) in your private structure — but no big deal if you're not. Much as I like foreach functions, I think explicit iteration would be clearer to copy the list into the queue, but again if you're not feeling keen don't worry about it too much.


@@ -1558,7 +1558,7 @@ open_porter (WockyMetaPorter *self,
  * Make an asynchronous request to open a connection to @contact if
  * one is not already open. The reference count of the porter to
  * @contact will be incrememented and so after completion
- * wocky_meta_porter_unref() should be called on contact to release
+ * wocky_meta_porter_unhold() should be called on contact to release
  * the reference.

If I were a pedant I would ask for the documentation to not talk about references, given that the functions no longer talk about references. But NBD.


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

Yes. The GIO convention is that _finish() functions return new refs. (If existing Wocky code gets this wrong, it's wrong too!) I think this would mean you wouldn't need the new non-copying macro.

I'd call the connector functions wocky_ll_connector_new_incoming_async(), wocky_ll_connector_new_outgoing_async(), wocky_ll_connector_new_finish().

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

As we discussed on IRC — maybe we should just document that you should only
call this once, and warn if it's called more than once. :)

Time to review the original patches I skipped…

This is a review of wocky-meta-porter.[ch] in its (gargantuan) entirety. I still haven't reviewed the loopback stream.

 ~~~

This file is *enormous*. I wonder whether parts of it could be split out into separate files to help keep it manageable.

  if (p->porter != NULL)
    {
      g_object_weak_unref (G_OBJECT (p->porter),
          porter_disposed, data);

      g_object_unref (p->porter);
    }

If PorterData owns a ref to its porter field, why does it also weak-ref it? It's pretty exciting how this file contains a function called porter_disposed() and another called porter_disposed_cb().

  if (!wocky_porter_close_finish (porter, result, &error))
    {
      DEBUG ("Failed to close porter: %s", error->message);
      g_clear_error (&error);
    }
  else
    {
      DEBUG ("Closed porter to '%s'", data->jid);
    }

If it's worth including the jid on one branch, it's worth including it on the other. I also do wonder what we should do if closing a porter fails (for a reason other than, say, ‘already closed’) — force close it?

  if (data->porter != NULL)
    g_object_unref (data->porter);
  data->porter = NULL;

‘g_clear_object (&data->porter)’.

  /* weak reffed WockyPorter* => handler ID */
  GHashTable *porters;

  DEBUG ("porter closed by remote, remove it from our records");

but it doesn't actually seem to be removed from the dictionary of porters. “It'll be removed because it's weak-reffed” isn't a good idea — you don't want to rely on that kind of side-effect. I think the hash table key should be a reffed porter.

(Reading this comment again, I think I may be confused between the various dictionaries of porters …)

  /* maybe start the timeout */
  if (data->refcount == 0)
    {
      DEBUG ("Started porter timeout...");
      data->timeout_id = g_timeout_add_seconds (5, porter_timeout_cb, data);
    }

This code is duplicated; add maybe_start_timeout().

  typedef struct
  {
    WockyMetaPorter *self;
    GInetAddress *addr;
  } NewConnectionData;

If you were feeling keen you could fish 'addr' out of the WockyXmppConnection returned by wocky_ll_connector_finish() and then you wouldn't need a context struct.

loopback_recv_open_cb and loopback_sent_open_cb both leak on the error path.

I think that any existing loopback porter should be torn down if set_jid() is called with an existing JID — either that or it should g_return_if_fail().

  /* Convenience function to call @callback with a porter and do all the
   * handling the creating a porter if necessary. Also, omg two user datas! */
  static void
  open_porter_if_necessary (WockyMetaPorter *self,
      WockyLLContact *contact,
      GCancellable *cancellable,
      OpenPorterIfNecessaryFunc callback,
      gpointer user_data1,
      gpointer user_data2)

Every caller of this function passes a GSimpleAsyncResult as the first user_data. Maybe its signature should reflect that.

  PorterData *porter = g_hash_table_lookup (priv->porters, contact);

  if (porter != NULL && porter->porter != NULL)

This perhaps should suggest that the name of the local variable is wrong. ;-)

  data = g_slice_new0 (OpenPorterData);
  data->self = self;
  data->contact = g_object_ref (contact);
  data->callback = callback;
  data->cancellable = cancellable;
  data->user_data1 = user_data1;
  data->user_data2 = user_data2;

If user_data1 was a GSimpleAsyncResult, you wouldn't need to hang onto self in data: you could get_self_object() on the GSAR.

  /* stamp on from if there is none */
  if (wocky_node_get_attribute (wocky_stanza_get_top_node (stanza),
          "from") == NULL)

This is secret code for ‘wocky_stanza_get_from (stanza) == NULL’.

static void
send (WockyMetaPorter *self,
    WockyPorter *porter,
    GCancellable *cancellable,
    const GError *error,
    gpointer user_data1,
    gpointer user_data2)

send is a terrible name for this function. How about:

  meta_porter_send_cb()
  meta_porter_send_got_porter_cb()
  wocky_meta_porter_send_async()

for this chain of functions?

  if (error != NULL)
    {
      DEBUG ("Failed to listen: %s", error->message);
      g_clear_error (&error);
    }

  DEBUG ("listening on port %u", port);

  g_socket_service_start (G_SOCKET_SERVICE (priv->listener));

  priv->port = port;

Are you missing a 'return' there?

In porter_handler_cb:

  from = wocky_node_get_attribute (wocky_stanza_get_top_node (stanza), "from");

  contact = wocky_contact_factory_ensure_ll_contact (
      priv->contact_factory, from);

Also secret code; and, is it guaranteed to be non-NULL?

  handler = g_slice_new0 (StanzaHandler);
  handler->self = self;
  handler->contact = g_object_ref (from);
  handler->porters = g_hash_table_new (g_direct_hash, g_direct_equal);

Pass NULL, NULL to g_hash_table_new(). You should also have a function that creates a StanzaHandler struct and stuffs it into the handlers hash table, rather than duplicating that code.

  /* there were no porters to close anyway */
  if (num == 0)
    {
      g_simple_async_result_complete (simple);
      g_object_unref (simple);
    }

This leaks the ClosePorterData structure. Instead, have a close_porters_maybe_complete() function called from here and from porter_close_cb().

    g_simple_async_result_set_op_res_gpointer (simple, stanza, NULL);

is a no-op.

  void
  wocky_meta_porter_open_async (WockyMetaPorter *self,
      WockyContact *contact,
      ^^^^^^^^^^^^
      GCancellable *cancellable,
      GAsyncReadyCallback callback,
      gpointer user_data)
  {
    GSimpleAsyncResult *simple;

    g_return_if_fail (WOCKY_IS_META_PORTER (self));
    g_return_if_fail (WOCKY_IS_LL_CONTACT (contact));
                               ^^

If the function requires a WockyLLContact then its signature should say so. Ditto functions like wocky_meta_porter_borrow_connection().

  /**
   * wocky_meta_porter_open_finish:
   * @porter: a #WockyMetaPorter
   * @result: the #GAsyncResult
   * @error: an optional #GError location to store an error message
   *
   * Finishes an asynchronous request to open a connection to @contact

This function has no contact argument:                        ^^^^^^^^

  /**
   * wocky_meta_porter_borrow_connection:
   * @porter: a #WockyMetaPorter
   * @contact: the #WockyContact
   *
   * Borrow the #GSocketConnection of the porter to @contact, if one
   * exists otherwise %NULL will be returned. Note that the connection
   * returned is not reffed and should not be kept as it still is owned

                                              ^^^^

Well, the caller can keep it if they want, by reffing it, but they should be aware that it might be disconnected spontaneously unless they hold() the corresponding contact.

   * and operated on by the underlying #WockyXmppConnection object.
   *
   * Returns: the #GSocketConnection or %NULL if no connection is open
   */

I'd be inclined to make this return a ref, too, to be honest. I'm not really all that hung up on it, but code that unrefs a pointer and then returns that same pointer looks suspect. :)

(It also be nice to have accessors for C2SPorter:connection and XmppConnetion:base-stream so that you don't have to use g_object_get() in the first place and then this would look less suspect.)

(In reply to comment #4)
> If I were a pedant I would ask for the documentation to not talk about
> references, given that the functions no longer talk about references. But NBD.

You *are* a pedant! Fixed

> Yes. The GIO convention is that _finish() functions return new refs. (If
> existing Wocky code gets this wrong, it's wrong too!) I think this would mean
> you wouldn't need the new non-copying macro.

I opened bug #35351 about WockyConnector.

The macro is used in the link-local connector which returns a new ref (the only ref) to the connection is creates but doesn't reference it anywhere else so won't disappear when you free the connector. So this is fine.

The macro is also used in the meta porter's send_iq_finish function as it passes the new reply stanza down. This is fine too.

> I'd call the connector functions wocky_ll_connector_new_incoming_async(),
> wocky_ll_connector_new_outgoing_async(), wocky_ll_connector_new_finish().

So I implemented the three functions, as you probably saw, but I don't like _new_ because we don't actually want a WockyLLConnector object. As a result, you never actually get the said object anywhere. Ignoring function names, what's wrong with this:

void wocky_ll_connector_incoming_async (
    GIOStream *stream,
    GCancellable *cancellable,
    GAsyncReadyCallback callback,
    gpointer user_data);

void wocky_ll_connector_outgoing_async (
    WockyXmppConnection *connection,
    const gchar *local_jid,
    const gchar *remote_jid,
    GCancellable *cancellable,
    GAsyncReadyCallback callback,
    gpointer user_data);

WockyXmppConnection * wocky_ll_connector_finish (
    WockyLLConnector *connector,
    GAsyncResult *result,
    gchar **from,
    GError **error);

The finish returns a new ref to the connection, of course.

> As we discussed on IRC — maybe we should just document that you should only
> call this once, and warn if it's called more than once. :)

OK, done.

+/*
+ * wocky-loopback-connection.h - Header for WockyLoopbackConnection
+ * Copyright (C) 2009-2011 Collabora Ltd.

O rly?            ^^^^

Oh, you said on IRC that this file is copy-pasta from the test suite. Okay then. It looks fine (as does the test).

(In reply to comment #5)
> This is a review of wocky-meta-porter.[ch] in its (gargantuan) entirety. I
> still haven't reviewed the loopback stream.

The loopback stream is literally a copy of the test stream with the concept of multiple streams removed. I'm not going to pretend I understand how it works, I just did some renaming and moving about.

> If PorterData owns a ref to its porter field, why does it also weak-ref it?
> It's pretty exciting how this file contains a function called porter_disposed()
> and another called porter_disposed_cb().

Eek, not cool. Sorry this was an old design which was fixed but I obviously didn't remove the weak-reffing. I've removed one and renamed the other to clear up the situation

> If it's worth including the jid on one branch, it's worth including it on the
> other.

Ah yes, done.

> I also do wonder what we should do if closing a porter fails (for a
> reason other than, say, ‘already closed’) — force close it?

Well isn't that what force_close_async is for?

>   if (data->porter != NULL)
>     g_object_unref (data->porter);
>   data->porter = NULL;
> 
> ‘g_clear_object (&data->porter)’.

I've left this for now.

> but it doesn't actually seem to be removed from the dictionary of porters.
> “It'll be removed because it's weak-reffed” isn't a good idea — you don't want
> to rely on that kind of side-effect. I think the hash table key should be a
> reffed porter.

The porter is unreffed and set to NULL in the PorterData struct. However, because it's the remote contact who closed the porter the PorterData struct isn't freed because we'll lose our reference^Whold count.

This way, once someone calls some method needing the same porter, it'll be re-opened with the same hold count. Does that make sense?

> This code is duplicated; add maybe_start_timeout().

Done.

> If you were feeling keen you could fish 'addr' out of the WockyXmppConnection
> returned by wocky_ll_connector_finish() and then you wouldn't need a context
> struct.

Good point, done.

> loopback_recv_open_cb and loopback_sent_open_cb both leak on the error path.

Good catch, fixed.

> Every caller of this function passes a GSimpleAsyncResult as the first
> user_data. Maybe its signature should reflect that.

:-) done.

> This perhaps should suggest that the name of the local variable is wrong. ;-)

Fixed.

> If user_data1 was a GSimpleAsyncResult, you wouldn't need to hang onto self in
> data: you could get_self_object() on the GSAR.

Okay, yes you're right but that's a bit of a pain, especially given get_source_object() returns a new ref. Holding self isn't so bad in the struct when we already have like 8000 pointers in this struct, no?

> This is secret code for ‘wocky_stanza_get_from (stanza) == NULL’.

Yes.

> send is a terrible name for this function. How about:
> 
>   meta_porter_send_cb()
>   meta_porter_send_got_porter_cb()
>   wocky_meta_porter_send_async()
> 
> for this chain of functions?

Done, and made the others clearer too, unless you like static void send_iq (...)? :-)

> Are you missing a 'return' there?

No? It's the porter start implementation which returns void. You can get the porter from wocky_meta_porter_get_port().

>   contact = wocky_contact_factory_ensure_ll_contact (
>       priv->contact_factory, from);
> 
> is it guaranteed to be non-NULL?

Ensure is, yes.

> Pass NULL, NULL to g_hash_table_new(). You should also have a function that
> creates a StanzaHandler struct and stuffs it into the handlers hash table,
> rather than duplicating that code.

Done.

>   /* there were no porters to close anyway */
>   if (num == 0)
>     {
>       g_simple_async_result_complete (simple);
>       g_object_unref (simple);
>     }
> 
> This leaks the ClosePorterData structure. Instead, have a
> close_porters_maybe_complete() function called from here and from
> porter_close_cb().

No it doesn't? That struct hasn't been created yet?

>     g_simple_async_result_set_op_res_gpointer (simple, stanza, NULL);
> 
> is a no-op.

Huh? Why?

> If the function requires a WockyLLContact then its signature should say so.
> Ditto functions like wocky_meta_porter_borrow_connection().

Fixed.

>    * Finishes an asynchronous request to open a connection to @contact
> 
> This function has no contact argument:                        ^^^^^^^^

Fixed.

>    * Borrow the #GSocketConnection of the porter to @contact, if one
>    * exists otherwise %NULL will be returned. Note that the connection
>    * returned is not reffed and should not be kept as it still is owned
> 
>                                               ^^^^
> 
> Well, the caller can keep it if they want, by reffing it, but they should be
> aware that it might be disconnected spontaneously unless they hold() the
> corresponding contact.

I fixie(d!) the docstring

>    * and operated on by the underlying #WockyXmppConnection object.
>    *
>    * Returns: the #GSocketConnection or %NULL if no connection is open
>    */
> 
> I'd be inclined to make this return a ref, too, to be honest. I'm not really
> all that hung up on it, but code that unrefs a pointer and then returns that
> same pointer looks suspect. :)

Hmmph, I /really/ don't want people to use it though...

> (It also be nice to have accessors for C2SPorter:connection and
> XmppConnetion:base-stream so that you don't have to use g_object_get() in the
> first place and then this would look less suspect.)

Well, tbh I think there aren't accessors for a reason, but I can add them if you really want?

Wow, that took a while. Hopefully I fixed everything just how you wanted Mista Thoson.

+  if (priv->jid != NULL)
+    {
+      DEBUG ("You cannot set the meta porter JID again");
+      return;
+    }
+
+  /* don't try and change existing porter's JIDs */
+

It's a programming error to call it more than once: use g_return_if_fail()?

http://cgit.freedesktop.org/~jonny/wocky/commit/?h=meta-porter&id=7cb02519648fea65317a51f5e3ebe85f79b668b8

Why doesn't this also take … all the other arguments?

(In reply to comment #8)
> (In reply to comment #5)
> > but it doesn't actually seem to be removed from the dictionary of porters.
> > “It'll be removed because it's weak-reffed” isn't a good idea — you don't want
> > to rely on that kind of side-effect. I think the hash table key should be a
> > reffed porter.
> 
> The porter is unreffed and set to NULL in the PorterData struct. However,
> because it's the remote contact who closed the porter the PorterData struct
> isn't freed because we'll lose our reference^Whold count.
> 
> This way, once someone calls some method needing the same porter, it'll be
> re-opened with the same hold count. Does that make sense?

Yeah, I guess that does make sense.

> > If user_data1 was a GSimpleAsyncResult, you wouldn't need to hang onto self in
> > data: you could get_self_object() on the GSAR.
> 
> Okay, yes you're right but that's a bit of a pain, especially given
> get_source_object() returns a new ref. Holding self isn't so bad in the struct
> when we already have like 8000 pointers in this struct, no?

Yeah, it's fine as-is, it was just a remark.

> Done, and made the others clearer too, unless you like static void send_iq
> (...)? :-)

Great.

> 
> > Are you missing a 'return' there?
> 
> No? It's the porter start implementation which returns void. You can get the
> porter from wocky_meta_porter_get_port().

I meant: in the error case <http://cgit.freedesktop.org/~jonny/wocky/tree/wocky/wocky-meta-porter.c?h=meta-porter&id=7cb02519648fea65317a51f5e3ebe85f79b668b8#n995>, do you really want to go on to call g_socket_service_start()?

> >   /* there were no porters to close anyway */
> >   if (num == 0)
> >     {
> >       g_simple_async_result_complete (simple);
> >       g_object_unref (simple);
> >     }
> > 
> > This leaks the ClosePorterData structure. Instead, have a
> > close_porters_maybe_complete() function called from here and from
> > porter_close_cb().
> 
> No it doesn't? That struct hasn't been created yet?

That's true. This would be more obvious if you replaced the goto in <http://cgit.freedesktop.org/~jonny/wocky/tree/wocky/wocky-meta-porter.c?h=meta-porter&id=7cb02519648fea65317a51f5e3ebe85f79b668b8#n1294> with if (num != 0) { ClosePorterData *data = g_slice_new0 ... }

Looking at this again, you initialize 'porters' twice, and leak the first copy in the num == 0 case.

> >     g_simple_async_result_set_op_res_gpointer (simple, stanza, NULL);
> > 
> > is a no-op.
> 
> Huh? Why?

Misread, sorry. It's still wrong, though: the destroy notify function should be g_object_unref, not NULL.

> >    * and operated on by the underlying #WockyXmppConnection object.
> >    *
> >    * Returns: the #GSocketConnection or %NULL if no connection is open
> >    */
> > 
> > I'd be inclined to make this return a ref, too, to be honest. I'm not really
> > all that hung up on it, but code that unrefs a pointer and then returns that
> > same pointer looks suspect. :)
> 
> Hmmph, I /really/ don't want people to use it though...

Fair enough.

> > (It also be nice to have accessors for C2SPorter:connection and
> > XmppConnetion:base-stream so that you don't have to use g_object_get() in the
> > first place and then this would look less suspect.)
> 
> Well, tbh I think there aren't accessors for a reason, but I can add them if
> you really want?

Nah, it's fine.

(In reply to comment #9)
> It's a programming error to call it more than once: use g_return_if_fail()?

Yeah, makes sense.

> Why doesn't this also take … all the other arguments?

Hm, not sure why I didn't do this before.

> > No? It's the porter start implementation which returns void. You can get the
> > porter from wocky_meta_porter_get_port().
> 
> I meant: in the error case
> <http://cgit.freedesktop.org/~jonny/wocky/tree/wocky/wocky-meta-porter.c?h=meta-porter&id=7cb02519648fea65317a51f5e3ebe85f79b668b8#n995>,
> do you really want to go on to call g_socket_service_start()?

Ahh yes, I completely missed that! Fixed.

> That's true. This would be more obvious if you replaced the goto in
> <http://cgit.freedesktop.org/~jonny/wocky/tree/wocky/wocky-meta-porter.c?h=meta-porter&id=7cb02519648fea65317a51f5e3ebe85f79b668b8#n1294>
> with if (num != 0) { ClosePorterData *data = g_slice_new0 ... }

Okay, just for you!

> Looking at this again, you initialize 'porters' twice, and leak the first copy
> in the num == 0 case.

Ha, totally didn't notice that. Fixed.

> Misread, sorry. It's still wrong, though: the destroy notify function should be
> g_object_unref, not NULL.

Yes, done.

As discussed on IRC, http://cgit.freedesktop.org/~jonny/wocky/tree/wocky/wocky-meta-porter.c?h=meta-porter&id=ff6037a2a24288d80fa201bd00b0dc9b5d668971#n1281 will break if num > 0 but any of the PorterDatas have porter == NULL.

Christmas has arrived, almost three months late.

zomg! Thanks!