Re: [evolution-patches] camel-uid-cache.c robustness patch

[Jeffrey Stedfast <fejj ximian com>]

Here's another go at it, removed the mkdir_hier() code and #include'd
camel-store.h for that implementation (so we don't have to have 2 copies
of it).

Also took care of all the issues (except the g_strsplit() issue) that
were brought up in the previous patch.

You'll note that a failed write() won't necessarily mean we won't still
overwrite the previous cache with the new cache. there's some (hopefully
correct) logic to figure out if we should try to overwrite the old cache
anyway in an attempt to save as much as we can.

There's a biggish comment in the code explaining the logic so I won't
repeat it in my mail.

Jeff

-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com

? 43406-again.patch
? ChangeLog.nonximian
? body
? body.c
? body.txt
? camel-mime-filter-windows.c
? camel-mime-filter-windows.h
? charset-map.c
? charset-map.diff
? cmsutil.c
? date.patch
? debug.patch
? ehlo.patch
? evolution-1.3-gpg.patch
? folder-info-build.patch
? foo
? html-filter-broken.msg
? imap
? invalid-content-id.patch
? iso
? iso.c
? pop3-uidl.patch
? smime
? uid-cache.patch
? win-charset.patch
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.1823
diff -u -r1.1823 ChangeLog
--- ChangeLog	1 Jun 2003 23:49:27 -0000	1.1823
+++ ChangeLog	4 Jun 2003 18:55:07 -0000
@@ -1,3 +1,23 @@
+2003-06-04  Jeffrey Stedfast  <fejj ximian com>
+
+	* camel-uid-cache.c (camel_uid_cache_new): Create the directory
+	with mode 0777 and the cache file itself with mode 0666. Let the
+	user's umask filter the permissions. Instead of saving the fd on
+	the Cache object, instead save the filename. Use camel_read()
+	instead of expecting read() to just always work without getting an
+	EINTR/etc.
+	(maybe_write_uid): Don't do anything if cache->fd == -1, this
+	means an error has occured in a previous callback. Replace the 2
+	calls to write() with camel_write() and check their return
+	values. If either of them fails, set cache->fd to -1 (GHashTable
+	doesn't give us a way to abort foreach'ing thru the table).
+	(camel_uid_cache_save): Save to a temp file instead of overwriting
+	the original. Do proper error checking, etc. Also added some
+	smarts about whether to try and overwrite the old cache even if we
+	haven't successfully saved all the uids in the cache.
+	(camel_uid_cache_destroy): Free the cache->filename, no longer
+	need to close (cache->fd).
+
 2003-06-01  Jeffrey Stedfast  <fejj ximian com>
 
 	* broken-date-parser.c (d): Turn off debugging.
Index: Makefile.am
===================================================================
RCS file: /cvs/gnome/evolution/camel/Makefile.am,v
retrieving revision 1.182
diff -u -r1.182 Makefile.am
--- Makefile.am	30 Apr 2003 15:35:28 -0000	1.182
+++ Makefile.am	4 Jun 2003 18:55:07 -0000
@@ -49,6 +49,7 @@
 	camel-http-stream.c			\
 	camel-index.c				\
 	camel-internet-address.c		\
+	camel-io.c				\
 	camel-lock.c				\
 	camel-lock-client.c			\
 	camel-medium.c				\
@@ -150,6 +151,7 @@
 	camel-http-stream.h			\
 	camel-index.h				\
 	camel-internet-address.h		\
+	camel-io.h				\
 	camel-i18n.h				\
 	camel-lock.h				\
 	camel-lock-client.h			\
Index: camel-io.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-io.c,v
retrieving revision 1.1
diff -u -r1.1 camel-io.c
--- camel-io.c	3 Mar 2003 22:53:15 -0000	1.1
+++ camel-io.c	4 Jun 2003 18:55:07 -0000
@@ -35,6 +35,10 @@
 #include "camel-operation.h"
 
 
+#ifndef MAX
+#define MAX(a,b) ((a) > (b) ? (a) : (b))
+#endif
+
 /* FIXME: should we trade out select() for a poll() instead? */
 
 ssize_t
Index: camel-uid-cache.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-uid-cache.c,v
retrieving revision 1.10
diff -u -r1.10 camel-uid-cache.c
--- camel-uid-cache.c	2 Nov 2002 00:42:07 -0000	1.10
+++ camel-uid-cache.c	4 Jun 2003 18:55:07 -0000
@@ -32,6 +32,8 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "camel-io.h"
+#include "camel-store.h"      /* for camel_mkdir_hier */
 #include "camel-uid-cache.h"
 
 struct _uid_state {
@@ -39,33 +41,6 @@
 	gboolean save;
 };
 
-static void free_uid (gpointer key, gpointer value, gpointer data);
-static void maybe_write_uid (gpointer key, gpointer value, gpointer data);
-
-
-static int
-mkdir_heir (const char *path, mode_t mode)
-{
-	char *copy, *p;
-	
-	p = copy = g_strdup (path);
-	do {
-		p = strchr (p + 1, '/');
-		if (p)
-			*p = '\0';
-		if (access (copy, F_OK) == -1) {
-			if (mkdir (copy, mode) == -1) {
-				g_free (copy);
-				return -1;
-			}
-		}
-		if (p)
-			*p = '/';
-	} while (p);
-	
-	g_free (copy);
-	return 0;
-}
 
 /**
  * camel_uid_cache_new:
@@ -86,30 +61,38 @@
 	int fd, i;
 	
 	dirname = g_path_get_dirname (filename);
-	mkdir_heir (dirname, 0700);
-	g_free (dirname);
+	if (camel_mkdir_hier (dirname, 0777) == -1) {
+		g_free (dirname);
+		return NULL;
+	}
 	
-	fd = open (filename, O_RDWR | O_CREAT, 0700);
-	if (fd == -1)
+	if ((fd = open (filename, O_RDWR | O_CREAT, 0666)) == -1)
 		return NULL;
 	
-	if (fstat (fd, &st) != 0) {
+	if (fstat (fd, &st) == -1) {
 		close (fd);
 		return NULL;
 	}
+	
 	buf = g_malloc (st.st_size + 1);
 	
-	if (read (fd, buf, st.st_size) == -1) {
+	if (st.st_size > 0 && camel_read (fd, buf, st.st_size) == -1) {
 		close (fd);
 		g_free (buf);
 		return NULL;
 	}
+	
 	buf[st.st_size] = '\0';
 	
+	close (fd);
+	
 	cache = g_new (CamelUIDCache, 1);
-	cache->fd = fd;
-	cache->level = 1;
 	cache->uids = g_hash_table_new (g_str_hash, g_str_equal);
+	cache->filename = g_strdup (filename);
+	cache->level = 1;
+	cache->expired = 0;
+	cache->size = 0;
+	cache->fd = -1;
 	
 	uids = g_strsplit (buf, "\n", 0);
 	g_free (buf);
@@ -122,11 +105,37 @@
 		
 		g_hash_table_insert (cache->uids, uids[i], state);
 	}
+	
 	g_free (uids);
 	
 	return cache;
 }
 
+
+static void
+maybe_write_uid (gpointer key, gpointer value, gpointer data)
+{
+	CamelUIDCache *cache = data;
+	struct _uid_state *state = value;
+	
+	if (cache->fd == -1)
+		return;
+	
+	if (state && state->level == cache->level && state->save) {
+		if (camel_write (cache->fd, key, strlen (key)) == -1 || 
+		    camel_write (cache->fd, "\n", 1) == -1) {
+			cache->fd = -1;
+		} else {
+			cache->size += strlen (key) + 1;
+		}
+	} else {
+		/* keep track of how much space the expired uids would
+		 * have taken up in the cache */
+		cache->expired += strlen (key) + 1;
+	}
+}
+
+
 /**
  * camel_uid_cache_save:
  * @cache: a CamelUIDCache
@@ -138,22 +147,85 @@
 gboolean
 camel_uid_cache_save (CamelUIDCache *cache)
 {
-	if (lseek (cache->fd, 0, SEEK_SET) != 0)
+	struct stat st;
+	char *filename;
+	int errnosav;
+	int fd;
+	
+	filename = g_strdup_printf ("%s~", cache->filename);
+	if ((fd = open (filename, O_WRONLY | O_CREAT | O_EXCL, 0666)) == -1) {
+		g_free (filename);
 		return FALSE;
+	}
+	
+	cache->fd = fd;
+	cache->size = 0;
+	cache->expired = 0;
 	g_hash_table_foreach (cache->uids, maybe_write_uid, cache);
-	return ftruncate (cache->fd, lseek (cache->fd, 0, SEEK_CUR)) == 0;
+	
+	if (cache->fd == -1)
+		goto exception;
+	
+ overwrite:
+	if (fsync (fd) == -1)
+		goto exception;
+	
+	close (fd);
+	fd = -1;
+	
+	if (rename (filename, cache->filename) == -1)
+		goto exception;
+	
+	g_free (filename);
+	
+	return TRUE;
+	
+ exception:
+	
+	errnosav = errno;
+	
+	if (fd != -1) {
+		/**
+		 * If our new cache size is larger than the old cache,
+		 * even if we haven't finished writing it out
+		 * successfully, we should still attempt to replace
+		 * the old cache with the new cache because it will at
+		 * least avoid re-downloading a few extra messages
+		 * than if we just kept the old cache.
+		 *
+		 * Similarly, even if the new cache size is smaller
+		 * than the old cache size, but we've expired enough
+		 * uids to make up for the difference in size (or
+		 * more), then we should replace the old cache with
+		 * the new cache as well.
+		 **/
+		
+		if (stat (cache->filename, &st) == 0 &&
+		    (cache->size > st.st_size || cache->size + cache->expired > st.st_size)) {
+			if (ftruncate (fd, (off_t) cache->size) != -1) {
+				cache->size = 0;
+				cache->expired = 0;
+				goto overwrite;
+			}
+		}
+		
+		close (fd);
+	}
+	
+	unlink (filename);
+	g_free (filename);
+	
+	errno = errnosav;
+	
+	return FALSE;
 }
 
+
 static void
-maybe_write_uid (gpointer key, gpointer value, gpointer data)
+free_uid (gpointer key, gpointer value, gpointer data)
 {
-	CamelUIDCache *cache = data;
-	struct _uid_state *state = value;
-	
-	if (state && state->level == cache->level && state->save) {
-		write (cache->fd, key, strlen (key));
-		write (cache->fd, "\n", 1);
-	}
+	g_free (key);
+	g_free (value);
 }
 
 
@@ -168,15 +240,8 @@
 {
 	g_hash_table_foreach (cache->uids, free_uid, NULL);
 	g_hash_table_destroy (cache->uids);
-	close (cache->fd);
+	g_free (cache->filename);
 	g_free (cache);
-}
-
-static void
-free_uid (gpointer key, gpointer value, gpointer data)
-{
-	g_free (key);
-	g_free (value);
 }
 
 
Index: camel-uid-cache.h
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-uid-cache.h,v
retrieving revision 1.6
diff -u -r1.6 camel-uid-cache.h
--- camel-uid-cache.h	27 Oct 2001 16:59:27 -0000	1.6
+++ camel-uid-cache.h	4 Jun 2003 18:55:07 -0000
@@ -30,12 +30,18 @@
 #pragma }
 #endif /* __cplusplus */
 
-#include <stdio.h>
 #include <glib.h>
 
+#include <stdio.h>
+#include <sys/types.h>
+
 typedef struct {
-	int fd, level;
+	char *filename;
 	GHashTable *uids;
+	unsigned int level;
+	size_t expired;
+	size_t size;
+	int fd;
 } CamelUIDCache;
 
 CamelUIDCache *camel_uid_cache_new (const char *filename);