From: Jani Nikula Date: Wed, 4 Dec 2013 18:40:19 +0000 (+0100) Subject: Re: [PATCH 1/2] lib: add return status to database close and destroy X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=471209553292b0fc7bd21b4b23b0c03736aea96d;p=notmuch-archives.git Re: [PATCH 1/2] lib: add return status to database close and destroy --- diff --git a/9f/6b4502423abe2c7a616aa275f749361606a098 b/9f/6b4502423abe2c7a616aa275f749361606a098 new file mode 100644 index 000000000..7a7ae6e2c --- /dev/null +++ b/9f/6b4502423abe2c7a616aa275f749361606a098 @@ -0,0 +1,220 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 09437431FD2 + for ; Wed, 4 Dec 2013 10:40:47 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[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 MgAPnpA6ZwOR for ; + Wed, 4 Dec 2013 10:40:39 -0800 (PST) +Received: from mail-qe0-f53.google.com (mail-qe0-f53.google.com + [209.85.128.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 81F7B431FD0 + for ; Wed, 4 Dec 2013 10:40:39 -0800 (PST) +Received: by mail-qe0-f53.google.com with SMTP id nc12so14632638qeb.40 + for ; Wed, 04 Dec 2013 10:40:39 -0800 (PST) +X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; + d=1e100.net; s=20130820; + h=x-gm-message-state:from:to:cc:subject:in-reply-to:references + :user-agent:date:message-id:mime-version:content-type; + bh=H3AnR/WHVt9NqSYD2Xsm24B7qu1fT96IxNbbgD2Iuvs=; + b=J7Pg+drn/5r3oRflblzLM0w4eS5Bw37kSDhG782GLoSJeogBUMPF3JDhD6yW4eHe7Q + +v2KBnQNWIdEo1RBUwjgxGz1QKXkSJbSIT1xl7WmiqckasMvFUi8OE+6myXnzMmJ9Jo6 + VZhXgYQRGp6oGaFwKIk3DHlSDsZT/kagZM+oMVcQTShwOYef4VppdyUNOyv4K2Mkua+p + +R+7HY1dmeOUxjO0SWSJCfCJkuvnP7AG7YZsdqbhzNl0j6fBDxfwiUfVQS1om7FeaSL9 + CUTtVlkn7d/JjHE4uXlQ9GInd6uBQUKQ5WEs9jNvK7rrXfwecxnCAubZEA9i3YJMqlNc + bFMQ== +X-Gm-Message-State: + ALoCoQlByi/hNkAMF3+cPcbjZrKoNff0LtMMLMrkONRF+2OhYFADob6sisC5t6nbWk/QRoDyMU6W +X-Received: by 10.224.75.200 with SMTP id z8mr138784229qaj.71.1386182438919; + Wed, 04 Dec 2013 10:40:38 -0800 (PST) +Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3]) + by mx.google.com with ESMTPSA id hb2sm20160543qeb.6.2013.12.04.10.40.38 + for + (version=TLSv1.1 cipher=RC4-SHA bits=128/128); + Wed, 04 Dec 2013 10:40:38 -0800 (PST) +From: Jani Nikula +To: Austin Clements +Subject: Re: [PATCH 1/2] lib: add return status to database close and destroy +In-Reply-To: <20131204163132.GC8854@mit.edu> +References: + <29b808bb6bf051fe21b6a72f12bb4ad1badfbf97.1385903109.git.jani@nikula.org> + <20131204163132.GC8854@mit.edu> +User-Agent: Notmuch/0.17~rc2+4~gd7b0a0a (http://notmuchmail.org) Emacs/23.2.1 + (x86_64-pc-linux-gnu) +Date: Wed, 04 Dec 2013 19:40:19 +0100 +Message-ID: <87ob4wsbn0.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +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: Wed, 04 Dec 2013 18:40:47 -0000 + +On Wed, 04 Dec 2013, Austin Clements wrote: +> Quoth Jani Nikula on Dec 01 at 3:13 pm: +>> notmuch_database_close may fail in Xapian ->flush() or ->close(), so +>> report the status. Similarly for notmuch_database_destroy which calls +>> close. +>> +>> This is required for notmuch insert to report error status if message +>> indexing failed. +>> +>> Bump the notmuch version to allow clients to conditional build against +>> both the current release and the next release (current git master). +>> --- +>> lib/database.cc | 18 ++++++++++++++---- +>> lib/notmuch.h | 17 ++++++++++++++--- +>> 2 files changed, 28 insertions(+), 7 deletions(-) +>> +>> diff --git a/lib/database.cc b/lib/database.cc +>> index f395061..98e2c31 100644 +>> --- a/lib/database.cc +>> +++ b/lib/database.cc +>> @@ -767,14 +767,17 @@ notmuch_database_open (const char *path, +>> return status; +>> } +>> +>> -void +>> +notmuch_status_t +>> notmuch_database_close (notmuch_database_t *notmuch) +>> { +>> + notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; +>> + +>> try { +>> if (notmuch->xapian_db != NULL && +>> notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE) +>> (static_cast (notmuch->xapian_db))->flush (); +>> } catch (const Xapian::Error &error) { +>> + status = NOTMUCH_STATUS_XAPIAN_EXCEPTION; +>> if (! notmuch->exception_reported) { +>> fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n", +>> error.get_msg().c_str()); +>> @@ -789,6 +792,7 @@ notmuch_database_close (notmuch_database_t *notmuch) +>> notmuch->xapian_db->close(); +>> } catch (const Xapian::Error &error) { +>> /* do nothing */ +>> + status = NOTMUCH_STATUS_XAPIAN_EXCEPTION; +>> } +>> } +>> +>> @@ -802,6 +806,8 @@ notmuch_database_close (notmuch_database_t *notmuch) +>> notmuch->value_range_processor = NULL; +>> delete notmuch->date_range_processor; +>> notmuch->date_range_processor = NULL; +>> + +>> + return status; +>> } +>> +>> #if HAVE_XAPIAN_COMPACT +>> @@ -966,7 +972,7 @@ notmuch_database_compact (const char *path, +>> +>> DONE: +>> if (notmuch) +>> - notmuch_database_destroy (notmuch); +>> + ret = notmuch_database_destroy (notmuch); +> +> This will clobber the error code on most of the error paths. Maybe +> you want to only set ret if it's currently NOTMUCH_STATUS_SUCCESS? + +Good point, will fix. + +BR, +Jani. + + +> +>> +>> talloc_free (local); +>> +>> @@ -984,11 +990,15 @@ notmuch_database_compact (unused (const char *path), +>> } +>> #endif +>> +>> -void +>> +notmuch_status_t +>> notmuch_database_destroy (notmuch_database_t *notmuch) +>> { +>> - notmuch_database_close (notmuch); +>> + notmuch_status_t status; +>> + +>> + status = notmuch_database_close (notmuch); +>> talloc_free (notmuch); +>> + +>> + return status; +>> } +>> +>> const char * +>> diff --git a/lib/notmuch.h b/lib/notmuch.h +>> index 7c3a30c..dbdce86 100644 +>> --- a/lib/notmuch.h +>> +++ b/lib/notmuch.h +>> @@ -42,7 +42,7 @@ NOTMUCH_BEGIN_DECLS +>> #endif +>> +>> #define NOTMUCH_MAJOR_VERSION 0 +>> -#define NOTMUCH_MINOR_VERSION 17 +>> +#define NOTMUCH_MINOR_VERSION 18 +>> #define NOTMUCH_MICRO_VERSION 0 +> +> This is actually what got me thinking about +> id:1386173986-9624-1-git-send-email-amdragon@mit.edu (which obviously +> conflicts). In particular, the Python bindings don't have access to +> these macros, and can only use the soname version. (I think the Go +> ones can use the macros; the Ruby ones certainly can.) +> +>> +>> /* +>> @@ -236,8 +236,16 @@ notmuch_database_open (const char *path, +>> * +>> * notmuch_database_close can be called multiple times. Later calls +>> * have no effect. +>> + * +>> + * Return value: +>> + * +>> + * NOTMUCH_STATUS_SUCCESS: Successfully closed the database. +>> + * +>> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the +>> + * database has been closed but there are no guarantees the +>> + * changes to the database, if any, have been flushed to disk. +>> */ +>> -void +>> +notmuch_status_t +>> notmuch_database_close (notmuch_database_t *database); +>> +>> /* A callback invoked by notmuch_database_compact to notify the user +>> @@ -263,8 +271,11 @@ notmuch_database_compact (const char* path, +>> +>> /* Destroy the notmuch database, closing it if necessary and freeing +>> * all associated resources. +>> + * +>> + * Return value as in notmuch_database_close if the database was open; +>> + * notmuch_database_destroy itself has no failure modes. +>> */ +>> -void +>> +notmuch_status_t +>> notmuch_database_destroy (notmuch_database_t *database); +>> +>> /* Return the database path of the given database. +> +> Regarding bindings (since you asked me to think about them), these +> should be a safe changes from an ABI perspective (though obviously the +> bindings will need changes to take advantage of the new return code).