[Bug 37145] Replace low level network code with GIO

[bugzilla-daemon at freedesktop.org]

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

--- Comment #19 from Debarshi Ray <debarshi.ray at gmail.com> 2011-09-10 14:27:17 PDT ---
(In reply to comment #18)
> +typedef struct _CloseAsyncData CloseAsyncData;
> +typedef struct _WriteAsyncData WriteAsyncData;
> 
> Vestigal typedefs!

Removed.

> +    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?

> +    memset(priv->input_buffer, '\0', sizeof(priv->input_buffer));
> +    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);
> 
> This is duplicated, it'd be nice to have a helper function for this.

Will do.

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

>   IdleConnection: Initialize priv->conn
> 
> +    priv->conn = NULL;
> 
> This shouldn't be needed: GObject zeroes out your structures when it allocates
> them.

Removed. Also removed similar instance of 'priv->io_stream = NULL' from
idle_server_connection_init.

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

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

> These are in existing code that you just moved around, feel free to ignore
> them:
> 
> +    if (!priv->realname || !priv->realname[0]) {
> 
> tp_str_empty()

Will do.

> +        if (g_realname && g_realname[0] && strcmp(g_realname, "Unknown"))
> 
> !tp_strdiff (g_realname, "Unknown")

What about g_strcmp0?

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