Thanks for the thorough review! Quoth Dmitry Kurochkin on Dec 10 at 3:25 am: > Hi Austin. > > + /* The number of children of this part. */ > + int children; > > Consider renaming to children_count or similar to make it clear that it > is a counter and not the actual children. Good point. Renamed to nchildren, which is shorter but still conveys that it's a count. > + notmuch_bool_t decrypt_success; > > Perhaps s/decrypt_success/is_decrypted/ for consistency with > is_encrypted? I actually got rid of is_encrypted/is_signed in the new version (see below). > +mime_node_child (const mime_node_t *parent, int child); > + > #include "command-line-arguments.h" > #endif > > #include should go above declarations. That one's bremner's fault. I'll follow up with a patch to move this. > + mime_node_t *out = talloc_zero (parent, mime_node_t); > > Perhaps s/out/node/? Done. > + /* this violates RFC 3156 section 4, so we won't bother with it. */ > + fprintf (stderr, "Error: %d part(s) for a multipart/encrypted " > + "message (should be exactly 2)\n", > + out->children); > > Perhaps s/should be exactly/must be exactly/? That is what the > RFC says. Same for signature. Done. > + out->is_encrypted = TRUE; > + out->is_signed = TRUE; > > These are set only if we do decryption/verification. But their > names and comments imply that they should reflect properties of > the part and be set independently from whether we are actually > doing decryption or verification. Good point. I replaced these fields with new fields, decrypt_attempted and sig_attempted, which are used the way the old fields were, but actually reflect the desired semantics and the information that the callers need. I first tried keeping the is_* fields and making them reflect the purely structural properties of the message, but then I realized that that was both redundant with the type of the MIME part and wasn't what callers actually needed to know. As an added benefit, sig_attempted sidesteps the question of whether a multipart/{signed,encrypted} without any signatures is "signed" and makes it possible for callers to distinguish between unsigned parts, signature verification failures, and encrypted parts with no signers. > + /* For some reason the GMimeSignatureValidity returned > + * here is not a const (inconsistent with that > + * returned by > + * g_mime_multipart_encrypted_get_signature_validity, > + * and therefore needs to be properly disposed of. > + * Hopefully the API will become more consistent. */ > > Ouch. In gmime 2.6 this API has changed, but looks like the > issue is still there. Is there a bug for it? If yes, we should > add a reference to the comment. Otherwise, we should file the > bug and then add a reference to the comment :) It looks like they're both non-const in 2.6 (which makes more sense to me). I updated the comment to mention this. > Both decryption and verification use cryptoctx. But decryption > is done iff decrypt is true (without checking if cryptoctx is > set) and verification is done iff cryptoctx is set (without any > special flag). Why is it asymmetric? Do we need to check if > cryptoctx is set for decryption? Oops, it wasn't supposed to be asymmetric. Decryption now requires cryptoctx to be set (it probably would have crashed before without it). > In mime_node_child(): > > + GMimeMultipart *multipart = GMIME_MULTIPART (parent->part); > + if (child == 1 && parent->decrypted_child) > + return _mime_node_create (parent, parent->decrypted_child); > > Multipart is not needed here, please move it's declaration below > the condition. > > + GMimeObject *child = g_mime_message_get_mime_part (message); > > The child variable eclipses the (int child) parameter. Consider > using a different name for the variable (or parameter). The above two were superseded by the next change. > + return _mime_node_create (parent, parent->decrypted_child); > + return _mime_node_create (parent, g_mime_multipart_get_part (multipart, child)); > ... > + return _mime_node_create (parent, child); > > All returns are similar. Consider declaring a local variable for > storing the part and using a single return, i.e.: > > GMimeObject *part; > if (...) > part = ...; > else ... > part = ...; > > return _mime_node_create (parent, part); Good idea. This made things compact enough to simplify the rest of this function. > Regards, > Dmitry > -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams.