[Bug 37145] Replace low level network code with GIO

[bugzilla-daemon at freedesktop.org]

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

--- Comment #21 from Debarshi Ray <debarshi.ray at gmail.com> 2011-09-16 11:08:20 PDT ---
> +    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.

Done.

> I notice that we've still lost setting TCP_NODELAY.

Using setsockopt since I could not locate a GSocket API. Worth adding one to
Glib?

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

Ok. I have left it as it was.

>>> These are in existing code that you just moved around, feel free to ignore
>>> them:

Replaced with tp_strdiff & tp_str_empty.

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

Done.

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

Will do.

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