From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 428F1431FB6 for ; Sat, 5 Jul 2014 20:57:44 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.799 X-Spam-Level: X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qs09UJcXr8Xq for ; Sat, 5 Jul 2014 20:57:37 -0700 (PDT) Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com [209.85.220.42]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id DE0AF431FAF for ; Sat, 5 Jul 2014 20:57:36 -0700 (PDT) Received: by mail-pa0-f42.google.com with SMTP id lj1so3704622pab.1 for ; Sat, 05 Jul 2014 20:57:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:message-id:from:to:cc:subject:in-reply-to:references :mime-version:content-type:content-disposition :content-transfer-encoding; bh=4zEpWlBODIKCL7+LnthhP5ZJlT/YM4xoRMvrncBiaDw=; b=ZvRhxU2Fz9kSXcPiwOTDmEvS/Wn5UtmuBo4lUgjBYzvFAnCqhnVNJ8XvcvAqi0KfCN 0YYDpoivnpxOHk1l9YgANTq92TE0UUIetB7RTRcRrL7c6zzreYQgfVeqtjV15D19VkGx mqMLZeh1jkWdCwCtUbl6/iHHDq82dP0mY0tRa5G5aC6KHnPASAQhMaZRoGGTrGPwSgh5 cfvAo/015b1jQYKtvYFgI0L00P27jnhEvhrUHj/NpFgTm5h0afNntBaEIrbcBg2LdFCi VV8rp/FzhdjnDTyhocalErCnrqzL6w83O+MG8y7JoWmDZxYvf2H9U9TDAFHftXpomq0w MqaA== X-Received: by 10.68.160.163 with SMTP id xl3mr20977648pbb.39.1404619055900; Sat, 05 Jul 2014 20:57:35 -0700 (PDT) Received: from localhost (215.42.233.220.static.exetel.com.au. [220.233.42.215]) by mx.google.com with ESMTPSA id r3sm14819829pdd.8.2014.07.05.20.57.32 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 05 Jul 2014 20:57:34 -0700 (PDT) Date: Sun, 6 Jul 2014 13:57:28 +1000 Message-ID: <20140706135728.GE1142@hili.localdomain> From: Peter Wang To: David Bremner Subject: Re: [PATCH v2 06/10] cli: refactor insert In-Reply-To: <87simgq702.fsf@maritornes.cs.unb.ca> References: <1397653165-15620-1-git-send-email-novalazy@gmail.com> <1397653165-15620-7-git-send-email-novalazy@gmail.com> <87simgq702.fsf@maritornes.cs.unb.ca> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="uhW2wu+cthCu47Lg" Content-Disposition: inline Content-Transfer-Encoding: 8bit Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Jul 2014 03:57:44 -0000 --uhW2wu+cthCu47Lg Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner wrote: > Peter Wang 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 --uhW2wu+cthCu47Lg Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="0001-goto-fail.patch" Content-Transfer-Encoding: 8bit >From be07c53d6d5f22ced723a44f996323d2a982113e Mon Sep 17 00:00:00 2001 From: Peter Wang 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 --uhW2wu+cthCu47Lg--