On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner <[email protected]> wrote: > Peter Wang <[email protected]> writes: > > > - cleanup_path = tmppath; > > - > > - if (! copy_stdin (fdin, fdout)) > > - goto FAIL; > > + if (! copy_stdin (fdin, fdout)) { > > + close (fdout); > > + unlink (tmppath); > > + return FALSE; > > + } > > I'm not completely convinced by replacement of the "goto FAIL" with the > multiple returns. I'd lean to towards being consistent with the notmuch > codebase unless the FAIL block is really horrendous
Eh, when I came back to the code I found it unnecessary convoluted. However, you can squash in the attached patch if you like. As an objective measure, the function with the FAIL block is longer. > > Is there a good reason to use TRUE and FALSE for return values rather > than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be > overall slightly simpler in notmuch_insert_command. Not sure what you have in mind. I think CLI exit codes should be confined to notmuch_insert_command. Peter
>From be07c53d6d5f22ced723a44f996323d2a982113e Mon Sep 17 00:00:00 2001 From: Peter Wang <[email protected]> Date: Sun, 6 Jul 2014 12:52:26 +1000 Subject: [PATCH] goto fail --- notmuch-insert.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/notmuch-insert.c b/notmuch-insert.c index 7db4f73..8dfc8bb 100644 --- a/notmuch-insert.c +++ b/notmuch-insert.c @@ -342,26 +342,25 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath) { char *tmppath; char *newdir; + char *cleanup_path; int fdout; fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir); if (fdout < 0) return FALSE; - if (! copy_stdin (fdin, fdout)) { - close (fdout); - unlink (tmppath); - return FALSE; - } + cleanup_path = tmppath; + + if (! copy_stdin (fdin, fdout)) + goto FAIL; if (fsync (fdout) != 0) { fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno)); - close (fdout); - unlink (tmppath); - return FALSE; + goto FAIL; } close (fdout); + fdout = -1; /* Atomically move the new message file from the Maildir 'tmp' directory * to the 'new' directory. We follow the Dovecot recommendation to @@ -370,16 +369,21 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath) */ if (rename (tmppath, *newpath) != 0) { fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno)); - unlink (tmppath); - return FALSE; + goto FAIL; } - if (! sync_dir (newdir)) { - unlink (*newpath); - return FALSE; - } + cleanup_path = *newpath; + + if (! sync_dir (newdir)) + goto FAIL; return TRUE; + + FAIL: + if (fdout >= 0) + close (fdout); + unlink (cleanup_path); + return FALSE; } int -- 1.8.4
_______________________________________________ notmuch mailing list [email protected] http://notmuchmail.org/mailman/listinfo/notmuch
