[Telepathy-commits] [telepathy-idle/master] Fix a warning about closing an invalid file descriptor

[Jonathon Jongsma]

When running the new test suite for idle (not yet merged upstream), I was
getting tests aborted and created core files due to a warning about closing an
invalid file descriptor.  The actual source of the warning I was seeing was in
async_connect_data_destroy() while shutting down the IO channel.  I believe this
is because a few lines earlier, we close the IOChannel's file descriptor, then
glib attempts to close it again in the g_io_channel_shutdown() function.

We seem to be doing this many other places, but we don't always get warnings
about it because often the close() call comes after the g_io_channel_shutdown()
call, and we don't check the return status from close().  It seems to me that
since glib will free the file descriptor for us when we shut down the IOChannel,
there's not even any reason to carry around the fd separately in
_AsyncConnectData (since we can always get it from the IOChannel anyway), so I
removed that as well.

I haven't changed similar uses in the SSL server connection yet.
---
 src/idle-server-connection.c |   28 ++++------------------------
 1 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/src/idle-server-connection.c b/src/idle-server-connection.c
index 1808cae..cd19651 100644
--- a/src/idle-server-connection.c
+++ b/src/idle-server-connection.c
@@ -55,7 +55,6 @@ enum {
 struct _AsyncConnectData {
 	guint watch_id;
 
-	guint fd;
 	GIOChannel *io_chan;
 
 	IdleDNSResult *res;
@@ -73,11 +72,6 @@ static void async_connect_data_destroy(struct _AsyncConnectData *data) {
 		data->watch_id = 0;
 	}
 
-	if (data->fd) {
-		close(data->fd);
-		data->fd = 0;
-	}
-
 	if (data->io_chan) {
 		g_io_channel_shutdown(data->io_chan, FALSE, NULL);
 		g_io_channel_unref(data->io_chan);
@@ -351,15 +345,14 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 	IdleDNSResult *cur = connect_data->cur;
 	IdleDNSResult *next = cur->ai_next;
 
-	g_assert(getsockopt(connect_data->fd, SOL_SOCKET, SO_ERROR, &optval, &optlen) == 0);
+	fd = g_io_channel_unix_get_fd(connect_data->io_chan);
+	g_assert(getsockopt(fd, SOL_SOCKET, SO_ERROR, &optval, &optlen) == 0);
 
 	if (optval == 0) {
 		int opt;
 
 		IDLE_DEBUG("connected!");
 
-		fd = connect_data->fd;
-
 		change_state(conn, SERVER_CONNECTION_STATE_CONNECTED, SERVER_CONNECTION_STATE_REASON_REQUESTED);
 
 		g_assert(priv->io_chan == NULL);
@@ -369,7 +362,6 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 		priv->io_chan = connect_data->io_chan;
 
 		connect_data->io_chan = NULL;
-		connect_data->fd = 0;
 
 		async_connect_data_destroy(priv->connect_data);
 		priv->connect_data = NULL;
@@ -404,12 +396,12 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 		IDLE_DEBUG("re-using existing socket for trying again");
 
 		errno = 0;
-		connect(connect_data->fd, next->ai_addr, next->ai_addrlen);
+		connect(fd, next->ai_addr, next->ai_addrlen);
 
 		for (int i = 0; i < 5 && errno == ECONNABORTED; i++) {
 			IDLE_DEBUG("got ECONNABORTED for %ith time", i + 1);
 			errno = 0;
-			connect(connect_data->fd, next->ai_addr, next->ai_addrlen);
+			connect(fd, next->ai_addr, next->ai_addrlen);
 		}
 
 		if (errno != EINPROGRESS) {
@@ -457,7 +449,6 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 		IDLE_DEBUG("failed to set socket to non-blocking mode: %s", g_strerror(errno));
 		g_io_channel_shutdown(io_chan, FALSE, NULL);
 		g_io_channel_unref(io_chan);
-		close(fd);
 		change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED, SERVER_CONNECTION_STATE_REASON_ERROR);
 		return FALSE;
 	}
@@ -469,7 +460,6 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 		IDLE_DEBUG("initial connect() failed: %s", g_strerror(errno));
 		g_io_channel_shutdown(io_chan, FALSE, NULL);
 		g_io_channel_unref(io_chan);
-		close(fd);
 		change_state(conn, SERVER_CONNECTION_STATE_NOT_CONNECTED, SERVER_CONNECTION_STATE_REASON_ERROR);
 		return FALSE;
 	}
@@ -478,9 +468,6 @@ static gboolean connect_io_func(GIOChannel *src, GIOCondition cond, gpointer dat
 	g_io_channel_unref(connect_data->io_chan);
 	connect_data->io_chan = io_chan;
 
-	close(connect_data->fd);
-	connect_data->fd = fd;
-
 	connect_data->watch_id = g_io_add_watch(io_chan, G_IO_OUT | G_IO_ERR, connect_io_func, conn);
 
 	return FALSE;
@@ -550,7 +537,6 @@ static void dns_result_callback(guint unused, IdleDNSResult *results, gpointer u
 	g_io_channel_set_encoding(io_chan, NULL, NULL);
 	g_io_channel_set_buffered(io_chan, FALSE);
 
-	priv->connect_data->fd = fd;
 	priv->connect_data->io_chan = io_chan;
 	priv->connect_data->res = results;
 	priv->connect_data->cur = cur;
@@ -573,7 +559,6 @@ static gboolean iface_disconnect_impl_full(IdleServerConnectionIface *iface, GEr
 	IdleServerConnection *conn = IDLE_SERVER_CONNECTION(iface);
 	IdleServerConnectionPrivate *priv = IDLE_SERVER_CONNECTION_GET_PRIVATE(conn);
 	GError *io_error = NULL;
-	int fd;
 	GIOStatus status;
 
 	g_assert(priv != NULL);
@@ -590,7 +575,6 @@ static gboolean iface_disconnect_impl_full(IdleServerConnectionIface *iface, GEr
 	}
 
 	if (priv->io_chan != NULL) {
-		fd = g_io_channel_unix_get_fd(priv->io_chan);
 		status = g_io_channel_shutdown(priv->io_chan, TRUE, &io_error);
 
 		if (status != G_IO_STATUS_NORMAL && io_error) {
@@ -598,10 +582,6 @@ static gboolean iface_disconnect_impl_full(IdleServerConnectionIface *iface, GEr
 			g_error_free(io_error);
 		}
 
-		if (close(fd)) {
-			IDLE_DEBUG("unable to close fd: %s", g_strerror(errno));
-		}
-
 		g_io_channel_unref(priv->io_chan);
 		priv->io_chan = NULL;
 	}
-- 
1.5.6.5