15/1e5be0b72348fea0ecc6c2e185a6efc537988c

   1 Return-Path: <amdragon@mit.edu>\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 CB6F8429E25\r
   6         for <notmuch@notmuchmail.org>; Fri, 23 Dec 2011 19:44:04 -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.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\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 ps2GtRYRzG6T for <notmuch@notmuchmail.org>;\r
  16         Fri, 23 Dec 2011 19:44:03 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-7.mit.edu (DMZ-MAILSEC-SCANNER-7.MIT.EDU\r
  18         [18.7.68.36])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 47FBA431FB6\r
  20         for <notmuch@notmuchmail.org>; Fri, 23 Dec 2011 19:44:03 -0800 (PST)\r
  21 X-AuditID: 12074424-b7fae6d000000906-20-4ef54a81b326\r
  22 Received: from mailhub-auth-3.mit.edu ( [18.9.21.43])\r
  23         by dmz-mailsec-scanner-7.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id EE.7C.02310.18A45FE4; Fri, 23 Dec 2011 22:44:01 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id pBO3i0hV024065; \r
  27         Fri, 23 Dec 2011 22:44:01 -0500\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id pBO3hxtT017625\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Fri, 23 Dec 2011 22:44:00 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1ReIXc-0002Eb-EM; Fri, 23 Dec 2011 22:45:00 -0500\r
  37 Date: Fri, 23 Dec 2011 22:45:00 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  40 Subject: Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME\r
  41         traversal.\r
  42 Message-ID: <20111224034500.GA1927@mit.edu>\r
  43 References: <1323027100-10307-1-git-send-email-amdragon@mit.edu>\r
  44         <1323460468-4030-1-git-send-email-amdragon@mit.edu>\r
  45         <1323460468-4030-3-git-send-email-amdragon@mit.edu>\r
  46         <87k46572f7.fsf@gmail.com>\r
  47 MIME-Version: 1.0\r
  48 Content-Type: text/plain; charset=us-ascii\r
  49 Content-Disposition: inline\r
  50 In-Reply-To: <87k46572f7.fsf@gmail.com>\r
  51 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  52 X-Brightmail-Tracker:\r
  53  H4sIAAAAAAAAA+NgFmpkleLIzCtJLcpLzFFi42IR4hTV1m30+upncPywtsXVrf3sFtdvzmR2\r
  54         YPLYOesuu8ezVbeYA5iiuGxSUnMyy1KL9O0SuDJWnVQo2KJesfjPQtYGxguyXYycHBICJhK7\r
  55         mhuYIWwxiQv31rN1MXJxCAnsY5R4+OQPK4SzgVHi7KeJTCBVQgInmSSOrymBSCxhlJjft5m9\r
  56         i5GDg0VAVWLdI3WQGjYBDYlt+5czgtgiAoYSty6+AtvALCAt8e13MxNIubBAhMTTHXUgYV4B\r
  57         bYkPi5dB7TrJKHH3TCsbREJQ4uTMJywQvVoSN/69BOsFmbP8HwdImFNAXWLm/nlgq0QFVCSm\r
  58         nNzGNoFRaBaS7llIumchdC9gZF7FKJuSW6Wbm5iZU5yarFucnJiXl1qka66Xm1mil5pSuokR\r
  59         FNLsLio7GJsPKR1iFOBgVOLhbVz6xU+INbGsuDL3EKMkB5OSKO9Vt69+QnxJ+SmVGYnFGfFF\r
  60         pTmpxYcYJTiYlUR4NZOAynlTEiurUovyYVLSHCxK4rwaWu/8hATSE0tSs1NTC1KLYLIyHBxK\r
  61         ErxPPIGGChalpqdWpGXmlCCkmTg4QYbzAA2fAlLDW1yQmFucmQ6RP8WoKCXOuw0kIQCSyCjN\r
  62         g+uFpZxXjOJArwjzvgSp4gGmK7juV0CDmYAGxxiBXF1ckoiQkmpgVAy24vtyR+TN5clRafLf\r
  63         Mq7YxzHaqitfP3q4vj1hz/MF27lCi/Zd8d0gfPuLSZqO4ILu9cYyJw/ypwZqhGrUpBwzWMAc\r
  64         8SSG8ZWs6N+PZ7bd6eCdyLfS7bjTnN4N/7/GmS8SD7y174+32P6qfT9nzrJeezVtvtmGOIZZ\r
  65         fq5f2/bzFs5QEny0UomlOCPRUIu5qDgRANOBrYwUAwAA\r
  66 Cc: notmuch@notmuchmail.org\r
  67 X-BeenThere: notmuch@notmuchmail.org\r
  68 X-Mailman-Version: 2.1.13\r
  69 Precedence: list\r
  70 List-Id: "Use and development of the notmuch mail system."\r
  71         <notmuch.notmuchmail.org>\r
  72 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  74 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  75 List-Post: <mailto:notmuch@notmuchmail.org>\r
  76 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  77 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  78         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  79 X-List-Received-Date: Sat, 24 Dec 2011 03:44:04 -0000\r
  80 \r
  81 Thanks for the thorough review!\r
  82 \r
  83 Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:\r
  84 > Hi Austin.\r
  85 > \r
  86 > +    /* The number of children of this part. */\r
  87 > +    int children;\r
  88 > \r
  89 > Consider renaming to children_count or similar to make it clear that it\r
  90 > is a counter and not the actual children.\r
  91 \r
  92 Good point.  Renamed to nchildren, which is shorter but still conveys\r
  93 that it's a count.\r
  94 \r
  95 > +    notmuch_bool_t decrypt_success;\r
  96 > \r
  97 > Perhaps s/decrypt_success/is_decrypted/ for consistency with\r
  98 > is_encrypted?\r
  99 \r
 100 I actually got rid of is_encrypted/is_signed in the new version (see\r
 101 below).\r
 102 \r
 103 > +mime_node_child (const mime_node_t *parent, int child);\r
 104 > +\r
 105 >  #include "command-line-arguments.h"\r
 106 >  #endif\r
 107 > \r
 108 > #include should go above declarations.\r
 109 \r
 110 That one's bremner's fault.  I'll follow up with a patch to move this.\r
 111 \r
 112 > +    mime_node_t *out = talloc_zero (parent, mime_node_t);\r
 113 > \r
 114 > Perhaps s/out/node/?\r
 115 \r
 116 Done.\r
 117 \r
 118 > +           /* this violates RFC 3156 section 4, so we won't bother with it. */\r
 119 > +           fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "\r
 120 > +                    "message (should be exactly 2)\n",\r
 121 > +                    out->children);\r
 122 > \r
 123 > Perhaps s/should be exactly/must be exactly/?  That is what the\r
 124 > RFC says.  Same for signature.\r
 125 \r
 126 Done.\r
 127 \r
 128 > +           out->is_encrypted = TRUE;\r
 129 > +           out->is_signed = TRUE;\r
 130 > \r
 131 > These are set only if we do decryption/verification.  But their\r
 132 > names and comments imply that they should reflect properties of\r
 133 > the part and be set independently from whether we are actually\r
 134 > doing decryption or verification.\r
 135 \r
 136 Good point.  I replaced these fields with new fields,\r
 137 decrypt_attempted and sig_attempted, which are used the way the old\r
 138 fields were, but actually reflect the desired semantics and the\r
 139 information that the callers need.  I first tried keeping the is_*\r
 140 fields and making them reflect the purely structural properties of the\r
 141 message, but then I realized that that was both redundant with the\r
 142 type of the MIME part and wasn't what callers actually needed to know.\r
 143 As an added benefit, sig_attempted sidesteps the question of whether a\r
 144 multipart/{signed,encrypted} without any signatures is "signed" and\r
 145 makes it possible for callers to distinguish between unsigned parts,\r
 146 signature verification failures, and encrypted parts with no signers.\r
 147 \r
 148 > +           /* For some reason the GMimeSignatureValidity returned\r
 149 > +            * here is not a const (inconsistent with that\r
 150 > +            * returned by\r
 151 > +            * g_mime_multipart_encrypted_get_signature_validity,\r
 152 > +            * and therefore needs to be properly disposed of.\r
 153 > +            * Hopefully the API will become more consistent. */\r
 154 > \r
 155 > Ouch.  In gmime 2.6 this API has changed, but looks like the\r
 156 > issue is still there.  Is there a bug for it?  If yes, we should\r
 157 > add a reference to the comment.  Otherwise, we should file the\r
 158 > bug and then add a reference to the comment :)\r
 159 \r
 160 It looks like they're both non-const in 2.6 (which makes more sense to\r
 161 me).  I updated the comment to mention this.\r
 162 \r
 163 > Both decryption and verification use cryptoctx.  But decryption\r
 164 > is done iff decrypt is true (without checking if cryptoctx is\r
 165 > set) and verification is done iff cryptoctx is set (without any\r
 166 > special flag).  Why is it asymmetric?  Do we need to check if\r
 167 > cryptoctx is set for decryption?\r
 168 \r
 169 Oops, it wasn't supposed to be asymmetric.  Decryption now requires\r
 170 cryptoctx to be set (it probably would have crashed before without\r
 171 it).\r
 172 \r
 173 > In mime_node_child():\r
 174 > \r
 175 > +       GMimeMultipart *multipart = GMIME_MULTIPART (parent->part);\r
 176 > +       if (child == 1 && parent->decrypted_child)\r
 177 > +           return _mime_node_create (parent, parent->decrypted_child);\r
 178 > \r
 179 > Multipart is not needed here, please move it's declaration below\r
 180 > the condition.\r
 181 >\r
 182 > +       GMimeObject *child = g_mime_message_get_mime_part (message);\r
 183 > \r
 184 > The child variable eclipses the (int child) parameter.  Consider\r
 185 > using a different name for the variable (or parameter).\r
 186 \r
 187 The above two were superseded by the next change.\r
 188 \r
 189 > +           return _mime_node_create (parent, parent->decrypted_child);\r
 190 > +       return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child));\r
 191 > ...\r
 192 > +       return _mime_node_create (parent, child);\r
 193 > \r
 194 > All returns are similar.  Consider declaring a local variable for\r
 195 > storing the part and using a single return, i.e.:\r
 196 > \r
 197 >   GMimeObject *part;\r
 198 >   if (...)\r
 199 >     part = ...;\r
 200 >   else ...\r
 201 >     part = ...;\r
 202 > \r
 203 >   return _mime_node_create (parent, part);\r
 204 \r
 205 Good idea.  This made things compact enough to simplify the rest of\r
 206 this function.\r
 207 \r
 208 > Regards,\r
 209 >   Dmitry\r
 210 > \r
 211 \r
 212 -- \r
 213 Austin Clements                                      MIT/'06/PhD/CSAIL\r
 214 amdragon@mit.edu                           http://web.mit.edu/amdragon\r
 215        Somewhere in the dream we call reality you will find me,\r
 216               searching for the reality we call dreams.\r