[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Daniel Kahn Gillmor on Dec 27 at  9:27 am:
> On 12/23/2011 10:45 PM, Austin Clements wrote:
> > Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
> >> +   /* 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.
> 
> Here's the bug report where this was discussed with upstream, fwiw:
> 
>   https://bugzilla.gnome.org/show_bug.cgi?id=640911

Oh, are we not supposed to be disposing of the GMimeSignatureValidity
returned by g_mime_multipart_encrypted_get_signature_validity
ourselves?  Comment 1 on that bug report suggests that we shouldn't.
The old show code did, so I ported that behavior over to the new code.

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Daniel Kahn Gillmor]

On 12/23/2011 10:45 PM, Austin Clements wrote:
> Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
>> +   /* 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.

Here's the bug report where this was discussed with upstream, fwiw:

  https://bugzilla.gnome.org/show_bug.cgi?id=640911

--dkg

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1030 bytes
Desc: OpenPGP digital signature
URL: 

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Daniel Kahn Gillmor]

On 12/23/2011 10:45 PM, Austin Clements wrote:
 Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
 +   /* 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.

Here's the bug report where this was discussed with upstream, fwiw:

  https://bugzilla.gnome.org/show_bug.cgi?id=640911

--dkg



signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Daniel Kahn Gillmor on Dec 27 at  9:27 am:
 On 12/23/2011 10:45 PM, Austin Clements wrote:
  Quoth Dmitry Kurochkin on Dec 10 at  3:25 am:
  +   /* 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.
 
 Here's the bug report where this was discussed with upstream, fwiw:
 
   https://bugzilla.gnome.org/show_bug.cgi?id=640911

Oh, are we not supposed to be disposing of the GMimeSignatureValidity
returned by g_mime_multipart_encrypted_get_signature_validity
ourselves?  Comment 1 on that bug report suggests that we shouldn't.
The old show code did, so I ported that behavior over to the new code.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Jameson Graef Rollins on Dec 10 at  1:17 pm:
> On Sat, 10 Dec 2011 03:25:48 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > +   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.
> 
> I don't think this directly affects anything at the moment, but this is
> a good point.  I can imagine that it might be good to maintain that
> distinction down the line so it's probably worth being consistent here.

See my response to Dmitry's original email.  The structural properties
can be derived directly from the type of the part field, so I got rid
of these fields.

> > 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?
> 
> We probably should.  We can probably fix this in followup patches, since
>  Austin is just working off of what dkg and I put in there originally.

Actually, this one was my fault from when I tweaked the control flow
to use the MIME node context.

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

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.

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

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
amdra...@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.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Quoth Jameson Graef Rollins on Dec 10 at  1:17 pm:
 On Sat, 10 Dec 2011 03:25:48 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@gmail.com wrote:
  +   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.
 
 I don't think this directly affects anything at the moment, but this is
 a good point.  I can imagine that it might be good to maintain that
 distinction down the line so it's probably worth being consistent here.

See my response to Dmitry's original email.  The structural properties
can be derived directly from the type of the part field, so I got rid
of these fields.

  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?
 
 We probably should.  We can probably fix this in followup patches, since
  Austin is just working off of what dkg and I put in there originally.

Actually, this one was my fault from when I tweaked the control flow
to use the MIME node context.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jameson Graef Rollins]

On Fri,  9 Dec 2011 14:54:26 -0500, Austin Clements  wrote:
> +/* Handle PGP/MIME parts */
> +if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
> + if (out->children != 2) {
> + /* 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);
> + } else {
> + out->is_encrypted = TRUE;

As per Dmitry's previous point, maybe it's better to do something like:

if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
out->is_encrypted = TRUE;
if (out->ctx->decrypt) {
if (out->children != 2) {
...

And similarly for is_signed.

> + GMimeMultipartEncrypted *encrypteddata =
> + GMIME_MULTIPART_ENCRYPTED (part);
> + out->decrypted_child = g_mime_multipart_encrypted_decrypt
> + (encrypteddata, out->ctx->cryptoctx, );
> + if (out->decrypted_child) {
> + out->decrypt_success = TRUE;
> + out->is_signed = TRUE;
> + out->sig_validity = 
> g_mime_multipart_encrypted_get_signature_validity (encrypteddata);

Encrypted messages are not necessarily signed, so we need to be careful
about setting is_signed = TRUE based just on decryption status.  The
problem is that gmime's handling of this stuff (at least last I looked
in 2.4) is not so good.
g_mime_multipart_encrypted_get_signature_validity () should return
GMIME_SIGNATURE_STATUS_UNKNOWN if there's no signature, so I think
is_signed should be set TRUE only if sig_validity is not UNKNOWN.

I've really been meaning to overhaul this stuff for gmime 2.6.
Hopefully I'll start looking at that after these patches go through.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jameson Graef Rollins]

On Sat, 10 Dec 2011 03:25:48 +0400, Dmitry Kurochkin  wrote:
> +notmuch_bool_t decrypt_success;
> 
> Perhaps s/decrypt_success/is_decrypted/ for consistency with
> is_encrypted?

This difference doesn't seem so bad to me, since the is_ variables point
to states of the original message, where as decrypt_success reflects the
processing of the message only.

> +   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.

I don't think this directly affects anything at the moment, but this is
a good point.  I can imagine that it might be good to maintain that
distinction down the line so it's probably worth being consistent here.

> +   /* 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 :)

Don't blame any of this stuff on Austin.  This is left over from the
original crypto processing patches.  I know dkg was in touch with the
gmime maintainers on this issue, but I'm not sure if a bug was filed.

> 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?

We probably should.  We can probably fix this in followup patches, since
 Austin is just working off of what dkg and I put in there originally.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

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.

+notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include "command-line-arguments.h"
 #endif

#include should go above declarations.

+mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+   /* 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.

+   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.

+   /* 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 :)

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?

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).

+   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);

Regards,
  Dmitry

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index 28e371a..aeb068d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -313,6 +313,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c

 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright ? 2009 Carl Worth
+ * Copyright ? 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth 
+ *  Keith Packard 
+ *  Austin Clements 
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res->mime_message)
+   g_object_unref (res->mime_message);
+
+if (res->parser)
+   g_object_unref (res->parser);
+
+if (res->stream)
+   g_object_unref (res->stream);
+
+if (res->file)
+   fclose (res->file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx->file = fopen (filename, "r");
+if (! mctx->file) {
+   fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx->stream = g_mime_stream_file_new (mctx->file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+mctx->cryptoctx = cryptoctx;
+mctx->decrypt = decrypt;
+
+/* Create the root node */
+root->part = GMIME_OBJECT (mctx->mime_message);
+root->envelope_file = message;
+root->children = 1;
+root->ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+mime_node_t *out = 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index 28e371a..aeb068d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -313,6 +313,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth cwo...@cworth.org
+ *  Keith Packard kei...@keithp.com
+ *  Austin Clements acleme...@csail.mit.edu
+ */
+
+#include notmuch-client.h
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res-mime_message)
+   g_object_unref (res-mime_message);
+
+if (res-parser)
+   g_object_unref (res-parser);
+
+if (res-stream)
+   g_object_unref (res-stream);
+
+if (res-file)
+   fclose (res-file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx-file = fopen (filename, r);
+if (! mctx-file) {
+   fprintf (stderr, Error opening %s: %s\n, filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx-stream = g_mime_stream_file_new (mctx-file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx-stream), FALSE);
+
+mctx-parser = g_mime_parser_new_with_stream (mctx-stream);
+
+mctx-mime_message = g_mime_parser_construct_message (mctx-parser);
+
+mctx-cryptoctx = cryptoctx;
+mctx-decrypt = decrypt;
+
+/* Create the root node */
+root-part = GMIME_OBJECT (mctx-mime_message);
+root-envelope_file = message;
+root-children = 1;
+root-ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Dmitry Kurochkin]

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.

+notmuch_bool_t decrypt_success;

Perhaps s/decrypt_success/is_decrypted/ for consistency with
is_encrypted?

+mime_node_child (const mime_node_t *parent, int child);
+
 #include command-line-arguments.h
 #endif

#include should go above declarations.

+mime_node_t *out = talloc_zero (parent, mime_node_t);

Perhaps s/out/node/?

+   /* 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.

+   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.

+   /* 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 :)

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?

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).

+   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);

Regards,
  Dmitry
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c

 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright ? 2009 Carl Worth
+ * Copyright ? 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth 
+ *  Keith Packard 
+ *  Austin Clements 
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res->mime_message)
+   g_object_unref (res->mime_message);
+
+if (res->parser)
+   g_object_unref (res->parser);
+
+if (res->stream)
+   g_object_unref (res->stream);
+
+if (res->file)
+   fclose (res->file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx->file = fopen (filename, "r");
+if (! mctx->file) {
+   fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx->stream = g_mime_stream_file_new (mctx->file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+mctx->cryptoctx = cryptoctx;
+mctx->decrypt = decrypt;
+
+/* Create the root node */
+root->part = GMIME_OBJECT (mctx->mime_message);
+root->envelope_file = message;
+root->children = 1;
+root->ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+mime_node_t *out = 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Thanks for the review.  A new version is on its way...

Quoth Jani Nikula on Nov 29 at  9:11 pm:
> > +mctx->stream = g_mime_stream_file_new (mctx->file);
> 
> AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
> closes it later. Calling fclose() on it in _mime_node_context_free()
> would be undefined behaviour. But please don't just take my word for it.

The next line tells GMime not to close the stream on its own.

Though, while I believe the code is correct, I'm not sure why we can't
just let GMime close the stream and remove our fclose.  The original
code for this was introduced in 357aba3e (along with most of the code
in show-message.c) with the cryptic comment that otherwise GMime would
close stdout.  Since I'd rather not regress some bug I don't
understand, I'll leave the code the way it is.

> > +g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

Thanks for the review.  A new version is on its way...

Quoth Jani Nikula on Nov 29 at  9:11 pm:
  +mctx-stream = g_mime_stream_file_new (mctx-file);
 
 AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
 closes it later. Calling fclose() on it in _mime_node_context_free()
 would be undefined behaviour. But please don't just take my word for it.

The next line tells GMime not to close the stream on its own.

Though, while I believe the code is correct, I'm not sure why we can't
just let GMime close the stream and remove our fclose.  The original
code for this was introduced in 357aba3e (along with most of the code
in show-message.c) with the cryptic comment that otherwise GMime would
close stdout.  Since I'd rather not regress some bug I don't
understand, I'll leave the code the way it is.

  +g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx-stream), FALSE);
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..a8e4a59
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth cwo...@cworth.org
+ *  Keith Packard kei...@keithp.com
+ *  Austin Clements acleme...@csail.mit.edu
+ */
+
+#include notmuch-client.h
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res-mime_message)
+   g_object_unref (res-mime_message);
+
+if (res-parser)
+   g_object_unref (res-parser);
+
+if (res-stream)
+   g_object_unref (res-stream);
+
+if (res-file)
+   fclose (res-file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx-file = fopen (filename, r);
+if (! mctx-file) {
+   fprintf (stderr, Error opening %s: %s\n, filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx-stream = g_mime_stream_file_new (mctx-file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx-stream), FALSE);
+
+mctx-parser = g_mime_parser_new_with_stream (mctx-stream);
+
+mctx-mime_message = g_mime_parser_construct_message (mctx-parser);
+
+mctx-cryptoctx = cryptoctx;
+mctx-decrypt = decrypt;
+
+/* Create the root node */
+root-part = GMIME_OBJECT (mctx-mime_message);
+root-envelope_file = message;
+root-children = 1;
+root-ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jani Nikula]

Hi, generally looks good to me, but please find a few comments below.

BR,
Jani.

On Sun, 27 Nov 2011 21:21:09 -0500, Austin Clements  wrote:
> This wraps all of the complex MIME part handling in a single, simple
> function that gets part N from *any* MIME object, so traversing a MIME
> part tree becomes a two-line for loop.  Furthermore, the MIME node
> structure provides easy access to envelopes for message parts as well
> as cryptographic information.
> 
> This code is directly derived from the current show_message_body code
> (much of it is identical), but the control relation is inverted:
> instead of show_message_body controlling the traversal of the MIME
> structure and invoking callbacks, the caller controls the traversal of
> the MIME structure.
> ---
>  Makefile.local   |1 +
>  mime-node.c  |  234 
> ++
>  notmuch-client.h |   80 ++
>  3 files changed, 315 insertions(+), 0 deletions(-)
>  create mode 100644 mime-node.c
> 
> diff --git a/Makefile.local b/Makefile.local
> index c94402b..c46ed26 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -312,6 +312,7 @@ notmuch_client_srcs = \
>   notmuch-time.c  \
>   query-string.c  \
>   show-message.c  \
> + mime-node.c \
>   json.c
>  
>  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
> diff --git a/mime-node.c b/mime-node.c
> new file mode 100644
> index 000..942738b
> --- /dev/null
> +++ b/mime-node.c
> @@ -0,0 +1,234 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright ? 2009 Carl Worth
> + * Copyright ? 2009 Keith Packard
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Authors: Carl Worth 
> + *  Keith Packard 
> + *  Austin Clements 
> + */
> +
> +#include "notmuch-client.h"
> +
> +/* Context that gets inherited from the root node. */
> +typedef struct mime_node_context {
> +/* Per-message resources.  These are allocated internally and must
> + * be destroyed. */
> +FILE *file;
> +GMimeStream *stream;
> +GMimeParser *parser;
> +GMimeMessage *mime_message;
> +

Leftover indentation spaces above.

> +/* Context provided by the caller. */
> +GMimeCipherContext *cryptoctx;
> +notmuch_bool_t decrypt;
> +} mime_node_context_t;
> +
> +static int
> +_mime_node_context_free (mime_node_context_t *res)
> +{
> +if (res->mime_message)
> + g_object_unref (res->mime_message);
> +
> +if (res->parser)
> + g_object_unref (res->parser);
> +
> +if (res->stream)
> + g_object_unref (res->stream);
> +
> +if (res->file)
> + fclose (res->file);
> +
> +return 0;
> +}
> +
> +notmuch_status_t
> +mime_node_open (const void *ctx, notmuch_message_t *message,
> + GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,

The style here seems to be * next to the variable name, not type.

> + mime_node_t **root_out)
> +{
> +const char *filename = notmuch_message_get_filename (message);
> +mime_node_context_t *mctx;
> +mime_node_t *root = NULL;

No need to initialize as it's initialized right away below.

> +notmuch_status_t status;
> +
> +root = talloc_zero (ctx, mime_node_t);
> +if (root == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> + goto DONE;
> +}
> +
> +/* Create the tree-wide context */
> +mctx = talloc_zero (root, mime_node_context_t);
> +if (mctx == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + status = NOTMUCH_STATUS_OUT_OF_MEMORY;
> + goto DONE;
> +}
> +talloc_set_destructor (mctx, _mime_node_context_free);
> +
> +mctx->file = fopen (filename, "r");
> +if (! mctx->file) {
> + fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
> + status = NOTMUCH_STATUS_FILE_ERROR;
> + goto DONE;
> +}
> +
> +mctx->stream = g_mime_stream_file_new (mctx->file);

AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
closes it later. Calling fclose() on it in _mime_node_context_free()
would be undefined behaviour. But please don't just take my word for it.

> +g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
> +
> +

Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Jani Nikula]

Hi, generally looks good to me, but please find a few comments below.

BR,
Jani.

On Sun, 27 Nov 2011 21:21:09 -0500, Austin Clements amdra...@mit.edu wrote:
 This wraps all of the complex MIME part handling in a single, simple
 function that gets part N from *any* MIME object, so traversing a MIME
 part tree becomes a two-line for loop.  Furthermore, the MIME node
 structure provides easy access to envelopes for message parts as well
 as cryptographic information.
 
 This code is directly derived from the current show_message_body code
 (much of it is identical), but the control relation is inverted:
 instead of show_message_body controlling the traversal of the MIME
 structure and invoking callbacks, the caller controls the traversal of
 the MIME structure.
 ---
  Makefile.local   |1 +
  mime-node.c  |  234 
 ++
  notmuch-client.h |   80 ++
  3 files changed, 315 insertions(+), 0 deletions(-)
  create mode 100644 mime-node.c
 
 diff --git a/Makefile.local b/Makefile.local
 index c94402b..c46ed26 100644
 --- a/Makefile.local
 +++ b/Makefile.local
 @@ -312,6 +312,7 @@ notmuch_client_srcs = \
   notmuch-time.c  \
   query-string.c  \
   show-message.c  \
 + mime-node.c \
   json.c
  
  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
 diff --git a/mime-node.c b/mime-node.c
 new file mode 100644
 index 000..942738b
 --- /dev/null
 +++ b/mime-node.c
 @@ -0,0 +1,234 @@
 +/* notmuch - Not much of an email program, (just index and search)
 + *
 + * Copyright © 2009 Carl Worth
 + * Copyright © 2009 Keith Packard
 + *
 + * This program is free software: you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation, either version 3 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program.  If not, see http://www.gnu.org/licenses/ .
 + *
 + * Authors: Carl Worth cwo...@cworth.org
 + *  Keith Packard kei...@keithp.com
 + *  Austin Clements acleme...@csail.mit.edu
 + */
 +
 +#include notmuch-client.h
 +
 +/* Context that gets inherited from the root node. */
 +typedef struct mime_node_context {
 +/* Per-message resources.  These are allocated internally and must
 + * be destroyed. */
 +FILE *file;
 +GMimeStream *stream;
 +GMimeParser *parser;
 +GMimeMessage *mime_message;
 +

Leftover indentation spaces above.

 +/* Context provided by the caller. */
 +GMimeCipherContext *cryptoctx;
 +notmuch_bool_t decrypt;
 +} mime_node_context_t;
 +
 +static int
 +_mime_node_context_free (mime_node_context_t *res)
 +{
 +if (res-mime_message)
 + g_object_unref (res-mime_message);
 +
 +if (res-parser)
 + g_object_unref (res-parser);
 +
 +if (res-stream)
 + g_object_unref (res-stream);
 +
 +if (res-file)
 + fclose (res-file);
 +
 +return 0;
 +}
 +
 +notmuch_status_t
 +mime_node_open (const void *ctx, notmuch_message_t *message,
 + GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,

The style here seems to be * next to the variable name, not type.

 + mime_node_t **root_out)
 +{
 +const char *filename = notmuch_message_get_filename (message);
 +mime_node_context_t *mctx;
 +mime_node_t *root = NULL;

No need to initialize as it's initialized right away below.

 +notmuch_status_t status;
 +
 +root = talloc_zero (ctx, mime_node_t);
 +if (root == NULL) {
 + fprintf (stderr, Out of memory.\n);
 + status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 + goto DONE;
 +}
 +
 +/* Create the tree-wide context */
 +mctx = talloc_zero (root, mime_node_context_t);
 +if (mctx == NULL) {
 + fprintf (stderr, Out of memory.\n);
 + status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 + goto DONE;
 +}
 +talloc_set_destructor (mctx, _mime_node_context_free);
 +
 +mctx-file = fopen (filename, r);
 +if (! mctx-file) {
 + fprintf (stderr, Error opening %s: %s\n, filename, strerror (errno));
 + status = NOTMUCH_STATUS_FILE_ERROR;
 + goto DONE;
 +}
 +
 +mctx-stream = g_mime_stream_file_new (mctx-file);

AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
closes it later. Calling fclose() on it in _mime_node_context_free()
would be undefined behaviour. But please don't just take my word for it.

 +g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx-stream), FALSE);
 +
 +mctx-parser = g_mime_parser_new_with_stream (mctx-stream);
 +
 +mctx-mime_message 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c

 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..942738b
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright ? 2009 Carl Worth
+ * Copyright ? 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth 
+ *  Keith Packard 
+ *  Austin Clements 
+ */
+
+#include "notmuch-client.h"
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res->mime_message)
+   g_object_unref (res->mime_message);
+
+if (res->parser)
+   g_object_unref (res->parser);
+
+if (res->stream)
+   g_object_unref (res->stream);
+
+if (res->file)
+   fclose (res->file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root = NULL;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, "Out of memory.\n");
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx->file = fopen (filename, "r");
+if (! mctx->file) {
+   fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx->stream = g_mime_stream_file_new (mctx->file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALSE);
+
+mctx->parser = g_mime_parser_new_with_stream (mctx->stream);
+
+mctx->mime_message = g_mime_parser_construct_message (mctx->parser);
+
+mctx->cryptoctx = cryptoctx;
+mctx->decrypt = decrypt;
+
+/* Create the root node */
+root->part = GMIME_OBJECT (mctx->mime_message);
+root->envelope_file = message;
+root->children = 1;
+root->ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)
+{
+mime_node_t *out 

[PATCH 2/4] Introduce a generic tree-like abstraction for MIME traversal.

[Austin Clements]

This wraps all of the complex MIME part handling in a single, simple
function that gets part N from *any* MIME object, so traversing a MIME
part tree becomes a two-line for loop.  Furthermore, the MIME node
structure provides easy access to envelopes for message parts as well
as cryptographic information.

This code is directly derived from the current show_message_body code
(much of it is identical), but the control relation is inverted:
instead of show_message_body controlling the traversal of the MIME
structure and invoking callbacks, the caller controls the traversal of
the MIME structure.
---
 Makefile.local   |1 +
 mime-node.c  |  234 ++
 notmuch-client.h |   80 ++
 3 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 mime-node.c

diff --git a/Makefile.local b/Makefile.local
index c94402b..c46ed26 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -312,6 +312,7 @@ notmuch_client_srcs =   \
notmuch-time.c  \
query-string.c  \
show-message.c  \
+   mime-node.c \
json.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/mime-node.c b/mime-node.c
new file mode 100644
index 000..942738b
--- /dev/null
+++ b/mime-node.c
@@ -0,0 +1,234 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2009 Carl Worth
+ * Copyright © 2009 Keith Packard
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Carl Worth cwo...@cworth.org
+ *  Keith Packard kei...@keithp.com
+ *  Austin Clements acleme...@csail.mit.edu
+ */
+
+#include notmuch-client.h
+
+/* Context that gets inherited from the root node. */
+typedef struct mime_node_context {
+/* Per-message resources.  These are allocated internally and must
+ * be destroyed. */
+FILE *file;
+GMimeStream *stream;
+GMimeParser *parser;
+GMimeMessage *mime_message;
+
+/* Context provided by the caller. */
+GMimeCipherContext *cryptoctx;
+notmuch_bool_t decrypt;
+} mime_node_context_t;
+
+static int
+_mime_node_context_free (mime_node_context_t *res)
+{
+if (res-mime_message)
+   g_object_unref (res-mime_message);
+
+if (res-parser)
+   g_object_unref (res-parser);
+
+if (res-stream)
+   g_object_unref (res-stream);
+
+if (res-file)
+   fclose (res-file);
+
+return 0;
+}
+
+notmuch_status_t
+mime_node_open (const void *ctx, notmuch_message_t *message,
+   GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
+   mime_node_t **root_out)
+{
+const char *filename = notmuch_message_get_filename (message);
+mime_node_context_t *mctx;
+mime_node_t *root = NULL;
+notmuch_status_t status;
+
+root = talloc_zero (ctx, mime_node_t);
+if (root == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+
+/* Create the tree-wide context */
+mctx = talloc_zero (root, mime_node_context_t);
+if (mctx == NULL) {
+   fprintf (stderr, Out of memory.\n);
+   status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+   goto DONE;
+}
+talloc_set_destructor (mctx, _mime_node_context_free);
+
+mctx-file = fopen (filename, r);
+if (! mctx-file) {
+   fprintf (stderr, Error opening %s: %s\n, filename, strerror (errno));
+   status = NOTMUCH_STATUS_FILE_ERROR;
+   goto DONE;
+}
+
+mctx-stream = g_mime_stream_file_new (mctx-file);
+g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx-stream), FALSE);
+
+mctx-parser = g_mime_parser_new_with_stream (mctx-stream);
+
+mctx-mime_message = g_mime_parser_construct_message (mctx-parser);
+
+mctx-cryptoctx = cryptoctx;
+mctx-decrypt = decrypt;
+
+/* Create the root node */
+root-part = GMIME_OBJECT (mctx-mime_message);
+root-envelope_file = message;
+root-children = 1;
+root-ctx = mctx;
+
+*root_out = root;
+return NOTMUCH_STATUS_SUCCESS;
+
+DONE:
+talloc_free (root);
+return status;
+}
+
+static int
+_signature_validity_free (GMimeSignatureValidity **proxy)
+{
+g_mime_signature_validity_free (*proxy);
+return 0;
+}
+
+static mime_node_t *
+_mime_node_create (const mime_node_t *parent, GMimeObject *part)