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

[Jeffrey Stedfast <fejj ximian com>]

On Tue, 2003-06-03 at 19:02, Not Zed wrote:
> 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.

I was wondering this myself, but figured I'd go for consistancy with
what grep gave me :-)

but yea, I agree that it should likely be 0666 and for mkdir we should
probably use 0777.

> 
> 
>         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.

*nod*

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

ya got me there. I have no idea. I was kinda wondering this myself.

> 
> +               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!

the camel_write() code at least checks for EINTR, etc. which is what I
had meant. But yea, if we get an error, it's all over anyway.

> 
> 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.

yea :\

> 
> 
> 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.

agreed. perhaps tomorrow I can rewrite it to not suck?

now the question is... what do we do if we fail to save all the uids? I
think that no matter what we will want to overwrite the old copy, since
this way at least we won't re-download as many messages? (well, assuming
new_st.st_size >= old_st.st_size - if that isn't true, then it'd likely
depend on whether or not the user is using keep-mail-on-server or
something.)

Jeff

>  
> 
> 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
-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com