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

[Not Zed <notzed ximian com>]

some comments inline


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 3 Jun 2003 18:57:58 -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  3 Jun 2003 18:57:58 -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   3 Jun 2003 18:57:58 -0000
@@ -32,6 +32,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "camel-io.h"
 #include "camel-uid-cache.h"
 
 struct _uid_state {
@@ -89,7 +90,7 @@
        mkdir_heir (dirname, 0700);
        g_free (dirname);
        
-       fd = open (filename, O_RDWR | O_CREAT, 0700);
+       fd = open (filename, O_RDWR | O_CREAT, 0600);


 ---- we should probably use 0666 for this (and most other things too,
for that matter, a problem i've been guilty of many times).  User can
use umask to control the actual perms used.


        if (fd == -1)
                return NULL;
        
@@ -99,7 +100,7 @@
        }
        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;
@@ -113,16 +114,19 @@
        
        uids = g_strsplit (buf, "\n", 0);
        g_free (buf);
-       for (i = 0; uids[i]; i++) {
-               struct _uid_state *state;
-               
-               state = g_new (struct _uid_state, 1);
-               state->level = cache->level;
-               state->save = TRUE;
+       if (uids != NULL) {

 ----- this is wrong, the original code was right, g_strsplit returns an
empty vector, not a null pointer, for no input.

Infact, the whole function is a bit weird, why isn't it just using
stdio, and fgets() ?

+               for (i = 0; uids[i]; i++) {
+                       struct _uid_state *state;
+                       
+                       state = g_new (struct _uid_state, 1);
+                       state->level = cache->level;
+                       state->save = TRUE;
+                       
+                       g_hash_table_insert (cache->uids, uids[i],
state);
+               }
                
-               g_hash_table_insert (cache->uids, uids[i], state);
+               g_free (uids);
        }
-       g_free (uids);
        
        return cache;
 }
@@ -150,9 +154,10 @@
        CamelUIDCache *cache = data;
        struct _uid_state *state = value;
        
+       /* FIXME: what should we do if a write() fails? abort? or what?
*/
        if (state && state->level == cache->level && state->save) {
-               write (cache->fd, key, strlen (key));
-               write (cache->fd, "\n", 1);
+               camel_write (cache->fd, key, strlen (key));
+               camel_write (cache->fd, "\n", 1);
        }
 }

 ------ so in what way does this give 'free' error checking, you dont
even check for errors!

This should probably quit the loop on error, or at least pass it out,
but i guess it doesn't matter.  Pity about ghashtable's awful api.


FWIW, The whole thing of keeping an fd open for the duration of the
object ... seems really wrong.  And the write method just re-writing
over the existing one and then truncating it (without even an fsync)
seems also wrong.
 

On Wed, 2003-06-04 at 04:44, Jeffrey Stedfast wrote:
> Attached patch makes the uid cache code more robust wrt handling
> read/write errors by using the camel-io functions.
> 
> Jeff