[gvfs/nfs] Address another round of reviews

[Ross Lagerwall <rossl src gnome org>]

commit 8c21c76edf7051748648d5f8195a82222ce8f468
Author: Ross Lagerwall <rosslagerwall gmail com>
Date:   Sat Jan 31 16:13:33 2015 +0000

    Address another round of reviews

 daemon/gvfsbackendnfs.c |  102 +++++++++++++++++++++++++++++-----------------
 1 files changed, 64 insertions(+), 38 deletions(-)
---
diff --git a/daemon/gvfsbackendnfs.c b/daemon/gvfsbackendnfs.c
index 845576f..4a52043 100644
--- a/daemon/gvfsbackendnfs.c
+++ b/daemon/gvfsbackendnfs.c
@@ -87,18 +87,28 @@ g_vfs_backend_nfs_init (GVfsBackendNfs *backend)
 }
 
 static void
-g_vfs_backend_nfs_finalize (GObject *object)
+g_vfs_backend_nfs_destroy_context (GVfsBackendNfs *backend)
 {
-  GVfsBackendNfs *backend = G_VFS_BACKEND_NFS (object);
+  if (backend->ctx)
+    {
+      nfs_destroy_context (backend->ctx);
+      backend->ctx = NULL;
+    }
 
   if (backend->source)
     {
       g_source_destroy (backend->source);
       g_source_unref (backend->source);
+      backend->source = NULL;
     }
+}
 
-  if (backend->ctx)
-    nfs_destroy_context (backend->ctx);
+static void
+g_vfs_backend_nfs_finalize (GObject *object)
+{
+  GVfsBackendNfs *backend = G_VFS_BACKEND_NFS (object);
+
+  g_vfs_backend_nfs_destroy_context (backend);
 
   if (G_OBJECT_CLASS (g_vfs_backend_nfs_parent_class)->finalize)
     (*G_OBJECT_CLASS (g_vfs_backend_nfs_parent_class)->finalize) (object);
@@ -117,14 +127,8 @@ nfs_source_prepare (GSource *source, gint *timeout)
 
   if (fd < 0)
     {
-      GVfsBackendNfs *backend = nfs_source->backend;
-
-      g_vfs_backend_force_unmount (G_VFS_BACKEND (backend));
-      nfs_destroy_context (nfs_source->ctx);
-      backend->ctx = NULL;
-      g_source_destroy (source);
-      g_source_unref (source);
-      backend->source = NULL;
+      g_vfs_backend_force_unmount (G_VFS_BACKEND (nfs_source->backend));
+      g_vfs_backend_nfs_destroy_context (nfs_source->backend);
     }
   else if (fd != nfs_source->fd)
     {
@@ -152,16 +156,10 @@ nfs_source_dispatch (GSource *source, GSourceFunc callback, gpointer user_data)
                      g_source_query_unix_fd (source, nfs_source->tag));
   if (err)
     {
-      GVfsBackendNfs *backend = nfs_source->backend;
-
       g_warning ("nfs_service error: %d, %s\n",
                  err, nfs_get_error (nfs_source->ctx));
-      g_vfs_backend_force_unmount (G_VFS_BACKEND (backend));
-      nfs_destroy_context (nfs_source->ctx);
-      backend->ctx = NULL;
-      g_source_destroy (source);
-      g_source_unref (source);
-      backend->source = NULL;
+      g_vfs_backend_force_unmount (G_VFS_BACKEND (nfs_source->backend));
+      g_vfs_backend_nfs_destroy_context (nfs_source->backend);
     }
 
   return G_SOURCE_CONTINUE;
@@ -182,8 +180,8 @@ do_mount (GVfsBackend *backend,
   const char *host;
   char *basename, *display_name, *export = NULL;
   int err;
-  int pathlen = strlen (mount_spec->mount_prefix);
-  int exportlen = 0;
+  size_t pathlen = strlen (mount_spec->mount_prefix);
+  size_t exportlen = SIZE_MAX;
   static GSourceFuncs nfs_source_callbacks = {
     nfs_source_prepare,
     NULL,
@@ -194,25 +192,34 @@ do_mount (GVfsBackend *backend,
   host = g_mount_spec_get (mount_spec, "host");
   export_list = mount_getexports (host);
 
-  /* Find the longest matching mount. E.g. if the given mount_prefix is
-   * /some/long/path * and there exist two mounts, /some and /some/long, match
-   * against the latter. */
+  /* Find the shortest matching mount. E.g. if the given mount_prefix is
+   * /some/long/path and there exist two mounts, /some and /some/long, match
+   * against /some. */
   for (ptr = export_list; ptr; ptr = ptr->ex_next)
     {
+      /* First check that the NFS mount point is a prefix of the mount_prefix. */
       if (g_str_has_prefix (mount_spec->mount_prefix, ptr->ex_dir))
         {
-          int this_exportlen = strlen (ptr->ex_dir);
+          size_t this_exportlen = strlen (ptr->ex_dir);
 
+          /* Check if the mount_prefix is longer than the NFS mount point.
+           * E.g. mount_prefix: /mnt/file, mount point: /mnt */
           if (pathlen > this_exportlen)
             {
+              /* Check if the mount_prefix has a slash at the correct point.
+               * E.g. if the mount point is /mnt, then it's a match if the
+               * mount_prefix is /mnt/a but not a match if the mount_prefix is
+               * /mnta.  Choose it if it is the shortest found so far. */
               char *s = mount_spec->mount_prefix + this_exportlen;
-              if (*s == '/' && this_exportlen > exportlen)
+              if (*s == '/' && this_exportlen < exportlen)
                 {
                   export = ptr->ex_dir;
                   exportlen = this_exportlen;
                 }
             }
-          else if (this_exportlen > exportlen)
+          /* The mount_prefix and NFS mount point are identical.  Choose it if
+           * it is the shortest found so far. */
+          else if (this_exportlen < exportlen)
             {
               export = ptr->ex_dir;
               exportlen = this_exportlen;
@@ -246,6 +253,7 @@ do_mount (GVfsBackend *backend,
         {
           g_vfs_job_failed_from_errno (G_VFS_JOB (job), -err);
         }
+      g_free (export);
       return;
     }
 
@@ -276,6 +284,7 @@ do_mount (GVfsBackend *backend,
   g_mount_spec_set_mount_prefix (nfs_mount_spec, export);
   g_vfs_backend_set_mount_spec (backend, nfs_mount_spec);
   g_mount_spec_unref (nfs_mount_spec);
+  g_free (export);
 
   /* cache the process's umask for later */
   op_backend->umask = umask (0);
@@ -399,11 +408,11 @@ try_read (GVfsBackend *backend,
   return TRUE;
 }
 
-static char *
+static const char *
 set_type_from_mode (GFileInfo *info, uint64_t mode)
 {
   GFileType type = G_FILE_TYPE_UNKNOWN;
-  char *mimetype = NULL;
+  const char *mimetype = NULL;
 
   if (S_ISREG (mode))
     type = G_FILE_TYPE_REGULAR;
@@ -445,11 +454,11 @@ set_type_from_mode (GFileInfo *info, uint64_t mode)
 
 static void
 set_name_info (GFileInfo *info,
-               char *mimetype,
+               const char *mimetype,
                const char *basename,
                GFileAttributeMatcher *matcher)
 {
-  gboolean free_mimetype = FALSE;
+  char *free_mimetype = NULL;
 
   g_file_info_set_name (info, basename);
   if (basename[0] == '.')
@@ -470,8 +479,8 @@ set_name_info (GFileInfo *info,
     {
       if (basename)
         {
-          mimetype = g_content_type_guess (basename, NULL, 0, NULL);
-          free_mimetype = TRUE;
+          free_mimetype = g_content_type_guess (basename, NULL, 0, NULL);
+          mimetype = free_mimetype;
         }
       else
         mimetype = "application/octet-stream";
@@ -511,7 +520,7 @@ set_name_info (GFileInfo *info,
     }
 
   if (free_mimetype)
-    g_free (mimetype);
+    g_free (free_mimetype);
 }
 
 static void
@@ -736,6 +745,9 @@ try_append_to (GVfsBackend *backend,
   return TRUE;
 }
 
+/* The following types and functions implement an asynchronous copy which calls
+ * a callback function with a boolean success or failure.  This is used in some
+ * cases for backup files when replacing. */
 #define COPY_BLKSIZE (64 * 1024)
 
 typedef void (*CopyFileCallback) (gboolean success, void *private_data);
@@ -850,6 +862,16 @@ copy_file (struct nfs_context *ctx,
   nfs_open_async (ctx, src, O_RDONLY, copy_open_source_cb, handle);
 }
 
+/* The replace code that follows is relatively straightforward but difficult to
+ * read due to its asynchronous nature.
+ * Firstly, the file is opened with O_EXCL.  If this fails, the file exists.
+ * Some checks on the existing file are made.  If possible, a temporary file is
+ * opened and renamed over the existing file on close.  If a backup is needed,
+ * the existing file is simply renamed to the backup.
+ * If it is not possible to create a temporary file (e.g. due to permissions),
+ * then the file is truncated and writing takes place directly into the file.
+ * However, if a backup needs to be made, the existing file is first copied
+ * into the backup file. */
 static void
 replace_trunc_cb (int err,
                   struct nfs_context *ctx,
@@ -1638,7 +1660,7 @@ enumerate_stat_cb (int err,
   if (err == 0)
     {
       struct nfs_stat_64 *st = data;
-      char *mimetype;
+      const char *mimetype;
       GFileInfo *new_info;
 
       new_info = g_file_info_new ();
@@ -1730,7 +1752,9 @@ enumerate_continue (EnumerateHandle *handle, struct nfs_context *ctx)
     }
   else
     {
-      g_vfs_job_enumerate_done (handle->op_job);
+      GVfsJobEnumerate *op_job = handle->op_job;
+      g_slice_free (EnumerateHandle, handle);
+      g_vfs_job_enumerate_done (op_job);
     }
 }
 
@@ -1856,6 +1880,7 @@ enumerate_cb (int err, struct nfs_context *ctx, void *data, void *private_data)
     }
   else
     {
+      g_slice_free (EnumerateHandle, handle);
       g_vfs_job_failed_from_errno (job, -err);
     }
 }
@@ -1937,7 +1962,8 @@ stat_cb (int err, struct nfs_context *ctx, void *data, void *private_data)
       GVfsJobQueryInfo *op_job = G_VFS_JOB_QUERY_INFO (job);
       GFileInfo *info = op_job->file_info;
       struct nfs_stat_64 *st = data;
-      char *basename, *mimetype, *etag;
+      const char *mimetype;
+      char *basename, *etag;
 
       set_info_from_stat (info, st, op_job->attribute_matcher);