[Bug 34931] Add a "meta porter"

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Wed Mar 16 11:50:49 CET 2011


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

--- Comment #8 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-03-16 03:50:48 PDT ---
(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.

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


More information about the telepathy-bugs mailing list