From f8143d3703a6c6e1b100f465fa2407fc71cb5217 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Sat, 19 May 2012 15:21:57 +2000 Subject: [PATCH] Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed --- df/70ecf1bbf633b050f12c46cb966b1546762ac9 | 232 ++++++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100644 df/70ecf1bbf633b050f12c46cb966b1546762ac9 diff --git a/df/70ecf1bbf633b050f12c46cb966b1546762ac9 b/df/70ecf1bbf633b050f12c46cb966b1546762ac9 new file mode 100644 index 000000000..60c273682 --- /dev/null +++ b/df/70ecf1bbf633b050f12c46cb966b1546762ac9 @@ -0,0 +1,232 @@ +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 EC2B2431FB6 + for ; Fri, 18 May 2012 12:22:01 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 lUsFhapNj3dv for ; + Fri, 18 May 2012 12:22:01 -0700 (PDT) +Received: from dmz-mailsec-scanner-6.mit.edu (DMZ-MAILSEC-SCANNER-6.MIT.EDU + [18.7.68.35]) + by olra.theworths.org (Postfix) with ESMTP id EA93B431FAE + for ; Fri, 18 May 2012 12:22:00 -0700 (PDT) +X-AuditID: 12074423-b7fcc6d0000008a8-3d-4fb6a158d41b +Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) + by dmz-mailsec-scanner-6.mit.edu (Symantec Messaging Gateway) with SMTP + id 93.48.02216.851A6BF4; Fri, 18 May 2012 15:22:00 -0400 (EDT) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id q4IJLxdg028457; + Fri, 18 May 2012 15:22:00 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q4IJLwB3022516 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Fri, 18 May 2012 15:21:59 -0400 (EDT) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1SVSkQ-00073K-1p; Fri, 18 May 2012 15:21:58 -0400 +Date: Fri, 18 May 2012 15:21:57 -0400 +From: Austin Clements +To: Jameson Graef Rollins +Subject: Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only + when needed +Message-ID: <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> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +Content-Disposition: inline +In-Reply-To: <1337362357-31281-6-git-send-email-jrollins@finestructure.net> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IRYrdT0Y1YuM3f4O0DSYs9+7wsrt+cyezA + 5HH3NJfHs1W3mAOYorhsUlJzMstSi/TtErgyNqz5zVzwWq/i16zTbA2My1S7GDk5JARMJC5f + /scMYYtJXLi3nq2LkYtDSGAfo8S+yT0sEM4GRon9KzqgnJNMErPO/meGcJYwSnyc/QOsn0VA + VeLu1nVgNpuAhsS2/csZQWwRATOJni9/wGxmAS2JrRs/gNnCAhESbxsusYDYvAI6Elu73oDF + hQT6mSVW/tKHiAtKnJz5hAWm98a/l0xdjBxAtrTE8n8cIGFOAW+J2/MegJWICqhITDm5jW0C + o9AsJN2zkHTPQuhewMi8ilE2JbdKNzcxM6c4NVm3ODkxLy+1SNdMLzezRC81pXQTIzioXZR3 + MP45qHSIUYCDUYmH9+Kkbf5CrIllxZW5hxglOZiURHl95wGF+JLyUyozEosz4otKc1KLDzFK + cDArifDOnA6U401JrKxKLcqHSUlzsCiJ82povfMTEkhPLEnNTk0tSC2CycpwcChJ8K5bANQo + WJSanlqRlplTgpBm4uAEGc4DNLwbpIa3uCAxtzgzHSJ/ilFRSpx3GUhCACSRUZoH1wtLOq8Y + xYFeEeadDVLFA0xYcN2vgAYzAQ2uZAMbXJKIkJJqYDT++vpUc4mCWRTbvIMLtL15n4fkP3Q1 + tdsscMLdal+LbvferGYebu6djw+fk+n6tcXs6Sax5/d8JB8de7LmJPOrbay/vHX3h1Zxm7ja + JjuVBe8JXuvZL9Nme+4qQ0LS5LNfF5qGv7TXCAh++WFSQ21w3d/NhvsYmc8dFnLwNTuerCzf + 4WO6X4mlOCPRUIu5qDgRAMs+ntsVAwAA +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:22:02 -0000 + +Quoth Jameson Graef Rollins on May 18 at 10:32 am: +> Move the creation of the crypto ctx into mime-node.c and create it +> only when needed. This removes code duplication from notmuch-show and +> notmuch-reply, and should speed up these functions considerably if the +> crypto flags are provided but the messages don't have any +> cryptographic parts. +> --- +> mime-node.c | 25 +++++++++++++++++++++++++ +> notmuch-client.h | 3 ++- +> notmuch-reply.c | 19 ------------------- +> notmuch-show.c | 23 ----------------------- +> 4 files changed, 27 insertions(+), 43 deletions(-) +> +> diff --git a/mime-node.c b/mime-node.c +> index 3adbe5a..592e0b6 100644 +> --- a/mime-node.c +> +++ b/mime-node.c +> @@ -182,6 +182,31 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part) +> return NULL; +> } +> +> + /* 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. + +> +#ifdef GMIME_ATLEAST_26 +> + /* TODO: GMimePasswordRequestFunc */ +> + node->ctx->crypto->gpgctx = g_mime_gpg_context_new (NULL, "gpg"); +> +#else +> + GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); +> + node->ctx->crypto->gpgctx = g_mime_gpg_context_new (session, "gpg"); +> + g_object_unref (session); +> +#endif +> + if (node->ctx->crypto->gpgctx) { +> + g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) node->ctx->crypto->gpgctx, FALSE); +> + } else { +> + /* If we fail to create the gpgctx set the verify and +> + * decrypt flags to FALSE so we don't try to do any +> + * further verification or decryption */ +> + node->ctx->crypto->verify = FALSE; +> + node->ctx->crypto->decrypt = FALSE; +> + fprintf (stderr, "Failed to construct gpg context.\n"); +> + } +> + } +> + } +> + +> /* Handle PGP/MIME parts */ +> if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto->decrypt) { +> if (node->nchildren != 2) { +> diff --git a/notmuch-client.h b/notmuch-client.h +> index c671c13..c79eaa9 100644 +> --- a/notmuch-client.h +> +++ b/notmuch-client.h +> @@ -348,7 +348,8 @@ struct mime_node { +> /* Construct a new MIME node pointing to the root message part of +> * message. If crypto->verify is true, signed child parts will be +> * verified. If crypto->decrypt is true, encrypted child parts will be +> - * decrypted. +> + * decrypted. The GPG context crypto->gpgctx does not need to be +> + * pre-initialized as it will be initialized lazily as needed. + +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. + +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. + +> * +> * Return value: +> * +> diff --git a/notmuch-reply.c b/notmuch-reply.c +> index 345be76..1a61aa7 100644 +> --- a/notmuch-reply.c +> +++ b/notmuch-reply.c +> @@ -707,25 +707,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[]) +> else +> reply_format_func = notmuch_reply_format_default; +> +> - if (crypto.decrypt) { +> -#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"); +> -#endif +> - if (crypto.gpgctx) { +> - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE); +> - } else { +> - crypto.decrypt = FALSE; +> - fprintf (stderr, "Failed to construct gpg context.\n"); +> - } +> -#ifndef GMIME_ATLEAST_26 +> - g_object_unref (session); +> -#endif +> - } +> - +> config = notmuch_config_open (ctx, NULL, NULL); +> if (config == NULL) +> return 1; +> diff --git a/notmuch-show.c b/notmuch-show.c +> index f4ee038..5f785d0 100644 +> --- a/notmuch-show.c +> +++ b/notmuch-show.c +> @@ -1056,29 +1056,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) +> break; +> } +> +> - if (params.crypto.decrypt || params.crypto.verify) { +> -#ifdef GMIME_ATLEAST_26 +> - /* TODO: GMimePasswordRequestFunc */ +> - params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg"); +> -#else +> - GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); +> - params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg"); +> -#endif +> - if (params.crypto.gpgctx) { +> - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE); +> - } else { +> - /* If we fail to create the gpgctx set the verify and +> - * decrypt flags to FALSE so we don't try to do any +> - * further verification or decryption */ +> - params.crypto.verify = FALSE; +> - params.crypto.decrypt = FALSE; +> - fprintf (stderr, "Failed to construct gpg context.\n"); +> - } +> -#ifndef GMIME_ATLEAST_26 +> - g_object_unref (session); +> -#endif +> - } +> - +> config = notmuch_config_open (ctx, NULL, NULL); +> if (config == NULL) +> return 1; -- 2.26.2