49/2d7b1ac0337d2ddbc46f8e779ec2e5bc429d66

   1 Return-Path: <m.walters@qmul.ac.uk>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 82114431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat, 31 Mar 2012 10:17:14 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id Y0UdHVOkPrGa for <notmuch@notmuchmail.org>;\r
  17         Sat, 31 Mar 2012 10:17:13 -0700 (PDT)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 54593431FAE\r
  22         for <notmuch@notmuchmail.org>; Sat, 31 Mar 2012 10:17:13 -0700 (PDT)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1SE1vG-0003RQ-ID; Sat, 31 Mar 2012 18:17:09 +0100\r
  27 Received: from 94-192-233-223.zone6.bethere.co.uk ([94.192.233.223]\r
  28         helo=localhost)\r
  29         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  30         (envelope-from <m.walters@qmul.ac.uk>)\r
  31         id 1SE1vG-0000RL-3i; Sat, 31 Mar 2012 18:17:06 +0100\r
  32 From: Mark Walters <markwalters1009@gmail.com>\r
  33 To: Justus Winter <4winter@informatik.uni-hamburg.de>, notmuch@notmuchmail.org\r
  34 Subject: Re: [PATCH 1/7] Split notmuch_database_close into two functions\r
  35 In-Reply-To:\r
  36  <1332291311-28954-2-git-send-email-4winter@informatik.uni-hamburg.de>\r
  37 References:\r
  38  <1332291311-28954-1-git-send-email-4winter@informatik.uni-hamburg.de>\r
  39         <1332291311-28954-2-git-send-email-4winter@informatik.uni-hamburg.de>\r
  40 User-Agent: Notmuch/0.12+88~gb9fb613 (http://notmuchmail.org) Emacs/23.3.1\r
  41         (x86_64-pc-linux-gnu)\r
  42 Date: Sat, 31 Mar 2012 18:17:15 +0100\r
  43 Message-ID: <87fwcopu5g.fsf@qmul.ac.uk>\r
  44 MIME-Version: 1.0\r
  45 Content-Type: text/plain; charset=us-ascii\r
  46 X-Sender-Host-Address: 94.192.233.223\r
  47 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  48 X-QM-Body-MD5: afb81fd1a941e6f44213228b1e25b7d9 (of first 20000 bytes)\r
  49 X-SpamAssassin-Score: -1.8\r
  50 X-SpamAssassin-SpamBar: -\r
  51 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  52         determine if it is\r
  53         spam. We require at least 5.0 points to mark a message as spam.\r
  54         This message scored -1.8 points.\r
  55         Summary of the scoring: \r
  56         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  57         *      medium trust\r
  58         *      [138.37.6.40 listed in list.dnswl.org]\r
  59         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  60         provider *      (markwalters1009[at]gmail.com)\r
  61         * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay\r
  62         *      domain\r
  63         *  0.5 AWL AWL: From: address is in the auto white-list\r
  64 X-QM-Scan-Virus: ClamAV says the message is clean\r
  65 X-BeenThere: notmuch@notmuchmail.org\r
  66 X-Mailman-Version: 2.1.13\r
  67 Precedence: list\r
  68 List-Id: "Use and development of the notmuch mail system."\r
  69         <notmuch.notmuchmail.org>\r
  70 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  72 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  73 List-Post: <mailto:notmuch@notmuchmail.org>\r
  74 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  75 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  76         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  77 X-List-Received-Date: Sat, 31 Mar 2012 17:17:14 -0000\r
  78 \r
  79 Justus Winter <4winter@informatik.uni-hamburg.de> writes:\r
  80 \r
  81 > Formerly notmuch_database_close closed the xapian database and\r
  82 > destroyed the talloc structure associated with the notmuch database\r
  83 > object. Split notmuch_database_close into notmuch_database_close and\r
  84 > notmuch_database_destroy.\r
  85 >\r
  86 > This makes it possible for long running programs to close the xapian\r
  87 > database and thus release the lock associated with it without\r
  88 > destroying the data structures obtained from it.\r
  89 >\r
  90 > This also makes the api more consistent since every other data\r
  91 > structure has a destructor function.\r
  92 \r
  93 I like the idea of this series but have two queries before reviewing it.\r
  94 \r
  95 The first is a concern that if we change the library functions we should\r
  96 update the library version otherwise out of tree users won't know which\r
  97 to call. (I don't actually know how versioning is done but I think we\r
  98 should at least be able to make new out-of-tree code cope with the old\r
  99 or new version). It might be worth keeping notmuch_database_close as it\r
 100 is for now and adding something like notmuch_database_weak_close with\r
 101 the new functionality. Then whenever the library version is next going\r
 102 to get bumped we could move to the destroy/close nomenclature.\r
 103 \r
 104 Secondly, I think the patch series could be made clearer and easier to\r
 105 review. If you do it in three steps\r
 106 \r
 107 1) change of notmuch_database_close to notmuch_database_destroy (just\r
 108    the function name change)\r
 109 2) split the new notmuch_database_destroy into two as in the current\r
 110    first patch\r
 111 3) Make any changes (if there are any) of notmuch_database_destroy to\r
 112    notmuch_database_close.\r
 113 \r
 114 The advantage is that the first change is easy to test (essentially does\r
 115 it build) and then changes from notmuch_database_destroy to\r
 116 notmuch_database_close in step 3 are explicit rather than the current\r
 117 situation where we need to grep the code to see if some instances of\r
 118 notmuch_database_close were not changed to notmuch_database_destroy.\r
 119 \r
 120 Of course if the decision is to go via the weak_close version then you\r
 121 just need to do the analogues of 2 and 3.\r
 122 \r
 123 Best wishes\r
 124 \r
 125 Mark\r
 126 \r
 127 >\r
 128 > Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>\r
 129 > ---\r
 130 >  lib/database.cc |   14 ++++++++++++--\r
 131 >  lib/notmuch.h   |   15 +++++++++++----\r
 132 >  2 files changed, 23 insertions(+), 6 deletions(-)\r
 133 >\r
 134 > diff --git a/lib/database.cc b/lib/database.cc\r
 135 > index 16c4354..2fefcad 100644\r
 136 > --- a/lib/database.cc\r
 137 > +++ b/lib/database.cc\r
 138 > @@ -642,7 +642,7 @@ notmuch_database_open (const char *path,\r
 139 >                        "       read-write mode.\n",\r
 140 >                        notmuch_path, version, NOTMUCH_DATABASE_VERSION);\r
 141 >               notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;\r
 142 > -             notmuch_database_close (notmuch);\r
 143 > +             notmuch_database_destroy (notmuch);\r
 144 >               notmuch = NULL;\r
 145 >               goto DONE;\r
 146 >           }\r
 147 > @@ -702,7 +702,7 @@ notmuch_database_open (const char *path,\r
 148 >      } catch (const Xapian::Error &error) {\r
 149 >       fprintf (stderr, "A Xapian exception occurred opening database: %s\n",\r
 150 >                error.get_msg().c_str());\r
 151 > -     notmuch_database_close (notmuch);\r
 152 > +     notmuch_database_destroy (notmuch);\r
 153 >       notmuch = NULL;\r
 154 >      }\r
 155 >  \r
 156 > @@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)\r
 157 >      }\r
 158 >  \r
 159 >      delete notmuch->term_gen;\r
 160 > +    notmuch->term_gen = NULL;\r
 161 >      delete notmuch->query_parser;\r
 162 > +    notmuch->query_parser = NULL;\r
 163 >      delete notmuch->xapian_db;\r
 164 > +    notmuch->xapian_db = NULL;\r
 165 >      delete notmuch->value_range_processor;\r
 166 > +    notmuch->value_range_processor = NULL;\r
 167 > +}\r
 168 > +\r
 169 > +void\r
 170 > +notmuch_database_destroy (notmuch_database_t *notmuch)\r
 171 > +{\r
 172 > +    notmuch_database_close (notmuch);\r
 173 >      talloc_free (notmuch);\r
 174 >  }\r
 175 >  \r
 176 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 177 > index babd208..6114f36 100644\r
 178 > --- a/lib/notmuch.h\r
 179 > +++ b/lib/notmuch.h\r
 180 > @@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;\r
 181 >   *\r
 182 >   * After a successful call to notmuch_database_create, the returned\r
 183 >   * database will be open so the caller should call\r
 184 > - * notmuch_database_close when finished with it.\r
 185 > + * notmuch_database_destroy when finished with it.\r
 186 >   *\r
 187 >   * The database will not yet have any data in it\r
 188 >   * (notmuch_database_create itself is a very cheap function). Messages\r
 189 > @@ -165,7 +165,7 @@ typedef enum {\r
 190 >   * An existing notmuch database can be identified by the presence of a\r
 191 >   * directory named ".notmuch" below 'path'.\r
 192 >   *\r
 193 > - * The caller should call notmuch_database_close when finished with\r
 194 > + * The caller should call notmuch_database_destroy when finished with\r
 195 >   * this database.\r
 196 >   *\r
 197 >   * In case of any failure, this function returns NULL, (after printing\r
 198 > @@ -175,11 +175,18 @@ notmuch_database_t *\r
 199 >  notmuch_database_open (const char *path,\r
 200 >                      notmuch_database_mode_t mode);\r
 201 >  \r
 202 > -/* Close the given notmuch database, freeing all associated\r
 203 > - * resources. See notmuch_database_open. */\r
 204 > +/* Close the given notmuch database.\r
 205 > + *\r
 206 > + * This function is called by notmuch_database_destroyed and can be\r
 207 > + * called multiple times. */\r
 208 >  void\r
 209 >  notmuch_database_close (notmuch_database_t *database);\r
 210 >  \r
 211 > +/* Destroy the notmuch database freeing all associated\r
 212 > + * resources */\r
 213 > +void\r
 214 > +notmuch_database_destroy (notmuch_database_t *database);\r
 215 > +\r
 216 >  /* Return the database path of the given database.\r
 217 >   *\r
 218 >   * The return value is a string owned by notmuch so should not be\r
 219 > -- \r
 220 > 1.7.9.1\r
 221 >\r
 222 > _______________________________________________\r
 223 > notmuch mailing list\r
 224 > notmuch@notmuchmail.org\r
 225 > http://notmuchmail.org/mailman/listinfo/notmuch\r