[Bug 37145] Replace low level network code with GIO

[bugzilla-daemon at freedesktop.org]

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

--- Comment #20 from Will Thompson <will.thompson at collabora.co.uk> 2011-09-15 09:40:27 PDT ---
I notice that we've still lost setting TCP_NODELAY.

(In reply to comment #19)
> (In reply to comment #18)
> > +    if (g_input_stream_read_finish(input_stream, res, &error) == -1) {
> > +        IDLE_DEBUG("g_input_stream_read failed: %s", error->message);
> > +        g_error_free(error);
> > +        goto disconnect;
> >      }
> > [...]
> >
> > It's a shame that we lose the errors in these cases. Ah, I see that in the
> > second case, you later hooked up the message.
> 
> Should we pass the error through the signal?

It might be nice—I don't think it's essential, though. At most it will end up
in debug-message in the ConnectionErrorDetails.

> > +    g_cancellable_reset(priv->cancellable);
> > +    g_object_ref(conn);
> > +    g_socket_client_connect_to_host_async(priv->socket_client, priv->host,
> > priv->port, priv->cancellable, _connect_to_host_ready, conn);
> > 
> > I don't see why you're resetting the cancellable here. If it was in the
> > cancelled state, surely we shouldn't be trying to connect at all.
> 
> I think we can get rid of the GCancellable inside IdleServerConnection once we
> have an _async version of the disconnect method.

Maybe so, but I still don't understand why you're calling _reset here. :)

> >  static void _connect_to_host_ready(GObject *source_object, GAsyncResult *res,
> > gpointer user_data) {
> >      GSocketClient *socket_client = G_SOCKET_CLIENT(source_object);
> > -    IdleServerConnection *conn = IDLE_SERVER_CONNECTION(user_data);
> > +    GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(user_data);
> > +    IdleServerConnection *conn =
> > IDLE_SERVER_CONNECTION(g_async_result_get_source_object(G_ASYNC_RESULT(result)));
> > …
> >          change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED,
> > SERVER_CONNECTION_STATE_REASON_ERROR);
> > -        g_object_unref(conn);
> > -        return;
> > +        goto cleanup;
> > …
> > +    g_cancellable_reset(priv->cancellable);
> > +    g_object_ref(conn);
> >      g_input_stream_read_async (input_stream, &priv->input_buffer,
> > sizeof(priv->input_buffer) - 1, G_PRIORITY_DEFAULT, priv->cancellable,
> > _input_stream_read_ready, conn);
> > 
> > g_async_result_get_source_object() returns a reference, so if I'm not mistaken,
> > the removal of the call to g_object_unref and the addition of the call to
> > g_object_ref mean you will leak a reference.
> 
> You are right. I though 'get' methods don't return a reference. I have tried to
> fix the leak by restoring the g_object_unref and removing the extra
> g_object_ref.

Looks right now.

> > IdleConnection: No need to disconnect if connection can not be made
> > 
> > I don't understand this change: if _iface_shut_down is called while we're in
> > state CONNECTING, surely we do want to tell the IdleServerConnection to give
> > up?
> 
> I have improved this patch with a better explanation in the commit message and
> comments. See this commit:
> IdleConnection: Handle connecting -> disconnected correctly
> 
> If you take this out, then the connect/connect-fail.py and
> connect/connect-fail-ssl.py tests will fail due a segmentation fault.

Okay, this makes a little more sense to me now. :)

> > These are in existing code that you just moved around, feel free to ignore
> > them:
> > 
> > +        if (g_realname && g_realname[0] && strcmp(g_realname, "Unknown"))
> > 
> > !tp_strdiff (g_realname, "Unknown")
> 
> What about g_strcmp0?

I don't like g_strcmp0 because it throws away type information. (It really
depresses me that there are so many different ways to compare strings in this
language…)

FWIW, having two commits titled «IdleServerConnection: Make the interface
"asynchronous friendly"» whose bodies say what they actually do «Replace
idle_server_connection_connect with
idle_server_connection_connect_async and
idle_server_connection_connect_finish.» makes for slightly confusing reading.
(Rather than, say, “Async-ify idle_server_connection_connect” and “Async-ify
idle_server_connection_send”.)


+    if (!idle_server_connection_connect_finish(sconn, res, &error)) {
+        IDLE_DEBUG("idle_server_connection_connect failed: %s",
error->message);

-        tp_base_connection_disconnect_with_dbus_error(base_conn,
-                                  TP_ERROR_STR_NETWORK_ERROR,
+       
tp_base_connection_disconnect_with_dbus_error(TP_BASE_CONNECTION(conn),
+                                  tp_error_get_dbus_name(error->code),
                                   NULL,
                                   TP_CONNECTION_STATUS_REASON_NETWORK_ERROR);

+        g_error_free(error);

You should probably sanity-check that error->domain == TP_ERRORS.

If you pass a hash table built using something like tp_asv_new
("debug-message", G_TYPE_STRING, error->message, NULL); as the third argument
to disconnect_with_dbus_error, this could conceivably be shown by a UI in some
kind of “technical details” expander.

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