ef/97d0bdce8ebbdf96f5b7d4f80242231663df9f

   1 Return-Path: <thomas@schwinge.name>\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 5E48F431FB6\r
   6         for <notmuch@notmuchmail.org>; Wed, 26 Jan 2011 08:53:15 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 0\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
  12         autolearn=unavailable\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id TQEvVxLjOM+h for <notmuch@notmuchmail.org>;\r
  16         Wed, 26 Jan 2011 08:53:15 -0800 (PST)\r
  17 Received: from smtprelay06.ispgateway.de (smtprelay06.ispgateway.de\r
  18         [80.67.31.96])\r
  19         by olra.theworths.org (Postfix) with ESMTP id ECEEA431FB5\r
  20         for <notmuch@notmuchmail.org>; Wed, 26 Jan 2011 08:53:14 -0800 (PST)\r
  21 Received: from [87.180.65.26] (helo=stokes.schwinge.homeip.net)\r
  22         by smtprelay06.ispgateway.de with esmtpa (Exim 4.68)\r
  23         (envelope-from <thomas@schwinge.name>) id 1Pi8cJ-0005eJ-ST\r
  24         for notmuch@notmuchmail.org; Wed, 26 Jan 2011 17:53:12 +0100\r
  25 Received: (qmail 24435 invoked from network); 26 Jan 2011 16:52:55 -0000\r
  26 Received: from kepler.schwinge.homeip.net (192.168.111.7)\r
  27         by stokes.schwinge.homeip.net with QMQP; 26 Jan 2011 16:52:55 -0000\r
  28 Received: (nullmailer pid 32394 invoked by uid 1000);\r
  29         Wed, 26 Jan 2011 16:52:55 -0000\r
  30 From: Thomas Schwinge <thomas@schwinge.name>\r
  31 To: Carl Worth <cworth@cworth.org>, notmuch@notmuchmail.org\r
  32 Subject: Re: [PATCH 2/3] new: Add all initial tags at once\r
  33 In-Reply-To: <1295603977-14326-4-git-send-email-sojkam1@fel.cvut.cz>\r
  34 References: <1295603977-14326-1-git-send-email-sojkam1@fel.cvut.cz>\r
  35         <1295603977-14326-4-git-send-email-sojkam1@fel.cvut.cz>\r
  36 User-Agent: Notmuch/0.5-33-g665f77b (http://notmuchmail.org) Emacs/23.2.1\r
  37         (i486-pc-linux-gnu)\r
  38 Date: Wed, 26 Jan 2011 17:52:53 +0100\r
  39 Message-ID: <87lj27vc7u.fsf@kepler.schwinge.homeip.net>\r
  40 MIME-Version: 1.0\r
  41 Content-Type: multipart/signed; boundary="=-=-=";\r
  42         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  43 X-Df-Sender: thomas@schwinge.name\r
  44 X-BeenThere: notmuch@notmuchmail.org\r
  45 X-Mailman-Version: 2.1.13\r
  46 Precedence: list\r
  47 List-Id: "Use and development of the notmuch mail system."\r
  48         <notmuch.notmuchmail.org>\r
  49 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  50         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  51 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  52 List-Post: <mailto:notmuch@notmuchmail.org>\r
  53 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  54 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  55         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  56 X-List-Received-Date: Wed, 26 Jan 2011 16:53:15 -0000\r
  57 \r
  58 --=-=-=\r
  59 Content-Type: text/plain; charset=utf-8\r
  60 Content-Transfer-Encoding: quoted-printable\r
  61 \r
  62 Hallo!\r
  63 \r
  64 On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrot=\r
  65 e:\r
  66 > --- a/notmuch-new.c\r
  67 > +++ b/notmuch-new.c\r
  68 > @@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,\r
  69 >       /* success */\r
  70 >       case NOTMUCH_STATUS_SUCCESS:\r
  71 >           state->added_messages++;\r
  72 > +         notmuch_message_freeze (message);\r
  73 >           for (tag=3Dstate->new_tags; *tag !=3D NULL; tag++)\r
  74 >               notmuch_message_add_tag (message, *tag);\r
  75 >           if (state->synchronize_flags =3D=3D TRUE) {\r
  76 > @@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,\r
  77 >                   notmuch_message_maildir_flags_to_tags (message);\r
  78 >               }\r
  79 >           }\r
  80 > +         notmuch_message_thaw (message);\r
  81 >           break;\r
  82 >       /* Non-fatal issues (go on to next file) */\r
  83 >       case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:\r
  84 \r
  85 I do support the patch's idea (which was recently committed; and what\r
  86 follows in this message is not at all directed towards Michal, who wrote\r
  87 this patch) -- but what about return values checking?  This is one aspect\r
  88 of the notmuch C code (which I generally consider to be nice to read and\r
  89 of high quality, as I said before already), that I consider totally\r
  90 lacking -- there are literally hundreds of C functions calls where the\r
  91 return values are just discarded.  This is bad.  For example (simulating\r
  92 a full disk):\r
  93 \r
  94     $ notmuch dump > /dev/full\r
  95     $ echo $?\r
  96     0\r
  97 \r
  98 The command returned ``success'' -- but no data has been saved.  This\r
  99 could, in some extreme (but still likely) case mean: total tag data loss.\r
 100 (This is likely due to printf / close or fprintf / fclose (yes, really,\r
 101 (f)close! -- think of buffering) not getting their return values\r
 102 checked.)  Here is what is expected:\r
 103 \r
 104     $ echo foo > /dev/full\r
 105     bash: echo: write error: No space left on device\r
 106     $ echo $?\r
 107     1\r
 108 \r
 109 Then, in the few (!) lines quoted above, there are (exactly / only) calls\r
 110 to notmuch_message_freeze, notmuch_message_add_tag,\r
 111 notmuch_message_maildir_flags_to_tags, notmuch_message_thaw -- each of\r
 112 which can fail, will return this via the notmuch_status_t return value\r
 113 (whose possible values are documented, too), but none has their return\r
 114 value checked.\r
 115 \r
 116 Other languages have the concept of exceptions; C doesn't, so we're\r
 117 supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''\r
 118 statements after each and every non-void (etc.) C function call.  Or make\r
 119 the functions abort themselves (which is not a too good idea, as we\r
 120 surely agree).  Or use a different programming language -- now, at the\r
 121 present state, it wouldn't be too painful to switch, in my opinion.  (I\r
 122 won't suggest any specific language, though.)  If staying with C (which I\r
 123 don't object, either), then this needs a whole code audit, and a lot of\r
 124 discipline in the future.\r
 125 \r
 126 Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a\r
 127 serious programming issue.)\r
 128 \r
 129 \r
 130 Gr=C3=BC=C3=9Fe,\r
 131  Thomas\r
 132 \r
 133 --=-=-=\r
 134 Content-Type: application/pgp-signature\r
 135 \r
 136 -----BEGIN PGP SIGNATURE-----\r
 137 Version: GnuPG v1.4.10 (GNU/Linux)\r
 138 \r
 139 iEYEARECAAYFAk1AUWUACgkQFaWaPJ2HwAp1+ACfSs2mp3sQHz6K085vSOi8KvAN\r
 140 UUgAoJB2ljmpxdapkd4j65rS0qcaqD+Y\r
 141 =+RYY\r
 142 -----END PGP SIGNATURE-----\r
 143 --=-=-=--\r