Bug 34932

I think it's stupid that the application has to deal with adding timeouts and _force_close_async(). _close_async() should take a timeout. I've filed Bug 35548.

+  for (l = priv->resolvers; l != NULL; l = l->next)
+    {
+      GaServiceResolver *resolver = l->data;
+      AvahiAddress address;
+      guint16 port;
+      AvahiIfIndex ifindex;
+
+      g_object_get (resolver, "interface", &ifindex, NULL);

… and 'ifindex' is never used. What's it meant to be for? Where's this code from?

+          socket_address = g_inet_socket_address_new (addr, port);
+          /* socket_address owns addr now */
+          g_object_unref (addr);

This comment is misleading—either it's taken its own ref and we can release the one we own, or it's stolen ours and we can't. Which is it? I'm gonna guess the former, which I don't think needs commenting.

+          addresses = g_list_append (addresses, socket_address);

<http://en.wikipedia.org/wiki/Schlemiel_the_Painter's_algorithm>. Yeah, it's not a big deal here, but this is one reason I like stack-allocated GQueues: you can build GLists with repeated O(1) append. I guess this isn't new code though.

avahi-contact-manager: add contacts to Wocky's contact factory too when created

Don't we also want to remove them when they're removed?

The whole of send_item_not_found can be replaced with wocky_porter_send_iq_error() but I guess the whole of salut-disco.c will be replaced with code in Wocky so.

salut_disco_request() documents its 'object' parameter  thusly:

 * @object: GObject to bind request to. the callback will not be
 *          called if this object has been unrefed. NULL if not needed

But as far as I can tell, that's not true any more. If the object dies, the cancellable is cancelled, which will lead to the send_iq callback being called. _finish() will return an error, which will be happily handed to the callback.

It looks like the only code paths which cancel the cancellable are those which mean the callback shouldn't be called, so you could just bolt an is_cancelled() check into disco_request_sent_cb().

+  if (wocky_node_get_child_ns (wocky_stanza_get_top_node (stanza), "invite",
+        GIBBER_TELEPATHY_NS_CLIQUE) != NULL)
+    /* discard Clique MUC invite */
     return FALSE;

I know this comment isn't new, but it is misleading — we're not discarding it.

In gibber-file-transfer.c:


+      case PROP_PORTER:
+        {
+          self->priv->porter = g_value_dup_object (value);
+
+          if (self->priv->porter != NULL)
+            {
+              self->priv->stanza_id =
+                wocky_porter_register_handler_from_anyone (self->priv->porter,
+                    WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_SUB_TYPE_NONE,
+                    WOCKY_PORTER_HANDLER_PRIORITY_NORMAL, received_stanza_cb,
+                    self, NULL);
+            }

Why would it be NULL?

I have reviewed up to and including “tubes-manager: stop using the XCM”.

(In reply to comment #1)
> I think it's stupid that the application has to deal with adding timeouts and
> _force_close_async(). _close_async() should take a timeout. I've filed Bug
> 35548.

I'll look at that soon.

> +      AvahiIfIndex ifindex;
> +
> +      g_object_get (resolver, "interface", &ifindex, NULL);
> 
> … and 'ifindex' is never used. What's it meant to be for? Where's this code
> from?

Removed.

> This comment is misleading—either it's taken its own ref and we can release the
> one we own, or it's stolen ours and we can't. Which is it? I'm gonna guess the
> former, which I don't think needs commenting.

OK, removed.

> +          addresses = g_list_append (addresses, socket_address);
> 
> <http://en.wikipedia.org/wiki/Schlemiel_the_Painter's_algorithm>. Yeah, it's
> not a big deal here, but this is one reason I like stack-allocated GQueues: you
> can build GLists with repeated O(1) append. I guess this isn't new code though.

Yeah yeah, I've never been a fan of this and actually I'm not sure why. I guess perhaps because it feels like such an epic hack to get around the terrible data structure we're using. I guess we're doing worse things just by using GObject.

Fixed.

> avahi-contact-manager: add contacts to Wocky's contact factory too when created
> 
> Don't we also want to remove them when they're removed?

The contact factory keeps a weak ref on the objects and they're removed when the object is disposed. In Salut's case, they're never disposed until disconnection because the Salut contact manager keeps a ref to the contacts.

> salut_disco_request() documents its 'object' parameter  thusly:
> 
>  * @object: GObject to bind request to. the callback will not be
>  *          called if this object has been unrefed. NULL if not needed
> 
> But as far as I can tell, that's not true any more. If the object dies, the
> cancellable is cancelled, which will lead to the send_iq callback being called.
> _finish() will return an error, which will be happily handed to the callback.
> 
> It looks like the only code paths which cancel the cancellable are those which
> mean the callback shouldn't be called, so you could just bolt an is_cancelled()
> check into disco_request_sent_cb().

Good catch, fixed!

> +  if (wocky_node_get_child_ns (wocky_stanza_get_top_node (stanza), "invite",
> +        GIBBER_TELEPATHY_NS_CLIQUE) != NULL)
> +    /* discard Clique MUC invite */
>      return FALSE;
> 
> I know this comment isn't new, but it is misleading — we're not discarding it.

Fixed.

> In gibber-file-transfer.c:
> 
> +      case PROP_PORTER:
> +        {
> +          self->priv->porter = g_value_dup_object (value);
> +
> +          if (self->priv->porter != NULL)
> +            {
> +              self->priv->stanza_id =
> +                wocky_porter_register_handler_from_anyone (self->priv->porter,
> +                    WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_SUB_TYPE_NONE,
> +                    WOCKY_PORTER_HANDLER_PRIORITY_NORMAL, received_stanza_cb,
> +                    self, NULL);
> +            }
> 
> Why would it be NULL?

Yeah I guess you're right. I've added a g_return_val_if_fail to the only place which creates it and it is construct-only after all. Fixed then.

Pushed fixes to my branch.

+iq_close_reply_cb (GObject *source_object,
+                   GAsyncResult *result,
                    gpointer user_data)
 {
+  WockyPorter *porter = WOCKY_PORTER (source_object);
+  GError *error = NULL;
+  WockyStanza *stanza;
+
+  stanza = wocky_porter_send_iq_finish (porter, result, &error);
+
+  if (stanza == NULL)
+    {
+      DEBUG ("Failed to close IQ: %s", error->message);
+      g_clear_error (&error);
+    }
+
+  g_object_unref (stanza);
 }

Yeah, unreffing that NULL stanza is gonna end well. :þ

 static void
+make_porter_connections (GibberBytestreamIBB *self)
+{
+  GibberBytestreamIBBPrivate *priv = GIBBER_BYTESTREAM_IBB_GET_PRIVATE (self);
+  static gboolean done = FALSE;
+  gchar *jid;
+
+  if (done)
+    return;
+
+  jid = wocky_contact_dup_jid (priv->contact);
+
+  priv->stanza_received_id = wocky_porter_register_handler_from (priv->porter,
+      WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_TYPE_NONE, jid,
+      WOCKY_PORTER_HANDLER_PRIORITY_NORMAL, received_stanza_cb, self, NULL);
+
+  g_free (jid);
+
+  done = TRUE;
+}

This is wrong. 'done' is not specific to 'self', so only the first GibberBytestreamIBB will ever work.

Also:

+      case PROP_PORTER:
+        priv->porter = g_value_dup_object (value);
+
+        if (priv->porter != NULL && priv->contact != NULL)
+          make_porter_connections (self);
+        break;
+      case PROP_CONTACT:
+        priv->contact = g_value_dup_object (value);
+
+        if (priv->porter != NULL && priv->contact != NULL)
+          make_porter_connections (self);

I'd just add

+        if (priv->porter != NULL && priv->contact != NULL)
+          make_porter_connections (self);

to _constructed(), and then you wouldn't need to track whether it's been done or not.

Ditto OOB.


+  conn = wocky_meta_porter_borrow_connection (WOCKY_META_PORTER (priv->porter),
+      priv->contact);
+
+  if (conn != NULL)
+    socket_address = g_socket_connection_get_remote_address (conn, NULL);
+
+  if (conn == NULL || socket_address == NULL)
+    {
+      /* I'm too lazy to create more specific errors for this  as it should
+       * never happen while using salut anyway.. */
+      GError e = { GIBBER_XMPP_ERROR, XMPP_ERROR_ITEM_NOT_FOUND,
+          "Unsable get socket address for the control connection" };
+      DEBUG ("Could not get socket address for the control connection" );
+      gibber_bytestream_iface_close (GIBBER_BYTESTREAM_IFACE (self), &e);
+      goto out;
+    }
+
+  if (!g_socket_address_to_native (G_SOCKET_ADDRESS (address), &(addr.storage),
+          sizeof (addr.storage), &error))
+    {
+      GError e = { GIBBER_XMPP_ERROR, XMPP_ERROR_ITEM_NOT_FOUND,
+          "Failed to turn socket address into bytes" };
+      DEBUG ("Failed to get native socket address: %s", error->message);
+      gibber_bytestream_iface_close (GIBBER_BYTESTREAM_IFACE (self), &e);
+      g_clear_error (&error);
+      goto out;
+    }
+
+  g_object_unref (socket_address);
+

The second error path leaks socket_address. And do you really mean to get a GSocketAddress, cast some other random thing to a GSocketAddress and call a method on it, and then unref the GSocketAddress you grabbed, unused?

http://cgit.freedesktop.org/~jonny/telepathy-salut/commit/?h=meta-porter&id=a6bf9ee54e934988f59a3d3296e3cc476b1bb703

What's the effect of this? Does gibber_file_transfer_is_file_offer() check that from is not NULL?

Your fixes to my first batch of comments look good, except:

(In reply to comment #2)
> (In reply to comment #1)
> > +          addresses = g_list_append (addresses, socket_address);
> > 
> > <http://en.wikipedia.org/wiki/Schlemiel_the_Painter's_algorithm>. Yeah, it's
> > not a big deal here, but this is one reason I like stack-allocated GQueues: you
> > can build GLists with repeated O(1) append. I guess this isn't new code though.
> 
> Yeah yeah, I've never been a fan of this and actually I'm not sure why. I guess
> perhaps because it feels like such an epic hack to get around the terrible data
> structure we're using. I guess we're doing worse things just by using GObject.
> 
> Fixed.

--. Repeated prepends followed by reverse is even worse than using a dumb O(n²) algorithm.

(In reply to comment #3)
> Yeah, unreffing that NULL stanza is gonna end well. :þ

Oops!

> This is wrong. 'done' is not specific to 'self', so only the first
> GibberBytestreamIBB will ever work.
> 
> Also, I'd just add
> 
> +        if (priv->porter != NULL && priv->contact != NULL)
> +          make_porter_connections (self);
> 
> to _constructed(), and then you wouldn't need to track whether it's been done
> or not.

Done.

> Ditto OOB.

Done.

> +  conn = wocky_meta_porter_borrow_connection (WOCKY_META_PORTER
> (priv->porter),
> +      priv->contact);
> +
> +  if (conn != NULL)
> +    socket_address = g_socket_connection_get_remote_address (conn, NULL);
> +
> +  if (conn == NULL || socket_address == NULL)
> +    {
> +      /* I'm too lazy to create more specific errors for this  as it should
> +       * never happen while using salut anyway.. */
> +      GError e = { GIBBER_XMPP_ERROR, XMPP_ERROR_ITEM_NOT_FOUND,
> +          "Unsable get socket address for the control connection" };
> +      DEBUG ("Could not get socket address for the control connection" );
> +      gibber_bytestream_iface_close (GIBBER_BYTESTREAM_IFACE (self), &e);
> +      goto out;
> +    }
> +
> +  if (!g_socket_address_to_native (G_SOCKET_ADDRESS (address),
> &(addr.storage),
> +          sizeof (addr.storage), &error))
> +    {
> +      GError e = { GIBBER_XMPP_ERROR, XMPP_ERROR_ITEM_NOT_FOUND,
> +          "Failed to turn socket address into bytes" };
> +      DEBUG ("Failed to get native socket address: %s", error->message);
> +      gibber_bytestream_iface_close (GIBBER_BYTESTREAM_IFACE (self), &e);
> +      g_clear_error (&error);
> +      goto out;
> +    }
> +
> +  g_object_unref (socket_address);
> +
> 
> The second error path leaks socket_address. And do you really mean to get a
> GSocketAddress, cast some other random thing to a GSocketAddress and call a
> method on it, and then unref the GSocketAddress you grabbed, unused?

Why didn't the compiler warn me about using an assigned variable I wonder.

Actually I think socket_address can be leaked in the first error path too. Anyway, fixed.

> http://cgit.freedesktop.org/~jonny/telepathy-salut/commit/?h=meta-porter&id=a6bf9ee54e934988f59a3d3296e3cc476b1bb703
> 
> What's the effect of this? Does gibber_file_transfer_is_file_offer() check that
> from is not NULL?

I can't actually remember, but I hit this problem at some point. The problem was something like that the stanza wasn't for us and the from contact wasn't set for some reason... Sorry I can't remember the actual reason.

(In reply to comment #4)
> --. Repeated prepends followed by reverse is even worse than using a dumb O(n²)
> algorithm.

OMG STATIC ALLOCATED HEAPLY GQUEUEUEUEUE IN USE NOW!

 static void
+gibber_bytestream_ibb_constructed (GObject *obj)
+{
+  GibberBytestreamIBB *self = GIBBER_BYTESTREAM_IBB (obj);
+  GibberBytestreamIBBPrivate *priv = GIBBER_BYTESTREAM_IBB_GET_PRIVATE (self);
+
+  if (priv->porter != NULL && priv->contact != NULL)
+    make_porter_connections (self);
+}

you need to chain up …

Fix this and the other one(s) to call the parent class's constructed method before the if (), and then merge this branch.

(In reply to comment #7)
> +gibber_bytestream_ibb_constructed (GObject *obj)
> 
> you need to chain up …

Done.

I've pushed some more patches to the branch which should be easy enough to review.

For the record, you've reviewed up to "avahi-contact: use stack-allocated GQueue instead of expensive or annoying GLists" already.

im-manager: ignore clique MUC invites and leave for the MUC manager

Why isn't the MUC manager just registering a higher-priority handler (and returning TRUE), rather than some other unrelated handler having to know details of Clique?

+      DEBUG ("failed to announce: %s",
+          error != NULL ? error->message : "(no error message)");
+
       tp_base_connection_change_status (
             TP_BASE_CONNECTION (self),
             TP_CONNECTION_STATUS_DISCONNECTED,
             TP_CONNECTION_STATUS_REASON_NETWORK_ERROR);
+
+      if (error != NULL)
+        g_error_free (error);

FYI g_clear_error() is null-safe.

+  if (force_called)
+    tp_base_connection_finish_shutdown (base);

It looks like you're ensuring that finish_shutdown will be called exactly 0 or 2 times, and never once? closed_cb only calls it if disconnect_timer is not 0 ... okay, so this is right, in which case why is the variable called "force_called"? I think it means exactly the opposite.

disco: only delete the request if it hasn't been already done for us

This looks completely wrong to me, I'm sorry to say. As far as I can tell, you'll *always* leak the SalutDiscoRequest if the cancellable is cancelled, given this patch.

muc-manager: don't assert on no invite node

This patch looks wrong too. When the handler is registered, the pattern specifies that the <invite xmlns='...clique'/> element must exist. And the commit message is not English.

   /* release any references held by the object here */
 
-  if (self->handle != 0)
-    tp_handle_unref (contact_repo, self->handle);
+  if (self->handle != 0 && self->connection != NULL)
+    {
+      TpHandleRepoIface *contact_repo = tp_base_connection_get_handles
+        ((TpBaseConnection *) self->connection, TP_HANDLE_TYPE_CONTACT);
+      tp_handle_unref (contact_repo, self->handle);
+    }

There's an argument that this code could just go away since tp_handle_unref is a no-op these days.

+        'nickname': re.sub(r'.*/', '', sys.argv[0]),

OOI, why not os.path.basename()? :þ (no big deal)

+        name += ('-' + re.sub(r'.*/', '', sys.argv[0])[:-3])

this is secret code for os.path.splitext(os.path.basename(sys.argv[0]))[0]


+        if len(self.contact_name) > 63:

Is there really no constant for this in the avahi library?

(In reply to comment #9)
> Why isn't the MUC manager just registering a higher-priority handler (and
> returning TRUE), rather than some other unrelated handler having to know
> details of Clique?

Fair, done.

> +      if (error != NULL)
> +        g_error_free (error);
> 
> FYI g_clear_error() is null-safe.

Done.

> +  if (force_called)
> +    tp_base_connection_finish_shutdown (base);
> 
> It looks like you're ensuring that finish_shutdown will be called exactly 0 or
> 2 times, and never once? closed_cb only calls it if disconnect_timer is not 0
> ... okay, so this is right, in which case why is the variable called
> "force_called"? I think it means exactly the opposite.

ha, half of that is true. force_called does indeed mean exactly the opposite, but I had also got the conditional to call finish_shutdown the wrong way round too, so if you replace that

  if (force_called) finish_shutdown()

with

  if (force_not_called) finish_shutdown()

That makes more sense, which is why it actually worked. Anyway, fixed.

> disco: only delete the request if it hasn't been already done for us
> 
> This looks completely wrong to me, I'm sorry to say. As far as I can tell,
> you'll *always* leak the SalutDiscoRequest if the cancellable is cancelled,
> given this patch.

Yeah I guess this was the wrong to go about fixing this problem and didn't actually solve it in every case. I appended a patch.

> muc-manager: don't assert on no invite node
> 
> This patch looks wrong too. When the handler is registered, the pattern
> specifies that the <invite xmlns='...clique'/> element must exist. And the
> commit message is not English.

Fair. (I removed the assertion when I had removed the handler register stanza in a local checkout etc.) Reverted.

>    /* release any references held by the object here */
> 
> -  if (self->handle != 0)
> -    tp_handle_unref (contact_repo, self->handle);
> +  if (self->handle != 0 && self->connection != NULL)
> +    {
> +      TpHandleRepoIface *contact_repo = tp_base_connection_get_handles
> +        ((TpBaseConnection *) self->connection, TP_HANDLE_TYPE_CONTACT);
> +      tp_handle_unref (contact_repo, self->handle);
> +    }
> 
> There's an argument that this code could just go away since tp_handle_unref is
> a no-op these days.

I think I'll just leave this for now.

> +        'nickname': re.sub(r'.*/', '', sys.argv[0]),
[...]
> +        name += ('-' + re.sub(r'.*/', '', sys.argv[0])[:-3])

Okay. I took this from your patch to do a similar thing in gabble (d7ae6c73bce69117).

> Is there really no constant for this in the avahi library?

No.

Okaaaaay, pushed.

(In reply to comment #10)
> (In reply to comment #9)
> > Why isn't the MUC manager just registering a higher-priority handler (and
> > returning TRUE), rather than some other unrelated handler having to know
> > details of Clique?
> 
> Fair, done.

I think invite_stanza_callback in the MUC manager should return TRUE in all cases: a malformed clique invite is still a clique invite. Otherwise we'll fall back to showing the text of the <body/>.

> > disco: only delete the request if it hasn't been already done for us
> > 
> > This looks completely wrong to me, I'm sorry to say. As far as I can tell,
> > you'll *always* leak the SalutDiscoRequest if the cancellable is cancelled,
> > given this patch.
> 
> Yeah I guess this was the wrong to go about fixing this problem and didn't
> actually solve it in every case. I appended a patch.

@@ -490,17 +496,10 @@ out:
     {
       request->callback (request->disco, request, request->contact, request->node,
           query_node, error, request->user_data);
-
-      /* if we really are cancelled, then don't delete the request as
-       * clearing the disco waiters is what caused us to be
-       * cancelled. */
-      if (error != NULL
-          && (error->domain != G_IO_ERROR || error->code != G_IO_ERROR_CANCELLED))
-        {
-          delete_request (request);
-        }
     }
 
+    delete_request (request);
+

nice indentation, bro.

and this is I think existing code, but while you're here you could replace this:

  wocky_stanza_get_type_info (reply, NULL, &sub_type);

  if (sub_type == WOCKY_STANZA_SUB_TYPE_ERROR)
    {
      wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL);
      goto out;
    }

with this:

  if (wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL))
    goto out;

> > +        'nickname': re.sub(r'.*/', '', sys.argv[0]),
> [...]
> > +        name += ('-' + re.sub(r'.*/', '', sys.argv[0])[:-3])
> 
> Okay. I took this from your patch to do a similar thing in gabble
> (d7ae6c73bce69117).

Yeaaaaaaaaaaaaaaah. :D

> > Is there really no constant for this in the avahi library?
> 
> No.

:'(

So, fix the clique invite handler, fix the indentation (and if you're keen the sub_type stuff), and merge away!

(In reply to comment #11)
> (In reply to comment #10)
> > Okay. I took this from your patch to do a similar thing in gabble
> > (d7ae6c73bce69117).
> 
> Yeaaaaaaaaaaaaaaah. :D

Relatedly, http://cgit.collabora.co.uk/git/user/wjt/telepathy-gabble-wjt.git/commit/?h=test-name-

(In reply to comment #11)
> I think invite_stanza_callback in the MUC manager should return TRUE in all
> cases: a malformed clique invite is still a clique invite. Otherwise we'll fall
> back to showing the text of the <body/>.

Done.

> nice indentation, bro.

Fixed.

> and this is I think existing code, but while you're here you could replace
> this:
> 
>   wocky_stanza_get_type_info (reply, NULL, &sub_type);
> 
>   if (sub_type == WOCKY_STANZA_SUB_TYPE_ERROR)
>     {
>       wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL);
>       goto out;
>     }
> 
> with this:
> 
>   if (wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL))
>     goto out;

OK.

> So, fix the clique invite handler, fix the indentation (and if you're keen the
> sub_type stuff), and merge away!

Fuck yeah!

Shit hot.