[Bug 34931] Add a "meta porter"

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Will Thompson <will.thompson at collabora.co.uk> 2011-03-18 09:33:21 PDT ---
+  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.

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