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 1F924431FB6 for ; Sun, 23 Jun 2013 05:20:00 -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 TgnN++6RnrOF for ; Sun, 23 Jun 2013 05:19:51 -0700 (PDT) Received: from mail-pa0-f41.google.com (mail-pa0-f41.google.com [209.85.220.41]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 2F58B431FAE for ; Sun, 23 Jun 2013 05:19:51 -0700 (PDT) Received: by mail-pa0-f41.google.com with SMTP id bj3so9838371pad.0 for ; Sun, 23 Jun 2013 05:19:50 -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=ZW6UzJMwrRRJSXPcRbbiWxrZ+/TepwD+4SVexBmvwQQ=; b=qINvEgDsn5Ss7pmOy2YbNEETI3t+L2MuEt8sNB2Oam3/BlvMXtpUqKz88F7ZlTUbbt fT3pdIcw9amIjFoBJz0tcsxgGycAi0vEJABBz4NPaARtilglz6iQ1oMNx3tv08k+Ad2W 8iN++1zrWhbbqKbdX0O1Xem+wOmNI44dGgHIrEKn6OkbB5k2dv4zBVfLuUs6J6dmzy7I WvJAxOGszZf15AGnW8f81acq0jOFQIt5PxlpX/8oM+fC0xJSI/IZaZY/Os5egT8DqbgR aOPgczQ2lHGF/LVAFZHglYldfAIrygdp2PM2wtpOXrsKli4jFUrDEz4rAdeQWI3xM+dM w48Q== X-Received: by 10.66.160.97 with SMTP id xj1mr12932797pab.5.1371989988343; Sun, 23 Jun 2013 05:19:48 -0700 (PDT) Received: from localhost (215.42.233.220.static.exetel.com.au. [220.233.42.215]) by mx.google.com with ESMTPSA id ri8sm13336924pbc.3.2013.06.23.05.19.45 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 23 Jun 2013 05:19:47 -0700 (PDT) Date: Sun, 23 Jun 2013 22:19:42 +1000 Message-ID: <20130623221942.GA18938@hili.localdomain> From: Peter Wang To: Mark Walters Subject: Re: [PATCH v7 03/12] cli: add insert command In-Reply-To: <8761x5uy3a.fsf@qmul.ac.uk> References: <1371961445-15182-1-git-send-email-novalazy@gmail.com> <1371961445-15182-4-git-send-email-novalazy@gmail.com> <8761x5uy3a.fsf@qmul.ac.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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, 23 Jun 2013 12:20:00 -0000 On Sun, 23 Jun 2013 07:42:49 +0100, Mark Walters wrote: > > This is a +1 modulo one small bug (I think) I found below. I am happy to > delay the fail-on-index-fail option, especially as that will need some > bikeshedding on its name. > > Also when posting new versions please include a diff from the previous > version (this is more difficult if there was significant rebasing). This > makew the v6 to v7 change obvious (the one comment change and the > bugfix). > > Moreover doing the diff with v4 (which I happen to have locally) I > found the bug below. > > Best wishes > > Mark > > > ... > > +static notmuch_bool_t > > +insert_message (void *ctx, notmuch_database_t *notmuch, int fdin, > > + const char *dir, tag_op_list_t *tag_ops) > > +{ > > + char *tmppath; > > + char *newpath; > > + char *newdir; > > + int fdout; > > + char *cleanup_path; > > + > > + fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir); > > + if (fdout < 0) > > + 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)); > > + 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 > > + * simply use rename() instead of link() and unlink(). > > + * See also: http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery > > + */ > > + if (rename (tmppath, newpath) != 0) { > > + fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno)); > > + goto FAIL; > > + } > > + > > + if (! sync_dir (newdir)) > > + goto FAIL; > > I think cleanup_path needs to be updated before the sync_dir is test as > newpath should be unlinked rather than oldpath. (v4 explicitly unlinked newpath) > Thanks for the continued close review. I'll post a followup to this specific patch. Peter