From b65c65a2a228cc028d551ca006aeb34c352bd6fe Mon Sep 17 00:00:00 2001 From: Jameson Graef Rollins Date: Sat, 19 May 2012 12:45:08 +1700 Subject: [PATCH] Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed --- ed/090a9e7ff87fb3da420431774c1b9cbec5cabd | 136 ++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 ed/090a9e7ff87fb3da420431774c1b9cbec5cabd diff --git a/ed/090a9e7ff87fb3da420431774c1b9cbec5cabd b/ed/090a9e7ff87fb3da420431774c1b9cbec5cabd new file mode 100644 index 000000000..d51c9a417 --- /dev/null +++ b/ed/090a9e7ff87fb3da420431774c1b9cbec5cabd @@ -0,0 +1,136 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 86CB4431FB6 + for ; Fri, 18 May 2012 12:45:16 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -2.29 +X-Spam-Level: +X-Spam-Status: No, score=-2.29 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_MED=-2.3, T_MIME_NO_TEXT=0.01] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id rROkj6zrb-U4 for ; + Fri, 18 May 2012 12:45:15 -0700 (PDT) +Received: from outgoing-mail.its.caltech.edu (outgoing-mail.its.caltech.edu + [131.215.239.19]) + by olra.theworths.org (Postfix) with ESMTP id 04677431FAE + for ; Fri, 18 May 2012 12:45:15 -0700 (PDT) +Received: from fire-doxen.imss.caltech.edu (localhost [127.0.0.1]) + by fire-doxen-postvirus (Postfix) with ESMTP id B78962E507A4; + Fri, 18 May 2012 12:45:14 -0700 (PDT) +X-Spam-Scanned: at Caltech-IMSS on fire-doxen by amavisd-new +Received: from finestructure.net (rrcs-24-103-26-131.nyc.biz.rr.com + [24.103.26.131]) (Authenticated sender: jrollins) + by fire-doxen-submit (Postfix) with ESMTP id A9C11328075; + Fri, 18 May 2012 12:45:11 -0700 (PDT) +Received: by finestructure.net (Postfix, from userid 1000) + id 893574AD; Fri, 18 May 2012 12:45:10 -0700 (PDT) +From: Jameson Graef Rollins +To: Austin Clements +Subject: Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only + when needed +In-Reply-To: <20120518192157.GV11804@mit.edu> +References: <1337362357-31281-1-git-send-email-jrollins@finestructure.net> + <1337362357-31281-2-git-send-email-jrollins@finestructure.net> + <1337362357-31281-3-git-send-email-jrollins@finestructure.net> + <1337362357-31281-4-git-send-email-jrollins@finestructure.net> + <1337362357-31281-5-git-send-email-jrollins@finestructure.net> + <1337362357-31281-6-git-send-email-jrollins@finestructure.net> + <20120518192157.GV11804@mit.edu> +User-Agent: Notmuch/0.12+183~g9d5ff3c (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Fri, 18 May 2012 12:45:08 -0700 +Message-ID: <87txzd9su3.fsf@servo.finestructure.net> +MIME-Version: 1.0 +Content-Type: multipart/signed; boundary="=-=-="; + micalg=pgp-sha256; protocol="application/pgp-signature" +Cc: Notmuch Mail +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Fri, 18 May 2012 19:45:16 -0000 + +--=-=-= + +On Fri, May 18 2012, Austin Clements wrote: +>> + /* Lazily create the gpgctx if it's needed and hasn't been initialized yet */ +>> + if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part)) +>> + && (node->ctx->crypto->verify || node->ctx->crypto->decrypt)) { +>> + if (!node->ctx->crypto->gpgctx) { +> +> These if conditions could be combined, like +> +> if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part)) +> && (node->ctx->crypto->verify || node->ctx->crypto->decrypt) +> && !node->ctx->crypto->gpgctx) { +> +> When I see two nested 'if's like this, I expect there to be an else +> part to the inner if or something after the inner if (why else would +> it be separate?) and then I wind up matching braces when I don't see +> anything. Also, one 'if' would save a level of indentation. + +To explain what I was explaining on IRC, and what I should have noted in +the original patch, I did this on purpose because I'm looking forward to +the S/MIME support I was trying to get working. + +gmime 2.6 provides a separate context for pkcs7 (S/MIME). In this +context initialization section we will therefore have to test for +initialization of the relevant context. Since I knew that going into +this I decided to anticipate it by constructing things this way now +future diffs wouldn't have to include the indentation of this section +and could therefore be cleaner and smaller. If folks have issue with +that explanation let me know. + +> Perhaps "If crypto->gpgctx is NULL, it will be lazily initialized."? +> The variable does have to be "initialized", in the sense that it can't +> be uninitialized data. + +That sounds like a better wording. I'll fix. + +> It's slightly awkward that it's the caller's responsibility to free +> this lazily constructed object. That should probably be documented. +> We could more carefully reference count it, but I think that would +> actually be worse because the reference count would probably bounce +> through zero frequently. + +I agree that this is awkward. Is there a suggestion on how to do it +better? We only want to initialize it if it's needed, and only +_mime_node_create knows that. And we don't want to free it with +_mime_node_context_free, or something, only to have to reinitialize it +again with the next node or message. Thoughts? + +jamie. + +--=-=-= +Content-Type: application/pgp-signature + +-----BEGIN PGP SIGNATURE----- +Version: GnuPG v1.4.12 (GNU/Linux) + +iQIcBAEBCAAGBQJPtqbEAAoJEO00zqvie6q8GOYP+wZO99ubAxHOriE9RK5KqZQM +ATLxdRhZ9r1AElwTo7sZpyh1uAiprArjN9U7UM5BwZKICfXaaw7kJ9uNdQibdb2B +m7DnPUtf4dFkUTHY0G4866evzUX2FehJwVHAmJ61mjlUtSNTqfkvgQ9aCBC0JuUS +yNFxLrSFtn4tEI38PCMrG+rkjcJwcGA/6KA5yvipjjLrYSG+YWvvdFA0q26MUC9g +oqycy0T4KminjofXckKoiGL/gs32i1u5AV8eORKBXDivbt6twU/aRnUucCIhqrMx +RXUZad7OLTG2G/OLTrXs/pNjIl39DJYivzzh8+xLEYeL7EX0Tw85wwxxojLI/hvA +xXRayU7R1pA3C8666OyA65uy22/Os/TSWYB2VmHqeIlgAt5I5bUGLaV6sKhN81Ho +4QbBAh0Fs976EUdLwKhZsUQ5VqhJk8whB1mmoa9PtQw6/thiNV28BgCkM8DIxWOK +DXDVkVmlbmUj93eYNYxqYaTncpQqX1cBEIlO/Mz+xON959kjwYRCS/D8yKXm6rZ9 +95HpztmcxBawjYhaErPtGwCGFGXBY4fs87Dx+8zLemgXRrNUPoWG/3lPfvBze+t2 +Ws4P7/9hO8aiqV9MMb7gQLqnE1xTNV9m5cT17aHe8klKVR8a1igyF2e8+3V3lpfH +PZuNaT4xDaV9x0/SGvio +=IZeH +-----END PGP SIGNATURE----- +--=-=-=-- -- 2.26.2