[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Daniel Kahn Gillmor 
On 05/18/2012 03:45 PM, Jameson Graef Rollins wrote:
> On Fri, May 18 2012, Austin Clements  wrote:
>> 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?

You could provide a "destructor" function for notmuch_crypto_t, which
whoever is responsible for the struct would need to call when they are
ready to dispose of it.

The destructor would just destroy any GMIME crypto contexts pointed to
by the struct, and reset those pointers to NULL.

Since the common workflow is a singleton notmuch_crypto_t that is a
subobject of the singleton notmuch_params_t, you could just call that
destructor function before the notmuch_params_t falls out of scope.

If you want to be fancy/symmetric, you could use the same pattern to
create a "destructor" function for notmuch_params_t (it would just
invoke the destructor on its crypto member), but this seems like
overkill to me, and not in line with the talloc approach of the rest of
the codebase.

I agree that it's a little awkward, but i think there's something of an
impedance mismatch between gmime's object interface and notmuch's use of
talloc and friends.

--dkg

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

[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Austin Clements 
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

[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Jameson Graef Rollins 
On Fri, May 18 2012, Daniel Kahn Gillmor  wrote:
> You could provide a "destructor" function for notmuch_crypto_t, which
> whoever is responsible for the struct would need to call when they are
> ready to dispose of it.
>
> The destructor would just destroy any GMIME crypto contexts pointed to
> by the struct, and reset those pointers to NULL.

That sounds reasonable.  I'll see if I can hack something like that.

> Since the common workflow is a singleton notmuch_crypto_t that is a
> subobject of the singleton notmuch_params_t, you could just call that
> destructor function before the notmuch_params_t falls out of scope.

Just to be clear, notmuch_crypto_t is not only used as a subobject of
notmuch_show_params_t.  At least in what I submitted it is used on it's
own in notmuch-reply.c, in place of notmuch_show_params_t, since the
reply code was only using the crypto context to decrypt messages being
replied to.  So it's probably best to handle it independently of
notmuch_show_params_t.

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

[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Jameson Graef Rollins 
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.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL:

[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Jameson Graef Rollins 
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) {
+#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.
  *
  * 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;
-- 
1.7.10

[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Jameson Graef Rollins 
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) {
+#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.
  *
  * 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;
-- 
1.7.10

___
notmuch mailing list
notmuch@notmuchmail.org

Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Austin Clements 
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
 - /*

Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed

2012-05-18 Thread Jameson Graef Rollins 
On Fri, May 18 2012, Daniel Kahn Gillmor d...@fifthhorseman.net wrote:
 You could provide a destructor function for notmuch_crypto_t, which
 whoever is responsible for the struct would need to call when they are
 ready to dispose of it.

 The destructor would just destroy any GMIME crypto contexts pointed to
 by the struct, and reset those pointers to NULL.

That sounds reasonable.  I'll see if I can hack something like that.

 Since the common workflow is a singleton notmuch_crypto_t that is a
 subobject of the singleton notmuch_params_t, you could just call that
 destructor function before the notmuch_params_t falls out of scope.

Just to be clear, notmuch_crypto_t is not only used as a subobject of
notmuch_show_params_t.  At least in what I submitted it is used on it's
own in notmuch-reply.c, in place of notmuch_show_params_t, since the
reply code was only using the crypto context to decrypt messages being
replied to.  So it's probably best to handle it independently of
notmuch_show_params_t.

jamie.


pgpiVAR72ugZc.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch