[Bug 34931] Add a "meta porter"

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Will Thompson <will.thompson at collabora.co.uk> 2011-03-15 13:52:23 PDT ---
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.)

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