[Patch v5 2/6] dump: when given output file name, write atomically

[Austin Clements]

(Pardon the mobile review)

On Apr 1, 2014 9:16 PM, David Bremner <david at tethera.net> wrote:
>
> It is useful to able to tell whether a dump completed successfully in 
> situtions where we don't have access to the return code.

"Situations."  This commit message doesn't seem very related to atomicity?

> --- 
> notmuch-dump.c | 61 +++++++++++++++++++++++++++++++++++++++++++++------------- 
> 1 file changed, 48 insertions(+), 13 deletions(-) 
>
> diff --git a/notmuch-dump.c b/notmuch-dump.c 
> index 28342b7..05ed6b4 100644 
> --- a/notmuch-dump.c 
> +++ b/notmuch-dump.c 
> @@ -129,30 +129,65 @@ notmuch_database_dump (notmuch_database_t *notmuch, 
> { 
>      gzFile output; 
>      const char *mode = gzip_output ? "w9" : "wT"; 
> +    const char *name_for_error = output_file_name ? output_file_name : "stdout"; 
>
> -    int ret; 
> +    char *tempname = NULL; 
> +    int outfd = -1; 
> + 
> +    int ret = -1; 
> + 
> +    if (output_file_name) { 
> + tempname = talloc_asprintf (notmuch, "%s.XXXXXX", output_file_name); 
> + outfd = mkstemp (tempname); 
> +    } else { 
> + outfd = fileno (stdout); 
> +    } 
>
> -    if (output_file_name) 
> - output = gzopen (output_file_name, mode); 
> -    else 
> - output = gzdopen (fileno (stdout), mode); 
> +    if (outfd < 0) { 
> + fprintf (stderr, "Bad output file %s\n", name_for_error); 
> + goto DONE; 
> +    } 
> + 
> +    output = gzdopen (outfd, mode); 
>
>      if (output == NULL) { 
> fprintf (stderr, "Error opening %s for (gzip) writing: %s\n", 
> - output_file_name ? output_file_name : "stdout", strerror (errno)); 
> - return EXIT_FAILURE; 
> + name_for_error, strerror (errno)); 
> + goto DONE; 
>      } 
>
>      ret = database_dump_file (notmuch, output, query_str, output_format); 
> +    if (ret) goto DONE; 
>
> -    if (gzflush (output, Z_FINISH)) { 
> - fprintf (stderr, "Error flushing output: %s\n", 
> - gzerror (output, NULL)); 
> - return EXIT_FAILURE; 
> +    ret = gzflush (output, Z_FINISH); 
> +    if (ret) { 
> + fprintf (stderr, "Error flushing output: %s\n", gzerror (output, NULL)); 
> + goto DONE; 
>      } 
>
> -    if (output_file_name) 
> - gzclose_w (output); 
> +    ret = fdatasync (outfd);

This may fail if outfd isn't a regular file.  We probably should only do this if output_file_name.  Or we could ignore EINVAL (but that may mask bugs).

> +    if (ret) { 
> + perror ("fdatasync");

perror's fine for quick hacks, but it would be better to give a real error message here.

> + goto DONE; 
> +    } 
> + 
> +    if (output_file_name) { 
> + ret = gzclose_w (output); 
> + if (ret != Z_OK) { 
> +     ret = EXIT_FAILURE; 
> +     goto DONE; 
> + } 
> + 
> + ret = rename (tempname, output_file_name); 
> + if (ret) { 
> +     perror ("rename"); 
> +     goto DONE; 
> + } 
> + 
> +    } 
> + DONE: 
> +    if (ret != EXIT_SUCCESS && output_file_name) 
> + (void) unlink (tempname);

We need to gzclose on the error paths to free zlib's internal resources.  This is a problem if we handed stdout to gzdopen.  We shouldn't write any more to stdout anyway in that case, but maybe it would be better to dup stdout?

>
>      return ret; 
> } 
> -- 
> 1.9.0 
>
> _______________________________________________ 
> notmuch mailing list 
> notmuch at notmuchmail.org 
> http://notmuchmail.org/mailman/listinfo/notmuch