[PATCH 1/2] cli: crypto: abstract gpg context creation for clarity

[PATCH 1/2] cli: crypto: abstract gpg context creation for clarity

[Jani Nikula]

The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
read. Abstract gpg context creation into a function, with separate
implementations for GMime 2.4 and 2.6, to clarify the code.

There should be no functional changes.
---
 crypto.c |   60 +++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 19 deletions(-)

diff --git a/crypto.c b/crypto.c
index cb361e1..7d1dfe4 100644
--- a/crypto.c
+++ b/crypto.c
@@ -20,6 +20,42 @@
 
 #include "notmuch-client.h"
 
+#ifdef GMIME_ATLEAST_26
+static notmuch_crypto_context_t *
+create_gpg_context (void)
+{
+    notmuch_crypto_context_t *gpgctx;
+
+    /* TODO: GMimePasswordRequestFunc */
+    gpgctx = g_mime_gpg_context_new (NULL, "gpg");
+    if (! gpgctx)
+	return NULL;
+
+    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+
+    return gpgctx;
+}
+#else
+static notmuch_crypto_context_t *
+create_gpg_context (void)
+{
+    GMimeSession *session;
+    notmuch_crypto_context_t *gpgctx;
+
+    session = g_object_new (g_mime_session_get_type (), NULL);
+    gpgctx = g_mime_gpg_context_new (session, "gpg");
+    g_object_unref (session);
+
+    if (! gpgctx)
+	return NULL;
+
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+
+    return gpgctx;
+}
+#endif
+
 /* for the specified protocol return the context pointer (initializing
  * if needed) */
 notmuch_crypto_context_t *
@@ -33,28 +69,14 @@ notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol)
      * parameter names as defined in this document are
      * case-insensitive."  Thus, we use strcasecmp for the protocol.
      */
-    if ((strcasecmp (protocol, "application/pgp-signature") == 0)
-	|| (strcasecmp (protocol, "application/pgp-encrypted") == 0)) {
-	if (!crypto->gpgctx) {
-#ifdef GMIME_ATLEAST_26
-	    /* TODO: GMimePasswordRequestFunc */
-	    crypto->gpgctx = g_mime_gpg_context_new (NULL, "gpg");
-#else
-	    GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	    crypto->gpgctx = g_mime_gpg_context_new (session, "gpg");
-	    g_object_unref (session);
-#endif
-	    if (crypto->gpgctx) {
-#ifdef GMIME_ATLEAST_26
-		g_mime_gpg_context_set_use_agent ((GMimeGpgContext*) crypto->gpgctx, TRUE);
-#endif
-		g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto->gpgctx, FALSE);
-	    } else {
+    if (strcasecmp (protocol, "application/pgp-signature") == 0 ||
+	strcasecmp (protocol, "application/pgp-encrypted") == 0) {
+	if (! crypto->gpgctx) {
+	    crypto->gpgctx = create_gpg_context ();
+	    if (! crypto->gpgctx)
 		fprintf (stderr, "Failed to construct gpg context.\n");
-	    }
 	}
 	cryptoctx = crypto->gpgctx;
-
     } else {
 	fprintf (stderr, "Unknown or unsupported cryptographic protocol.\n");
     }
-- 
1.7.10.4

[PATCH 2/2] cli: mime node: abstract decryption and signature verification

[Jani Nikula]

The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
read. Abstract the decryption and signature verification into
functions, with separate implementations for GMime 2.4 and 2.6, to
clarify the code.

There should be no functional changes.
---
 mime-node.c |  210 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 134 insertions(+), 76 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 839737a..ba7f709 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -130,26 +130,155 @@ DONE:
 }
 
 #ifdef GMIME_ATLEAST_26
+
 static int
 _signature_list_free (GMimeSignatureList **proxy)
 {
     g_object_unref (*proxy);
     return 0;
 }
-#else
+
+static void
+set_signature_list_destructor (mime_node_t *node)
+{
+    GMimeSignatureList **proxy = talloc (node, GMimeSignatureList *);
+    if (proxy) {
+	*proxy = node->sig_list;
+	talloc_set_destructor (proxy, _signature_list_free);
+    }
+}
+
+static void
+node_verify (mime_node_t *node, GMimeObject *part,
+	     notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+
+    node->verify_attempted = TRUE;
+    node->sig_list = g_mime_multipart_signed_verify
+	(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
+
+    if (node->sig_list)
+	set_signature_list_destructor (node);
+    else
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+
+    if (err)
+	g_error_free (err);
+}
+
+static void
+node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
+			 notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+    GMimeDecryptResult *decrypt_result = NULL;
+    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
+
+    node->decrypt_attempted = TRUE;
+    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+	(encrypteddata, cryptoctx, &decrypt_result, &err);
+    if (! node->decrypted_child) {
+	fprintf (stderr, "Failed to decrypt part: %s\n",
+		 err ? err->message : "no error explanation given");
+	goto DONE;
+    }
+
+    node->decrypt_success = TRUE;
+    node->verify_attempted = TRUE;
+
+    /* This may be NULL if the part is not signed. */
+    node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
+    if (node->sig_list) {
+	g_object_ref (node->sig_list);
+	set_signature_list_destructor (node);
+    }
+    g_object_unref (decrypt_result);
+
+ DONE:
+    if (err)
+	g_error_free (err);
+}
+
+#else /* GMIME_ATLEAST_26 */
+
 static int
 _signature_validity_free (GMimeSignatureValidity **proxy)
 {
     g_mime_signature_validity_free (*proxy);
     return 0;
 }
-#endif
+
+static void
+set_signature_validity_destructor (mime_node_t *node)
+{
+    GMimeSignatureValidity **proxy = talloc (node, GMimeSignatureValidity *);
+    if (proxy) {
+	*proxy = node->sig_validity;
+	talloc_set_destructor (proxy, _signature_validity_free);
+    }
+}
+
+static void
+node_verify (mime_node_t *node, GMimeObject *part,
+	     notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+
+    node->verify_attempted = TRUE;
+    node->sig_validity = g_mime_multipart_signed_verify
+	(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
+    if (node->sig_validity) {
+	set_signature_validity_destructor (node);
+    } else {
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+    }
+
+    if (err)
+	g_error_free (err);
+}
+
+static void
+node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
+			 notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
+
+    node->decrypt_attempted = TRUE;
+    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+	(encrypteddata, cryptoctx, &err);
+    if (! node->decrypted_child) {
+	fprintf (stderr, "Failed to decrypt part: %s\n",
+		 err ? err->message : "no error explanation given");
+	goto DONE;
+    }
+
+    node->decrypt_success = TRUE;
+    node->verify_attempted = TRUE;
+
+    /* The GMimeSignatureValidity returned here is a const, unlike the
+     * one returned by g_mime_multipart_signed_verify() in
+     * node_verify() above, so the destructor is not needed.
+     */
+    node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+    if (! node->sig_validity)
+	fprintf (stderr, "Failed to verify encrypted signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+
+ DONE:
+    if (err)
+	g_error_free (err);
+}
+
+#endif  /* GMIME_ATLEAST_26 */
 
 static mime_node_t *
 _mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
-    GError *err = NULL;
     notmuch_crypto_context_t *cryptoctx = NULL;
 
     /* Set basic node properties */
@@ -198,32 +327,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 		     "message (must be exactly 2)\n",
 		     node->nchildren);
 	} else {
-	    GMimeMultipartEncrypted *encrypteddata =
-		GMIME_MULTIPART_ENCRYPTED (part);
-	    node->decrypt_attempted = TRUE;
-#ifdef GMIME_ATLEAST_26
-	    GMimeDecryptResult *decrypt_result = NULL;
-	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, cryptoctx, &decrypt_result, &err);
-#else
-	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, cryptoctx, &err);
-#endif
-	    if (node->decrypted_child) {
-		node->decrypt_success = node->verify_attempted = TRUE;
-#ifdef GMIME_ATLEAST_26
-		/* This may be NULL if the part is not signed. */
-		node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
-		if (node->sig_list)
-		    g_object_ref (node->sig_list);
-		g_object_unref (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"));
-	    }
+	    node_decrypt_and_verify (node, part, cryptoctx);
 	}
     } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify && cryptoctx) {
 	if (node->nchildren != 2) {
@@ -232,56 +336,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 		     "(must be exactly 2)\n",
 		     node->nchildren);
 	} else {
-#ifdef GMIME_ATLEAST_26
-	    node->sig_list = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
-	    node->verify_attempted = TRUE;
-
-	    if (!node->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
-	     * g_mime_multipart_encrypted_get_signature_validity,
-	     * and therefore needs to be properly disposed of.
-	     *
-	     * In GMime 2.6, they're both non-const, so we'll be able
-	     * to clean up this asymmetry. */
-	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
-	    node->verify_attempted = TRUE;
-	    node->sig_validity = sig_validity;
-	    if (sig_validity) {
-		GMimeSignatureValidity **proxy =
-		    talloc (node, GMimeSignatureValidity *);
-		*proxy = sig_validity;
-		talloc_set_destructor (proxy, _signature_validity_free);
-	    }
-#endif
+	    node_verify (node, part, cryptoctx);
 	}
     }
 
-#ifdef GMIME_ATLEAST_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 *);
-	*proxy = node->sig_list;
-	talloc_set_destructor (proxy, _signature_list_free);
-    }
-#endif
-
-#ifndef GMIME_ATLEAST_26
-    if (node->verify_attempted && !node->sig_validity)
-	fprintf (stderr, "Failed to verify signed part: %s\n",
-		 (err ? err->message : "no error explanation given"));
-#endif
-
-    if (err)
-	g_error_free (err);
-
     return node;
 }
 
-- 
1.7.10.4

Re: [PATCH 2/2] cli: mime node: abstract decryption and signature verification

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
> read. Abstract the decryption and signature verification into
> functions, with separate implementations for GMime 2.4 and 2.6, to
> clarify the code.

This series mostly looks OK, although it's a challenge to track all the
code movement.

I'd like some way to make it more obvious to the reader which version of
the functions they are reading (when the #ifdef GMIME_ATLEAST_26 has
scrolled off screen). The simplest would just be a comment in front of
each function.

d

Re: [PATCH 2/2] cli: mime node: abstract decryption and signature verification

[Jani Nikula]

On Sat, 30 Mar 2013, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
>> read. Abstract the decryption and signature verification into
>> functions, with separate implementations for GMime 2.4 and 2.6, to
>> clarify the code.
>
> This series mostly looks OK, although it's a challenge to track all the
> code movement.

I agree it is. Personally I just tried to look at support for either 2.4
or 2.6 at a time, and compare the old and new code paths, to wrap my
head around it.

BTW I didn't test the 2.4 support, I don't have a system with that
anymore.

> I'd like some way to make it more obvious to the reader which version of
> the functions they are reading (when the #ifdef GMIME_ATLEAST_26 has
> scrolled off screen). The simplest would just be a comment in front of
> each function.

That feels a bit excessive, but perhaps that is just because I'm very
accustomed to this style of conditional build. I can add the comments.

BR,
Jani.