Re: g_file_write()

["Alexis S. L. Carvalho" <alexis cecm usp br>]

Thus spake Soeren Sandmann:
> Havoc Pennington <hp redhat com> writes:
> 
> > Should it do the atomic write thing? (write to a temporary file
> > alongside the new file, then rename() to the final filename)
> > 
> > The minus is that you need double the size of the file to write it, but
> > the plus is that you aren't screwed if you run out of disk space mid-
> > overwrite.
> 
> I think it makes sense to do the atomic write. Here is a new patch
> that does that. It also uses the g_* versions where appropriate and
> hopefully doesn't leak.

How reliable do you want this atomic write to be?  To handle Havoc's
"disk full" scenario, I think the patch is mostly ok - but see a few
comments below.

To survive the case where the program/OS/computer crashes during the
operation, you'd need more stuff, and it'd be harder to guarantee that
it'd work on all OS+filesystem combinations.

A few comments, assuming the first case:

> Index: gfileutils.c
> ===================================================================
> RCS file: /cvs/gnome/glib/glib/gfileutils.c,v
> retrieving revision 1.59
> diff -u -r1.59 gfileutils.c
> --- gfileutils.c	24 Feb 2005 23:46:36 -0000	1.59
> +++ gfileutils.c	6 Mar 2005 23:14:24 -0000
> @@ -811,6 +811,180 @@
>  
>  #endif
>  
> +static gchar *
> +write_to_temp_file (const gchar *contents,
> +		    gint length,

maybe gssize instead of gint? (see below)

> +  
> +  tmp_name = g_strdup_printf (".%s.XXXXXX", template);
> +

You'll have problems if template also specifies a directory (e.g.
foo/bar , /home/user/.foorc , ./foo ).  Also, will Windows et al handle
dot-files correctly?

> +  fd = g_mkstemp (tmp_name);

> +  file = g_fopen (tmp_name, "wb");

This is kind of ugly and leaves a race between file creation and
re-opening.  I'm guessing there would be portability problems with
fdopen?

> +  /* We have a FILE pointer now, so close the filedescriptor */

stale comment

> +  
> +  retval = g_strdup (tmp_name);
> +
> + out:
> +  if (fd != -1)
> +    close (fd);
> +  g_free (tmp_name);
> +  g_free (display_name);
> +  
> +  return retval;
> +}

Maybe replace the "g_strdup (tmp_name)" with 

  retval = tmp_name;
  tmp_name = NULL;

to avoid another allocation?

Also, if the g_fopen/fwrite/fclose fails, you're leaving the temp file
behind.

> +gboolean
> +g_file_write (const gchar *filename,
> +	      const gchar *contents,
> +	      gsize	   length,

This should probably be a gssize since -1 is valid and
write_to_temp_file works with signed variables.

> +  if (g_file_test (filename, G_FILE_TEST_EXISTS) && g_unlink (filename) == -1)

> +  if (g_rename (tmp_filename, filename) == -1)

As Havoc mentioned, the unlink is not only not needed, but actually
harmful on Unix, since you lose atomicity.

> Index: gmain.c
> ===================================================================
> RCS file: /cvs/gnome/glib/glib/gmain.c,v
> retrieving revision 1.125
> diff -u -r1.125 gmain.c
> --- gmain.c	8 Nov 2004 18:49:35 -0000	1.125
> +++ gmain.c	6 Mar 2005 23:14:25 -0000

I'm guessing this part was accidentally included.

HTH

Alexis