[glib] Add g_close(), use it

From: Colin Walters <walters src gnome org>
To: commits-list gnome org
Cc:
Subject: [glib] Add g_close(), use it
Date: Tue, 29 Jan 2013 15:29:46 +0000 (UTC)
commit f398bec5bcc0d924e2401c76a6b94133e9490835
Author: Colin Walters <walters verbum org>
Date:   Fri Jan 25 12:05:26 2013 -0500

    Add g_close(), use it
    
    There are two benefits to this:
    
    1) We can centralize any operating system specific knowledge of
       close-vs-EINTR handling.  For example, while on Linux we should never
       retry, if someone cared enough later about HP-UX, they could come by
       and change this one spot.
    2) For places that do care about the return value and want to provide
       the caller with a GError, this function makes it convenient to do so.
    
    Note that gspawn.c had an incorrect EINTR loop-retry around close().
    
    https://bugzilla.gnome.org/show_bug.cgi?id=682819

 docs/reference/glib/glib-sections.txt |    1 +
 gio/gapplicationimpl-dbus.c           |    3 +-
 gio/gdbusprivate.c                    |    3 +-
 gio/gdbusserver.c                     |    4 ++-
 gio/gdesktopappinfo.c                 |    6 +++-
 gio/gfile.c                           |   15 ++++++++--
 gio/glocalfile.c                      |    7 +++-
 gio/glocalfileinfo.c                  |    3 +-
 gio/glocalfileinputstream.c           |   24 +++++++---------
 gio/glocalfileoutputstream.c          |   49 +++++++++++++--------------------
 gio/gnetworkmonitornetlink.c          |    5 ++-
 glib/glib-unix.c                      |    1 -
 glib/gspawn.c                         |   14 +++-------
 glib/gstdio.c                         |   43 +++++++++++++++++++++++++++++
 glib/gstdio.h                         |    4 +++
 15 files changed, 115 insertions(+), 67 deletions(-)
---
diff --git a/docs/reference/glib/glib-sections.txt b/docs/reference/glib/glib-sections.txt
index 927d4f7..fb160a3 100644
--- a/docs/reference/glib/glib-sections.txt
+++ b/docs/reference/glib/glib-sections.txt
@@ -1256,6 +1256,7 @@ g_access
 g_creat
 g_chdir
 g_utime
+g_close
 
 <SUBSECTION Private>
 g_file_error_quark
diff --git a/gio/gapplicationimpl-dbus.c b/gio/gapplicationimpl-dbus.c
index 4239640..5afb0d0 100644
--- a/gio/gapplicationimpl-dbus.c
+++ b/gio/gapplicationimpl-dbus.c
@@ -32,6 +32,7 @@
 #include "gdbusconnection.h"
 #include "gdbusintrospection.h"
 #include "gdbuserror.h"
+#include "glib/gstdio.h"
 
 #include <string.h>
 #include <stdio.h>
@@ -709,7 +710,7 @@ g_dbus_command_line_get_stdin (GApplicationCommandLine *cmdline)
       fds = g_unix_fd_list_steal_fds (fd_list, &n_fds);
       result = g_unix_input_stream_new (fds[0], TRUE);
       for (i = 1; i < n_fds; i++)
-        close (fds[i]);
+        (void) g_close (fds[i], NULL);
       g_free (fds);
     }
 
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index cda0b07..0e5bef2 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -39,6 +39,7 @@
 #include "ginputstream.h"
 #include "gmemoryinputstream.h"
 #include "giostream.h"
+#include "glib/gstdio.h"
 #include "gsocketcontrolmessage.h"
 #include "gsocketconnection.h"
 #include "gsocketoutputstream.h"
@@ -621,7 +622,7 @@ _g_dbus_worker_do_read_cb (GInputStream  *input_stream,
                     {
                       /* TODO: really want a append_steal() */
                       g_unix_fd_list_append (worker->read_fd_list, fds[n], NULL);
-                      close (fds[n]);
+                      (void) g_close (fds[n], NULL);
                     }
                 }
               g_free (fds);
diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c
index e132b70..02508e6 100644
--- a/gio/gdbusserver.c
+++ b/gio/gdbusserver.c
@@ -45,6 +45,7 @@
 #include "gsocketservice.h"
 #include "gthreadedsocketservice.h"
 #include "gresolver.h"
+#include "glib/gstdio.h"
 #include "ginetaddress.h"
 #include "ginetsocketaddress.h"
 #include "ginputstream.h"
@@ -878,7 +879,8 @@ try_tcp (GDBusServer  *server,
           bytes_written += ret;
           bytes_remaining -= ret;
         }
-      close (fd);
+      if (!g_close (fd, error))
+        goto out;
       file_escaped = g_uri_escape_string (server->nonce_file, "/\\", FALSE);
       server->client_address = g_strdup_printf ("nonce-tcp:host=%s,port=%d,noncefile=%s",
                                                 host,
diff --git a/gio/gdesktopappinfo.c b/gio/gdesktopappinfo.c
index bb1c99c..c60939c 100644
--- a/gio/gdesktopappinfo.c
+++ b/gio/gdesktopappinfo.c
@@ -33,6 +33,9 @@
 
 #include "gcontenttypeprivate.h"
 #include "gdesktopappinfo.h"
+#ifdef G_OS_UNIX
+#include "glib-unix.h"
+#endif
 #include "gfile.h"
 #include "gioerror.h"
 #include "gthemedicon.h"
@@ -2100,7 +2103,8 @@ g_desktop_app_info_ensure_saved (GDesktopAppInfo  *info,
 
   desktop_id = g_path_get_basename (filename);
 
-  close (fd);
+  /* FIXME - actually handle error */
+  (void) g_close (fd, NULL);
   
   res = g_file_set_contents (filename, data, data_size, error);
   g_free (data);
diff --git a/gio/gfile.c b/gio/gfile.c
index 3d78d72..04b5239 100644
--- a/gio/gfile.c
+++ b/gio/gfile.c
@@ -46,6 +46,7 @@
 #endif
 
 #include "gfile.h"
+#include "glib/gstdio.h"
 #ifdef G_OS_UNIX
 #include "glib-unix.h"
 #endif
@@ -2843,7 +2844,7 @@ splice_stream_with_progress (GInputStream           *in,
                              gpointer                progress_callback_data,
                              GError                **error)
 {
-  int buffer[2];
+  int buffer[2] = { -1, -1 };
   gboolean res;
   goffset total_size;
   loff_t offset_in;
@@ -2907,9 +2908,17 @@ splice_stream_with_progress (GInputStream           *in,
   if (progress_callback)
     progress_callback (offset_in, total_size, progress_callback_data);
 
+  if (!g_close (buffer[0], error))
+    goto out;
+  buffer[0] = -1;
+  if (!g_close (buffer[1], error))
+    goto out;
+  buffer[1] = -1;
  out:
-  close (buffer[0]);
-  close (buffer[1]);
+  if (buffer[0] != -1)
+    (void) g_close (buffer[0], NULL);
+  if (buffer[1] != -1)
+    (void) g_close (buffer[1], NULL);
 
   return res;
 }
diff --git a/gio/glocalfile.c b/gio/glocalfile.c
index 9617754..fe17e0c 100644
--- a/gio/glocalfile.c
+++ b/gio/glocalfile.c
@@ -64,6 +64,9 @@
 #include "gioerror.h"
 #include <glib/gstdio.h>
 #include "glibintl.h"
+#ifdef G_OS_UNIX
+#include "glib-unix.h"
+#endif
 
 #ifdef G_OS_WIN32
 #include <windows.h>
@@ -1366,7 +1369,7 @@ g_local_file_read (GFile         *file,
 
   if (ret == 0 && S_ISDIR (buf.st_mode))
     {
-      close (fd);
+      (void) g_close (fd, NULL);
       g_set_error_literal (error, G_IO_ERROR,
                            G_IO_ERROR_IS_DIRECTORY,
                            _("Can't open directory"));
@@ -2056,7 +2059,7 @@ g_local_file_trash (GFile         *file,
       return FALSE;
     }
 
-  close (fd);
+  (void) g_close (fd, NULL);
 
   /* TODO: Maybe we should verify that you can delete the file from the trash
      before moving it? OTOH, that is hard, as it needs a recursive scan */
diff --git a/gio/glocalfileinfo.c b/gio/glocalfileinfo.c
index 98f7664..90a074b 100644
--- a/gio/glocalfileinfo.c
+++ b/gio/glocalfileinfo.c
@@ -63,6 +63,7 @@
 #include <gvfs.h>
 
 #ifndef G_OS_WIN32
+#include "glib-unix.h"
 #include "glib-private.h"
 #endif
 #include "glibintl.h"
@@ -1260,7 +1261,7 @@ get_content_type (const char          *basename,
 	      ssize_t res;
 	      
 	      res = read (fd, sniff_buffer, sniff_length);
-	      close (fd);
+	      (void) g_close (fd, NULL);
 	      if (res >= 0)
 		{
 		  g_free (content_type);
diff --git a/gio/glocalfileinputstream.c b/gio/glocalfileinputstream.c
index 5aa436d..bb05bb6 100644
--- a/gio/glocalfileinputstream.c
+++ b/gio/glocalfileinputstream.c
@@ -39,6 +39,7 @@
 #include "glibintl.h"
 
 #ifdef G_OS_UNIX
+#include "glib-unix.h"
 #include "gfiledescriptorbased.h"
 #endif
 
@@ -239,7 +240,6 @@ g_local_file_input_stream_close (GInputStream  *stream,
 				 GError       **error)
 {
   GLocalFileInputStream *file;
-  int res;
 
   file = G_LOCAL_FILE_INPUT_STREAM (stream);
 
@@ -249,22 +249,18 @@ g_local_file_input_stream_close (GInputStream  *stream,
   if (file->priv->fd == -1)
     return TRUE;
 
-  while (1)
+  if (!g_close (file->priv->fd, NULL))
     {
-      res = close (file->priv->fd);
-      if (res == -1)
-        {
-          int errsv = errno;
-
-          g_set_error (error, G_IO_ERROR,
-                       g_io_error_from_errno (errsv),
-                       _("Error closing file: %s"),
-                       g_strerror (errsv));
-        }
-      break;
+      int errsv = errno;
+      
+      g_set_error (error, G_IO_ERROR,
+                   g_io_error_from_errno (errsv),
+                   _("Error closing file: %s"),
+                   g_strerror (errsv));
+      return FALSE;
     }
 
-  return res != -1;
+  return TRUE;
 }
 
 
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index 59d8639..22fefc4 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -222,7 +222,6 @@ _g_local_file_output_stream_really_close (GLocalFileOutputStream *file,
 					  GError        **error)
 {
   GLocalFileStat final_stat;
-  int res;
 
 #ifdef HAVE_FSYNC
   if (file->priv->sync_on_close &&
@@ -246,8 +245,7 @@ _g_local_file_output_stream_really_close (GLocalFileOutputStream *file,
   if (_fstati64 (file->priv->fd, &final_stat) == 0)
     file->priv->etag = _g_local_file_info_create_etag (&final_stat);
 
-  res = close (file->priv->fd);
-  if (res == -1)
+  if (!g_close (file->priv->fd, NULL))
     {
       int errsv = errno;
       
@@ -341,34 +339,25 @@ _g_local_file_output_stream_really_close (GLocalFileOutputStream *file,
   if (fstat (file->priv->fd, &final_stat) == 0)
     file->priv->etag = _g_local_file_info_create_etag (&final_stat);
 
-  while (1)
+  if (!g_close (file->priv->fd, NULL))
     {
-      res = close (file->priv->fd);
-      if (res == -1)
-	{
-          int errsv = errno;
-
-	  g_set_error (error, G_IO_ERROR,
-		       g_io_error_from_errno (errsv),
-		       _("Error closing file: %s"),
-		       g_strerror (errsv));
-	}
-      break;
+      int errsv = errno;
+      
+      g_set_error (error, G_IO_ERROR,
+                   g_io_error_from_errno (errsv),
+                   _("Error closing file: %s"),
+                   g_strerror (errsv));
+      goto err_out;
     }
-  
-  return res != -1;
-
-#else
-
-  return TRUE;
 
 #endif
-
+  
+  return TRUE;
  err_out:
 
 #ifndef G_OS_WIN32
   /* A simple try to close the fd in case we fail before the actual close */
-  close (file->priv->fd);
+  (void) g_close (file->priv->fd, NULL);
 #endif
   if (file->priv->tmp_filename)
     g_unlink (file->priv->tmp_filename);
@@ -938,14 +927,14 @@ handle_overwrite_open (const char    *filename,
 	      original_stat.st_gid != tmp_statbuf.st_gid ||
 	      original_stat.st_mode != tmp_statbuf.st_mode)
 	    {
-	      close (tmpfd);
+	      (void) g_close (tmpfd, NULL);
 	      g_unlink (tmp_filename);
 	      g_free (tmp_filename);
 	      goto fallback_strategy;
 	    }
 	}
 
-      close (fd);
+      (void) g_close (fd, NULL);
       *temp_filename = tmp_filename;
       return tmpfd;
     }
@@ -1014,7 +1003,7 @@ handle_overwrite_open (const char    *filename,
                                    G_IO_ERROR_CANT_CREATE_BACKUP,
                                    _("Backup file creation failed"));
 	      g_unlink (backup_filename);
-	      close (bfd);
+	      (void) g_close (bfd, NULL);
 	      g_free (backup_filename);
 	      goto err_out;
 	    }
@@ -1028,13 +1017,13 @@ handle_overwrite_open (const char    *filename,
                                G_IO_ERROR_CANT_CREATE_BACKUP,
                                _("Backup file creation failed"));
 	  g_unlink (backup_filename);
-	  close (bfd);
+          (void) g_close (bfd, NULL);
 	  g_free (backup_filename);
 	  
 	  goto err_out;
 	}
       
-      close (bfd);
+      (void) g_close (bfd, NULL);
       g_free (backup_filename);
 
       /* Seek back to the start of the file after the backup copy */
@@ -1052,7 +1041,7 @@ handle_overwrite_open (const char    *filename,
 
   if (flags & G_FILE_CREATE_REPLACE_DESTINATION)
     {
-      close (fd);
+      (void) g_close (fd, NULL);
       
       if (g_unlink (filename) != 0)
 	{
@@ -1104,7 +1093,7 @@ handle_overwrite_open (const char    *filename,
   return fd;
 
  err_out:
-  close (fd);
+  (void) g_close (fd, NULL);
  err_out2:
   return -1;
 }
diff --git a/gio/gnetworkmonitornetlink.c b/gio/gnetworkmonitornetlink.c
index f909ca9..17df1da 100644
--- a/gio/gnetworkmonitornetlink.c
+++ b/gio/gnetworkmonitornetlink.c
@@ -29,6 +29,7 @@
 #include "ginitable.h"
 #include "giomodule-priv.h"
 #include "glibintl.h"
+#include "glib/gstdio.h"
 #include "gnetworkingprivate.h"
 #include "gnetworkmonitor.h"
 #include "gsocket.h"
@@ -108,7 +109,7 @@ g_network_monitor_netlink_initable_init (GInitable     *initable,
       g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv),
                    _("Could not create network monitor: %s"),
                    g_strerror (errno));
-      close (sockfd);
+      (void) g_close (sockfd, NULL);
       return FALSE;
     }
 
@@ -116,7 +117,7 @@ g_network_monitor_netlink_initable_init (GInitable     *initable,
   if (error)
     {
       g_prefix_error (error, "%s", _("Could not create network monitor: "));
-      close (sockfd);
+      (void) g_close (sockfd, NULL);
       return FALSE;
     }
 
diff --git a/glib/glib-unix.c b/glib/glib-unix.c
index 0f12cc4..26f1509 100644
--- a/glib/glib-unix.c
+++ b/glib/glib-unix.c
@@ -179,7 +179,6 @@ g_unix_set_fd_nonblocking (gint       fd,
 #endif
 }
 
-
 /**
  * g_unix_signal_source_new:
  * @signum: A signal number
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 3545a78..381ed5c 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -43,6 +43,7 @@
 
 #include "gspawn.h"
 #include "gthread.h"
+#include "glib/gstdio.h"
 
 #include "genviron.h"
 #include "gmem.h"
@@ -150,23 +151,16 @@ g_spawn_async (const gchar          *working_directory,
  * on a file descriptor twice, and another thread has
  * re-opened it since the first close)
  */
-static gint
+static void
 close_and_invalidate (gint *fd)
 {
-  gint ret;
-
   if (*fd < 0)
-    return -1;
+    return;
   else
     {
-    again:
-      ret = close (*fd);
-      if (ret == -1 && errno == EINTR)
-	goto again;
+      (void) g_close (*fd, NULL);
       *fd = -1;
     }
-
-  return ret;
 }
 
 /* Some versions of OS X define READ_OK in public headers */
diff --git a/glib/gstdio.c b/glib/gstdio.c
index 71431f1..bbdad5b 100644
--- a/glib/gstdio.c
+++ b/glib/gstdio.c
@@ -835,3 +835,46 @@ g_utime (const gchar    *filename,
   return utime (filename, utb);
 #endif
 }
+
+/**
+ * g_close:
+ * @fd: A file descriptor
+ * @error: a #GError
+ *
+ * This wraps the close() call; in case of error, %errno will be
+ * preserved, but the error will also be stored as a #GError in @error.
+ *
+ * Besides using #GError, there is another major reason to prefer this
+ * function over the call provided by the system; on Unix, it will
+ * attempt to correctly handle %EINTR, which has platform-specific
+ * semantics.
+ */
+gboolean
+g_close (gint       fd,
+         GError   **error)
+{
+  int res;
+  res = close (fd);
+  /* Just ignore EINTR for now; a retry loop is the wrong thing to do
+   * on Linux at least.  Anyone who wants to add a conditional check
+   * for e.g. HP-UX is welcome to do so later...
+   *
+   * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
+   * https://bugzilla.gnome.org/show_bug.cgi?id=682819
+   * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
+   * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
+   */
+  if (G_UNLIKELY (res == -1 && errno == EINTR))
+    return TRUE;
+  else if (res == -1)
+    {
+      int errsv = errno;
+      g_set_error_literal (error, G_FILE_ERROR,
+                           g_file_error_from_errno (errsv),
+                           g_strerror (errsv));
+      errno = errsv;
+      return FALSE;
+    }
+  return TRUE;
+}
+
diff --git a/glib/gstdio.h b/glib/gstdio.h
index 238d60e..90ee74e 100644
--- a/glib/gstdio.h
+++ b/glib/gstdio.h
@@ -163,6 +163,10 @@ int g_utime     (const gchar    *filename,
 
 #endif /* G_OS_UNIX */
 
+GLIB_AVAILABLE_IN_2_36
+gboolean g_close (gint       fd,
+                  GError   **error);
+
 G_END_DECLS
 
 #endif /* __G_STDIO_H__ */
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]