[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.
Austin Clements
amdragon at MIT.EDU
Fri Dec 23 19:45:00 PST 2011
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 at 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.
More information about the notmuch
mailing list