Bug 23639

Reply to Will's comments [1] of the wocky branch:

“Loudmouth allows registering handlers”, or (better) “Loudmouth lets you register handlers”.

fixed


"Maybe WockyXmppNodes itself should have a copy() method?"

typo fixed.
Will ask to sjoerd about the API addition.


"+ /* FIXME: check SSL errors */"

Gio doesn't expose SSL errors. :(


"Stage 2 of connecting. This function is called once the connect operation has finished. It checks if the connection succeeded, creates and starts the WockyPorter, then sends a discovery request..."

fixed

"We should be sure to file bugs about these regressions."
I opened #23640 and #23641


"Why else can wocky_porter_close() fail?"
Network error for example: the connection is broken before we send our close stanza or receive the close stanza from the peer.


"+ /* FIXME: cancel connecting if we are connecting */"

I opened #23642

"Does anything actually need lm_message_node_get_child_with_namespace() to recursively look down all its children? If not maybe we could delete it entirely? :)"

git grep lm_message_node_get_child_with_namespace | wc -l
87
So yeah it's used. Parsing code could probably be improved to stop to use this function but that's out of the scope of this branch.


[1] http://people.collabora.co.uk/~wjt/tmp/Wocky%20port.html

(In reply to comment #1)
> "+ /* FIXME: check SSL errors */"
> 
> Gio doesn't expose SSL errors. :(

I opened https://bugs.freedesktop.org/show_bug.cgi?id=23649 and https://bugs.freedesktop.org/show_bug.cgi?id=23650

Tests:

+        if conn.GetStatus() == cs.CONN_STATUS_CONNECTED:
+            # Connection is connected, properly disconnect it
+            disconnect_conn(queue, conn, stream)
+        else:
+            # Connection is not connected, call Disconnect() to destroy it
+            conn.Disconnect()

I don't get this: if the connection's not connected, it could be either CONNECTING (in which case, Disconnect() works) or DISCONNECTED (in which case, Disconnect() is either redundant (it's about to die anyway) or not right (if the connection hasn't started connecting yet, does Disconnect() kill it?)

+  /* It appears that dbus-glib registers a filter that wrongly returns
+   * DBUS_HANDLER_RESULT_HANDLED for signals, so for *our* filter to have any
+   * effect, we need to install it as soon as possible */

Did you file a bug?

main() in main-debug.c never unrefs its TpDBusDaemon.

Why is the Disconnected filter necessary, anyway?

(In reply to comment #1)
> > "Maybe WockyXmppNodes itself should have a copy() method?"
>
> Will ask to sjoerd about the API addition.

I don't think this is a merge blocker.

> > "+ /* FIXME: check SSL errors */"
> 
> Gio doesn't expose SSL errors. :(

Okay, I don't think representing them by NetworkError is toooo big a regression. Not a merge blocker.

> git grep lm_message_node_get_child_with_namespace | wc -l
> 87
> So yeah it's used. Parsing code could probably be improved to stop to use this
> function but that's out of the scope of this branch.

Yeah, I imagine most of them don't *actually* want to find a child at any point in the hierarchy. But as you say, not in scope.

(In reply to comment #3)
> Tests:
> 
> +        if conn.GetStatus() == cs.CONN_STATUS_CONNECTED:
> +            # Connection is connected, properly disconnect it
> +            disconnect_conn(queue, conn, stream)
> +        else:
> +            # Connection is not connected, call Disconnect() to destroy it
> +            conn.Disconnect()
> 
> I don't get this: if the connection's not connected, it could be either
> CONNECTING (in which case, Disconnect() works) or DISCONNECTED (in which case,
> Disconnect() is either redundant (it's about to die anyway) or not right (if
> the connection hasn't started connecting yet, does Disconnect() kill it?)

In the CONNECTED case, we have to wait for the stream-closed event and send the close too (Disconnect is async).
We can't do that if the connection isn't connected.

> +  /* It appears that dbus-glib registers a filter that wrongly returns
> +   * DBUS_HANDLER_RESULT_HANDLED for signals, so for *our* filter to have any
> +   * effect, we need to install it as soon as possible */
> 
> Did you file a bug?

No, I stole this comment from telepathy-glib/run.c.
I can open one of you want.

> main() in main-debug.c never unrefs its TpDBusDaemon.

fixed.

> Why is the Disconnected filter necessary, anyway?

When running in tests, Gabble is generally killed because of this signal. By catching it, I can run wocky_deinit () before exiting the process and so produce a more useful Valgring report.

Okay, looks good to me!

Fixed in git for 0.9.0