Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
Thanks for this review Austin. New version coming soon. On Tue, 17 Jan 2012 17:25:46 -0500, Austin Clements amdra...@mit.edu wrote: Quoth Thomas Jost on Jan 17 at 11:50 am: There are lots of API changes in gmime 2.6 crypto handling. By adding preprocessor directives, it is however possible to add gmime 2.6 compatibility while preserving compatibility with gmime 2.4 too. This is mostly based on id:8762i8hrb9@bookbinder.fernseed.info. This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test fails (signature verification with signer key unavailable) but this will be hard to fix since the new API does not report the reason why a signature verification fails (other than the human-readable error message). --- mime-node.c | 56 ++-- notmuch-client.h | 28 +++- notmuch-reply.c |7 notmuch-show.c | 95 ++ show-message.c |4 ++ 5 files changed, 185 insertions(+), 5 deletions(-) diff --git a/mime-node.c b/mime-node.c index d26bb44..e575e1c 100644 --- a/mime-node.c +++ b/mime-node.c @@ -33,7 +33,11 @@ typedef struct mime_node_context { GMimeMessage *mime_message; /* Context provided by the caller. */ +#ifdef GMIME_26 +GMimeCryptoContext *cryptoctx; +#else GMimeCipherContext *cryptoctx; +#endif notmuch_bool_t decrypt; } mime_node_context_t; @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res) notmuch_status_t mime_node_open (const void *ctx, notmuch_message_t *message, - GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt, - mime_node_t **root_out) +#ifdef GMIME_26 + GMimeCryptoContext *cryptoctx, +#else + GMimeCipherContext *cryptoctx, +#endif + notmuch_bool_t decrypt, mime_node_t **root_out) { const char *filename = notmuch_message_get_filename (message); mime_node_context_t *mctx; @@ -112,12 +120,21 @@ DONE: return status; } +#ifdef GMIME_26 +static int +_signature_list_free (GMimeSignatureList **proxy) +{ +g_object_unref (*proxy); +return 0; +} +#else static int _signature_validity_free (GMimeSignatureValidity **proxy) { g_mime_signature_validity_free (*proxy); return 0; } +#endif static mime_node_t * _mime_node_create (const mime_node_t *parent, GMimeObject *part) @@ -165,11 +182,22 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part) GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part); node-decrypt_attempted = TRUE; +#ifdef GMIME_26 + GMimeDecryptResult *decrypt_result = NULL; Reading through the GMime code, it looks like we do have to unref decrypt_result. I think this is easy, though. Right after you call g_mime_decrypt_result_get_signatures below, do: g_object_ref (node-sig_list); g_object_unref (decrypt_result); Added, thanks! + node-decrypted_child = g_mime_multipart_encrypted_decrypt + (encrypteddata, node-ctx-cryptoctx, decrypt_result, err); +#else node-decrypted_child = g_mime_multipart_encrypted_decrypt (encrypteddata, node-ctx-cryptoctx, err); +#endif if (node-decrypted_child) { - node-decrypt_success = node-verify_attempted = TRUE; + node-decrypt_success = node-verify_attempted =TRUE; Whitespace. (There should be no diff on the above line) Oops, my bad. +#ifdef GMIME_26 + /* This may be NULL if the part is not signed. */ + node-sig_list = g_mime_decrypt_result_get_signatures (decrypt_result); +#else node-sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata); +#endif } else { fprintf (stderr, Failed to decrypt part: %s\n, (err ? err-message : no error explanation given)); @@ -182,6 +210,16 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part) (must be exactly 2)\n, node-nchildren); } else { +#ifdef GMIME_26 + GMimeSignatureList *sig_list = g_mime_multipart_signed_verify + (GMIME_MULTIPART_SIGNED (part), node-ctx-cryptoctx, err); + node-verify_attempted = TRUE; + node-sig_list = sig_list; Just a nit. This could be node-sig_list = g_mime_multipart_signed_verify ( ... ) To me, the local variable just makes this code more verbose without adding anything. Up to you. Yep, the local variable is useless. Removed it. + + if (!sig_list) + fprintf (stderr, Failed to verify signed part: %s\n, +(err ? err-message : no error
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Wed, 18 Jan 2012 20:00:12 +0200, Tomi Ollila tomi.oll...@iki.fi wrote: On Wed, 18 Jan 2012 12:19:45 -0500, Tom Prince tom.pri...@ualberta.net wrote: How did you test against multiple versions? Using different machines? If there was a way for configure (or something to pick the version, I would setup the buildbot to test against both, so we don't regress either. I currently compile notmuch on Fedora 15 so that I have LIBRARY_PATH=/my/own/path/to/x86_64-linux/lib and CPATH=/my/own/path/to/x86_64-linux/include and gmime 2.4 development files are located in these directories. I'll be hiding gmime 2.4 stuff from these soon in order to build against 2.6 stuff. Tomi For testing gmime 2.4 and 2.6, I just uninstalled 2.6 and reinstalled 2.4 (kept the binary package on purpose -- not a problem since notmuch is the only package using gmime on my system). When hacking the gmime git in order to fix https://bugzilla.gnome.org/show_bug.cgi?id=668085, I set some environment variables before calling ./configure and building notmuch: LDFLAGS=-Wl,-rpath,/home/schnouki/dev/gmime/gmime/.libs ./configure --prefix=/usr --sysconfdir=/etc make ldd ./notmuch Regards, -- Thomas/Schnouki pgpybiDupPPyc.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Tue, 17 Jan 2012 17:25:46 -0500, Austin Clements amdra...@mit.edu wrote: Quoth Thomas Jost on Jan 17 at 11:50 am: +#ifdef GMIME_26 +/* sig_list may be created in both above cases, so we need to + * cleanly handle it here. */ +if (node-sig_list) { + GMimeSignatureList **proxy = + talloc (node, GMimeSignatureList *); This doesn't need to be split into two lines. + *proxy = node-sig_list; + talloc_set_destructor (proxy, _signature_list_free); +} +#else if (node-verify_attempted !node-sig_validity) fprintf (stderr, Failed to verify signed part: %s\n, (err ? err-message : no error explanation given)); +#endif I'd rather see the above as a separate #ifdef GMIME_26 and #ifndef GMIME_26, since they aren't logical alternates of each other. That reminds me that it should then be like GMIME_ATLEAST_26, so that this might be useful when GMIME 2.6 is available... Tomi ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Tue, 17 Jan 2012 11:50:53 +0100, Thomas Jost schno...@schnouki.net wrote: This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test fails (signature verification with signer key unavailable) but this will be hard to fix since the new API does not report the reason why a signature verification fails (other than the human-readable error message). How did you test against multiple versions? Using different machines? If there was a way for configure (or something to pick the version, I would setup the buildbot to test against both, so we don't regress either. Tom ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Wed, 18 Jan 2012 12:19:45 -0500, Tom Prince tom.pri...@ualberta.net wrote: How did you test against multiple versions? Using different machines? If there was a way for configure (or something to pick the version, I would setup the buildbot to test against both, so we don't regress either. I currently compile notmuch on Fedora 15 so that I have LIBRARY_PATH=/my/own/path/to/x86_64-linux/lib and CPATH=/my/own/path/to/x86_64-linux/include and gmime 2.4 development files are located in these directories. I'll be hiding gmime 2.4 stuff from these soon in order to build against 2.6 stuff. Tomi ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Tue, 17 Jan 2012 11:50:53 +0100, Thomas Jost schno...@schnouki.net wrote: There are lots of API changes in gmime 2.6 crypto handling. By adding preprocessor directives, it is however possible to add gmime 2.6 compatibility while preserving compatibility with gmime 2.4 too. This is mostly based on id:8762i8hrb9@bookbinder.fernseed.info. This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test fails (signature verification with signer key unavailable) but this will be hard to fix since the new API does not report the reason why a signature verification fails (other than the human-readable error message). --- LGTM. Some whitespace things but most of those were there already; I'd have one uncrustify round to be applied to the source and after that be more strict about those... actually gmime 2.4 has GMIME_CHECK_VERSION defined as g_mime_check_version (major, minor, micro) so the comment about it is not entirely accurate (it is unusable in our context, though) Tomi ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
On Tue, 17 Jan 2012 14:48:34 +0200, Tomi Ollila tomi.oll...@iki.fi wrote: On Tue, 17 Jan 2012 11:50:53 +0100, Thomas Jost schno...@schnouki.net wrote: There are lots of API changes in gmime 2.6 crypto handling. By adding preprocessor directives, it is however possible to add gmime 2.6 compatibility while preserving compatibility with gmime 2.4 too. This is mostly based on id:8762i8hrb9@bookbinder.fernseed.info. This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test fails (signature verification with signer key unavailable) but this will be hard to fix since the new API does not report the reason why a signature verification fails (other than the human-readable error message). --- LGTM. Some whitespace things but most of those were there already; I'd have one uncrustify round to be applied to the source and after that be more strict about those... Thanks! I'll fix the whitespace issues in these patches when the source in Git is uncrustified then. actually gmime 2.4 has GMIME_CHECK_VERSION defined as g_mime_check_version (major, minor, micro) so the comment about it is not entirely accurate (it is unusable in our context, though) Oops, yes. What I really meant is that GMIME_MAJOR_VERSION is not available as a preprocessor constant in 2.4, and GMIME_CHECK_VERSION is unusable in our context since it just calls a runtime function. By the way, how do you guys feel about setting GMIME_26 in notmuch-client.h? Is that good enough, or should it be done in configure so that gcc is called with -DGMIME_26? I filed a bug about gmime 2.6 incorrect handling of signatures with missing public keys: https://bugzilla.gnome.org/show_bug.cgi?id=668085. A patch is attached there, feel free to test and comment. (Arch Linux users: I made a little PKGBUILD that includes this patch if you want to build your own gmime 2.6.4: http://fichiers.schnouki.net/tmp/gmime-2.6.4-1.src.tar.gz) Best regards, -- Thomas/Schnouki pgp3y7I0N927m.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 2/2] Add pseudo-compatibility with gmime 2.6
Quoth Thomas Jost on Jan 17 at 11:50 am: There are lots of API changes in gmime 2.6 crypto handling. By adding preprocessor directives, it is however possible to add gmime 2.6 compatibility while preserving compatibility with gmime 2.4 too. This is mostly based on id:8762i8hrb9@bookbinder.fernseed.info. This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test fails (signature verification with signer key unavailable) but this will be hard to fix since the new API does not report the reason why a signature verification fails (other than the human-readable error message). --- mime-node.c | 56 ++-- notmuch-client.h | 28 +++- notmuch-reply.c |7 notmuch-show.c | 95 ++ show-message.c |4 ++ 5 files changed, 185 insertions(+), 5 deletions(-) diff --git a/mime-node.c b/mime-node.c index d26bb44..e575e1c 100644 --- a/mime-node.c +++ b/mime-node.c @@ -33,7 +33,11 @@ typedef struct mime_node_context { GMimeMessage *mime_message; /* Context provided by the caller. */ +#ifdef GMIME_26 +GMimeCryptoContext *cryptoctx; +#else GMimeCipherContext *cryptoctx; +#endif notmuch_bool_t decrypt; } mime_node_context_t; @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res) notmuch_status_t mime_node_open (const void *ctx, notmuch_message_t *message, - GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt, - mime_node_t **root_out) +#ifdef GMIME_26 + GMimeCryptoContext *cryptoctx, +#else + GMimeCipherContext *cryptoctx, +#endif + notmuch_bool_t decrypt, mime_node_t **root_out) { const char *filename = notmuch_message_get_filename (message); mime_node_context_t *mctx; @@ -112,12 +120,21 @@ DONE: return status; } +#ifdef GMIME_26 +static int +_signature_list_free (GMimeSignatureList **proxy) +{ +g_object_unref (*proxy); +return 0; +} +#else static int _signature_validity_free (GMimeSignatureValidity **proxy) { g_mime_signature_validity_free (*proxy); return 0; } +#endif static mime_node_t * _mime_node_create (const mime_node_t *parent, GMimeObject *part) @@ -165,11 +182,22 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part) GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part); node-decrypt_attempted = TRUE; +#ifdef GMIME_26 + GMimeDecryptResult *decrypt_result = NULL; Reading through the GMime code, it looks like we do have to unref decrypt_result. I think this is easy, though. Right after you call g_mime_decrypt_result_get_signatures below, do: g_object_ref (node-sig_list); g_object_unref (decrypt_result); + node-decrypted_child = g_mime_multipart_encrypted_decrypt + (encrypteddata, node-ctx-cryptoctx, decrypt_result, err); +#else node-decrypted_child = g_mime_multipart_encrypted_decrypt (encrypteddata, node-ctx-cryptoctx, err); +#endif if (node-decrypted_child) { - node-decrypt_success = node-verify_attempted = TRUE; + node-decrypt_success = node-verify_attempted =TRUE; Whitespace. (There should be no diff on the above line) +#ifdef GMIME_26 + /* This may be NULL if the part is not signed. */ + node-sig_list = g_mime_decrypt_result_get_signatures (decrypt_result); +#else node-sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata); +#endif } else { fprintf (stderr, Failed to decrypt part: %s\n, (err ? err-message : no error explanation given)); @@ -182,6 +210,16 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part) (must be exactly 2)\n, node-nchildren); } else { +#ifdef GMIME_26 + GMimeSignatureList *sig_list = g_mime_multipart_signed_verify + (GMIME_MULTIPART_SIGNED (part), node-ctx-cryptoctx, err); + node-verify_attempted = TRUE; + node-sig_list = sig_list; Just a nit. This could be node-sig_list = g_mime_multipart_signed_verify ( ... ) To me, the local variable just makes this code more verbose without adding anything. Up to you. + + if (!sig_list) + fprintf (stderr, Failed to verify signed part: %s\n, + (err ? err-message : no error explanation given)); +#else /* For some reason the GMimeSignatureValidity returned * here is not a const (inconsistent with that * returned by @@ -200,12 +238,24 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part) *proxy =