[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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: <http://notmuchmail.org/pipermail/notmuch/attachments/20120518/4fd3f976/attachment-0001.pgp>
[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
Quoth Jameson Graef Rollins on May 18 at 10:32 am:
> This simplifies some more interfaces and gets rid of another #ifdef.
> ---
> mime-node.c | 22 --
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/mime-node.c b/mime-node.c
> index 183ff5d..3dda900 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -33,12 +33,7 @@ typedef struct mime_node_context {
> GMimeMessage *mime_message;
>
> /* Context provided by the caller. */
> -#ifdef GMIME_ATLEAST_26
> -GMimeCryptoContext *cryptoctx;
> -#else
> -GMimeCipherContext *cryptoctx;
> -#endif
> -notmuch_bool_t decrypt;
> +notmuch_crypto_t *crypto;
> } mime_node_context_t;
>
> static int
> @@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t
> *message,
> goto DONE;
> }
>
> -mctx->cryptoctx = crypto->gpgctx;
> -mctx->decrypt = crypto->decrypt;
> +mctx->crypto = crypto;
>
> /* Create the root node */
> root->part = GMIME_OBJECT (mctx->mime_message);
> @@ -190,7 +184,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>
> /* Handle PGP/MIME parts */
> if (GMIME_IS_MULTIPART_ENCRYPTED (part)
> - && node->ctx->cryptoctx && node->ctx->decrypt) {
> + && node->ctx->crypto->gpgctx && node->ctx->crypto->decrypt) {
> if (node->nchildren != 2) {
> /* this violates RFC 3156 section 4, so we won't bother with it. */
> fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> @@ -203,10 +197,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject
> *part)
> #ifdef GMIME_ATLEAST_26
> GMimeDecryptResult *decrypt_result = NULL;
> node->decrypted_child = g_mime_multipart_encrypted_decrypt
> - (encrypteddata, node->ctx->cryptoctx, _result, );
> + (encrypteddata, node->ctx->crypto->gpgctx, _result,
> );
> #else
> node->decrypted_child = g_mime_multipart_encrypted_decrypt
> - (encrypteddata, node->ctx->cryptoctx, );
> + (encrypteddata, node->ctx->crypto->gpgctx, );
> #endif
> if (node->decrypted_child) {
> node->decrypt_success = node->verify_attempted = TRUE;
> @@ -224,7 +218,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>(err ? err->message : "no error explanation given"));
> }
> }
> -} else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
> +} else if (GMIME_IS_MULTIPART_SIGNED (part) &&
> node->ctx->crypto->gpgctx) {
> if (node->nchildren != 2) {
> /* this violates RFC 3156 section 5, so we won't bother with it. */
> fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
> @@ -233,7 +227,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
> } else {
> #ifdef GMIME_ATLEAST_26
> node->sig_list = g_mime_multipart_signed_verify
> - (GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, );
> + (GMIME_MULTIPART_SIGNED (part), node->ctx->crypto->gpgctx,
> );
> node->verify_attempted = TRUE;
>
> if (!node->sig_list)
> @@ -249,7 +243,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>* 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), node->ctx->cryptoctx, );
> + (GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, );
This should be crypto->gpgctx.
> node->verify_attempted = TRUE;
> node->sig_validity = sig_validity;
> if (sig_validity) {
[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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: <http://notmuchmail.org/pipermail/notmuch/attachments/20120518/6713f727/attachment.pgp>
[PATCH 4/6] cli: intialize crypto structure in show and reply
On 05/18/2012 04:20 AM, Jani Nikula wrote:
> We have -Wextra, which enables -Wmissing-field-initializers, which
> requires us to use full initialization of struct fields when doing
> regular, non-designated initialization. The point is that you might
> introduce subtle bugs if you added new struct fields and forgot to check
> the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
> of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)
i think we can agree that this is the right choice. We might even want
to discourage non-designated initializations entirely.
> IMHO the whole point of designated initializers is that the
> initialization is not vulnerable to struct changes, and you can pick
> which fields you choose to initialize explicitly. Also, it has the added
> benefit of documenting the fields that are initialized, without having
> to look at the struct definition.
Agreed.
> Do we now want to initialize all struct fields explicitly, everywhere,
> even when using designated initializers? Isn't that the question then?
I'm not sure it has to be this dramatic and "all or nothing". For
example, it could be reasonable to explicitly initialize some subobjects
and not others. For example, the notmuch_crypto_t jamie is proposing
would effectively encode the default setting for the --verify and
--decrypt flags. I could see wanting to explicitly initialize those
default policy choices, even if they happen to be identical to the
implicit "zero"ing.
> Won't that maintain and promote the misconception that explicit
> initialization is required, when it's really not, failing to educate the
> non-experts and planting a seed of doubt in the experts...?
i see your point here, which is why i'm not arguing that all subobjects
need to be explicitly initialized all the time.
> It's not always clear whether something is a matter of taste, style, or
> language paradigm. If it feels like a paradigm, sticking with it
> ultimately benefits *both* perspectives.
yep, understood.
--dkg
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1030 bytes
Desc: OpenPGP digital signature
URL:
<http://notmuchmail.org/pipermail/notmuch/attachments/20120518/10cf7d3e/attachment.pgp>
[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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:
<http://notmuchmail.org/pipermail/notmuch/attachments/20120518/e3fd7ec4/attachment.pgp>
[PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
On Fri, May 18 2012, Austin Clements wrote: >> -(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, ); >> +(GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, ); > > This should be crypto->gpgctx. Weird. Good catch, Austin! But I must admit I'm thoroughly confused. I tested every stage of this series right before I sent, and sure enough this patch compiles cleanly and all tests pass. Can anyone explain why? I don't understand. I'll send a new version of this one patch now anyway. jamie. -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 835 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120518/98b883ac/attachment.pgp>
Using procmail to set tags
Tomi Ollila writes: > On Tue, May 15 2012, Austin Clements wrote: > * execute notmuch new > * use the environment variables in post-new hook > (notmuch tags $NEW_TAGS -new tag:new) > Thanks to all, that gave me enough hints to make progress. It looks like adding a new tag in the config, "needsProcessing", and then a post-new hook to a script that I write should do the job. Instead of dealing an even more complex and arcane procmail setup, I write a script that runs as the post-new hook. I'm finding the "new" flagging works correctly for simple mail drop from postfix, so the extra flag will work. I don't want to work off "new" because that's also useful for mail reading and other uses. The post-new hook can start by finding the emails that are tagged "needsProcessing", parse and tag them appropriately, and remove the needsProcessing tag. It just needs to start with a query and be written to work on all the messages found, so it doesn't matter how many are new. At the moment it looks like a simple python script using the notmuch library binding may be easiest. If not, there are other tools to do the job. R Horn rjhorn at alum.mit.edu
[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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 4/5] cli: new crypto verify flag to handle verification
Use this flag rather than depend on the existence of an initialized
gpgctx, to determine whether we should verify a multipart/signed. We
will be moving to create the ctx lazily, so we don't want to depend on
it being previously initialized if it's not needed.
---
mime-node.c |5 ++---
notmuch-client.h |8
notmuch-reply.c |1 +
notmuch-show.c | 14 +++---
4 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index 3dda900..3adbe5a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -183,8 +183,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
}
/* Handle PGP/MIME parts */
-if (GMIME_IS_MULTIPART_ENCRYPTED (part)
- && node->ctx->crypto->gpgctx && node->ctx->crypto->decrypt) {
+if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto->decrypt) {
if (node->nchildren != 2) {
/* this violates RFC 3156 section 4, so we won't bother with it. */
fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
@@ -218,7 +217,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
(err ? err->message : "no error explanation given"));
}
}
-} else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->gpgctx) {
+} else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify) {
if (node->nchildren != 2) {
/* this violates RFC 3156 section 5, so we won't bother with it. */
fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
diff --git a/notmuch-client.h b/notmuch-client.h
index 9892968..c671c13 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -80,6 +80,7 @@ typedef struct notmuch_crypto {
#else
GMimeCipherContext* gpgctx;
#endif
+notmuch_bool_t verify;
notmuch_bool_t decrypt;
} notmuch_crypto_t;
@@ -345,10 +346,9 @@ struct mime_node {
};
/* Construct a new MIME node pointing to the root message part of
- * message. If crypto->gpgctx is non-NULL, it will be used to verify
- * signatures on any child parts. If crypto->decrypt is true, then
- * crypto.gpgctx will additionally be used to decrypt any encrypted
- * child parts.
+ * message. If crypto->verify is true, signed child parts will be
+ * verified. If crypto->decrypt is true, encrypted child parts will be
+ * decrypted.
*
* Return value:
*
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 34a906e..345be76 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -674,6 +674,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
int opt_index, ret = 0;
int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
notmuch_crypto_t crypto = {
+ .verify = FALSE,
.decrypt = FALSE
};
int format = FORMAT_DEFAULT;
diff --git a/notmuch-show.c b/notmuch-show.c
index 66c74e2..f4ee038 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -987,11 +987,11 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
.part = -1,
.omit_excluded = TRUE,
.crypto = {
+ .verify = FALSE,
.decrypt = FALSE
}
};
int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
-notmuch_bool_t verify = FALSE;
int exclude = EXCLUDE_TRUE;
notmuch_opt_desc_t options[] = {
@@ -1008,7 +1008,7 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
{ NOTMUCH_OPT_INT, , "part", 'p', 0 },
{ NOTMUCH_OPT_BOOLEAN, _thread, "entire-thread", 't', 0 },
{ NOTMUCH_OPT_BOOLEAN, , "decrypt", 'd', 0 },
- { NOTMUCH_OPT_BOOLEAN, , "verify", 'v', 0 },
+ { NOTMUCH_OPT_BOOLEAN, , "verify", 'v', 0 },
{ 0, 0, 0, 0, 0 }
};
@@ -1018,6 +1018,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
return 1;
}
+/* decryption implies verification */
+if (params.crypto.decrypt)
+ params.crypto.verify = TRUE;
+
if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {
/* if part was requested and format was not specified, use format=raw */
if (params.part >= 0)
@@ -1052,7 +1056,7 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
break;
}
-if (params.crypto.decrypt || verify) {
+if (params.crypto.decrypt || params.crypto.verify) {
#ifdef GMIME_ATLEAST_26
/* TODO: GMimePasswordRequestFunc */
params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
@@ -1063,6 +1067,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
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
+
[PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
This simplifies some more interfaces and gets rid of another #ifdef.
---
mime-node.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index 183ff5d..3dda900 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -33,12 +33,7 @@ typedef struct mime_node_context {
GMimeMessage *mime_message;
/* Context provided by the caller. */
-#ifdef GMIME_ATLEAST_26
-GMimeCryptoContext *cryptoctx;
-#else
-GMimeCipherContext *cryptoctx;
-#endif
-notmuch_bool_t decrypt;
+notmuch_crypto_t *crypto;
} mime_node_context_t;
static int
@@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
-mctx->cryptoctx = crypto->gpgctx;
-mctx->decrypt = crypto->decrypt;
+mctx->crypto = crypto;
/* Create the root node */
root->part = GMIME_OBJECT (mctx->mime_message);
@@ -190,7 +184,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
/* Handle PGP/MIME parts */
if (GMIME_IS_MULTIPART_ENCRYPTED (part)
- && node->ctx->cryptoctx && node->ctx->decrypt) {
+ && node->ctx->crypto->gpgctx && node->ctx->crypto->decrypt) {
if (node->nchildren != 2) {
/* this violates RFC 3156 section 4, so we won't bother with it. */
fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
@@ -203,10 +197,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
#ifdef GMIME_ATLEAST_26
GMimeDecryptResult *decrypt_result = NULL;
node->decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node->ctx->cryptoctx, _result, );
+ (encrypteddata, node->ctx->crypto->gpgctx, _result,
);
#else
node->decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node->ctx->cryptoctx, );
+ (encrypteddata, node->ctx->crypto->gpgctx, );
#endif
if (node->decrypted_child) {
node->decrypt_success = node->verify_attempted = TRUE;
@@ -224,7 +218,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
(err ? err->message : "no error explanation given"));
}
}
-} else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->cryptoctx) {
+} else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->gpgctx) {
if (node->nchildren != 2) {
/* this violates RFC 3156 section 5, so we won't bother with it. */
fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
@@ -233,7 +227,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
} else {
#ifdef GMIME_ATLEAST_26
node->sig_list = g_mime_multipart_signed_verify
- (GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, );
+ (GMIME_MULTIPART_SIGNED (part), node->ctx->crypto->gpgctx,
);
node->verify_attempted = TRUE;
if (!node->sig_list)
@@ -249,7 +243,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
* 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), node->ctx->cryptoctx, );
+ (GMIME_MULTIPART_SIGNED (part), node->ctx->crypto.gpgctx, );
node->verify_attempted = TRUE;
node->sig_validity = sig_validity;
if (sig_validity) {
--
1.7.10
[PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument
This simplifies the interface considerably, getting rid of #ifdefs
along the way.
---
mime-node.c | 11 +++
notmuch-client.h | 14 +-
notmuch-reply.c |6 ++
notmuch-show.c |3 +--
4 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..183ff5d 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -61,12 +61,7 @@ _mime_node_context_free (mime_node_context_t *res)
notmuch_status_t
mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
- GMimeCryptoContext *cryptoctx,
-#else
- GMimeCipherContext *cryptoctx,
-#endif
- notmuch_bool_t decrypt, mime_node_t **root_out)
+ notmuch_crypto_t *crypto, mime_node_t **root_out)
{
const char *filename = notmuch_message_get_filename (message);
mime_node_context_t *mctx;
@@ -118,8 +113,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
-mctx->cryptoctx = cryptoctx;
-mctx->decrypt = decrypt;
+mctx->cryptoctx = crypto->gpgctx;
+mctx->decrypt = crypto->decrypt;
/* Create the root node */
root->part = GMIME_OBJECT (mctx->mime_message);
diff --git a/notmuch-client.h b/notmuch-client.h
index 2ad24cf..9892968 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -345,9 +345,10 @@ struct mime_node {
};
/* Construct a new MIME node pointing to the root message part of
- * message. If cryptoctx is non-NULL, it will be used to verify
- * signatures on any child parts. If decrypt is true, then cryptoctx
- * will additionally be used to decrypt any encrypted child parts.
+ * message. If crypto->gpgctx is non-NULL, it will be used to verify
+ * signatures on any child parts. If crypto->decrypt is true, then
+ * crypto.gpgctx will additionally be used to decrypt any encrypted
+ * child parts.
*
* Return value:
*
@@ -359,12 +360,7 @@ struct mime_node {
*/
notmuch_status_t
mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
- GMimeCryptoContext *cryptoctx,
-#else
- GMimeCipherContext *cryptoctx,
-#endif
- notmuch_bool_t decrypt, mime_node_t **node_out);
+ notmuch_crypto_t *crypto, mime_node_t **node_out);
/* Return a new MIME node for the requested child part of parent.
* parent will be used as the talloc context for the returned child
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 8d608e1..34a906e 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -544,8 +544,7 @@ notmuch_reply_format_default(void *ctx,
g_object_unref (G_OBJECT (reply));
reply = NULL;
- if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
- ) == NOTMUCH_STATUS_SUCCESS) {
+ if (mime_node_open (ctx, message, crypto, ) ==
NOTMUCH_STATUS_SUCCESS) {
format_part_reply (root);
talloc_free (root);
}
@@ -574,8 +573,7 @@ notmuch_reply_format_json(void *ctx,
messages = notmuch_query_search_messages (query);
message = notmuch_messages_get (messages);
-if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
- ) != NOTMUCH_STATUS_SUCCESS)
+if (mime_node_open (ctx, message, crypto, ) != NOTMUCH_STATUS_SUCCESS)
return 1;
reply = create_reply_message (ctx, config, message, reply_all);
diff --git a/notmuch-show.c b/notmuch-show.c
index 7779c3e..66c74e2 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -810,8 +810,7 @@ show_message (void *ctx,
mime_node_t *root, *part;
notmuch_status_t status;
-status = mime_node_open (local, message, params->crypto.gpgctx,
-params->crypto.decrypt, );
+status = mime_node_open (local, message, &(params->crypto), );
if (status)
goto DONE;
part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part));
--
1.7.10
[PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters
The main point here is to keep track of the crypto stuff together in
one place. In notmuch-show the crypto struct is a sub structure of
the parameters struct. In notmuch-reply, which had been using a
notmuch_show_params_t to store the crypto parameters, we can now just
use the general crypto struct.
I slip in a name change of the crypto context itself to better reflect
what the context is specifically for: it's actually a GPG context,
which is a sub type of Crypto context. There are other types of
Crypto contexts (Pkcs7 in particular) so we want to be clear.
The following patches will use this to simplify some function
interfaces.
---
notmuch-client.h | 16 ++--
notmuch-reply.c | 36 +++-
notmuch-show.c | 30 ++
3 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/notmuch-client.h b/notmuch-client.h
index 19b7f01..2ad24cf 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -74,17 +74,21 @@ typedef struct notmuch_show_format {
const char *message_set_end;
} notmuch_show_format_t;
+typedef struct notmuch_crypto {
+#ifdef GMIME_ATLEAST_26
+GMimeCryptoContext* gpgctx;
+#else
+GMimeCipherContext* gpgctx;
+#endif
+notmuch_bool_t decrypt;
+} notmuch_crypto_t;
+
typedef struct notmuch_show_params {
notmuch_bool_t entire_thread;
notmuch_bool_t omit_excluded;
notmuch_bool_t raw;
int part;
-#ifdef GMIME_ATLEAST_26
-GMimeCryptoContext* cryptoctx;
-#else
-GMimeCipherContext* cryptoctx;
-#endif
-notmuch_bool_t decrypt;
+notmuch_crypto_t crypto;
} notmuch_show_params_t;
/* There's no point in continuing when we've detected that we've done
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7184a5d..8d608e1 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -515,7 +515,7 @@ static int
notmuch_reply_format_default(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
-notmuch_show_params_t *params,
+notmuch_crypto_t *crypto,
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -544,7 +544,7 @@ notmuch_reply_format_default(void *ctx,
g_object_unref (G_OBJECT (reply));
reply = NULL;
- if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
+ if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
) == NOTMUCH_STATUS_SUCCESS) {
format_part_reply (root);
talloc_free (root);
@@ -559,7 +559,7 @@ static int
notmuch_reply_format_json(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
- notmuch_show_params_t *params,
+ notmuch_crypto_t *crypto,
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -574,7 +574,7 @@ notmuch_reply_format_json(void *ctx,
messages = notmuch_query_search_messages (query);
message = notmuch_messages_get (messages);
-if (mime_node_open (ctx, message, params->cryptoctx, params->decrypt,
+if (mime_node_open (ctx, message, crypto->gpgctx, crypto->decrypt,
) != NOTMUCH_STATUS_SUCCESS)
return 1;
@@ -605,7 +605,7 @@ static int
notmuch_reply_format_headers_only(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
- unused (notmuch_show_params_t *params),
+ unused (notmuch_crypto_t *crypto),
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -674,8 +674,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
notmuch_query_t *query;
char *query_string;
int opt_index, ret = 0;
-int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_show_params_t *params, notmuch_bool_t
reply_all);
-notmuch_show_params_t params = { .part = -1 };
+int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
+notmuch_crypto_t crypto = {
+ .decrypt = FALSE
+};
int format = FORMAT_DEFAULT;
int reply_all = TRUE;
@@ -689,7 +691,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
(notmuch_keyword_t []){ { "all", TRUE },
{ "sender", FALSE },
{ 0, 0 } } },
- { NOTMUCH_OPT_BOOLEAN, , "decrypt", 'd', 0 },
+ { NOTMUCH_OPT_BOOLEAN, , "decrypt", 'd', 0 },
{ 0, 0, 0, 0, 0 }
};
@@ -706,18 +708,18 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
else
reply_format_func = notmuch_reply_format_default;
-if (params.decrypt) {
+if
[PATCH v2 0/5] cli: improve handling of crypto parameters and contexts
Here's a second version of this series [0]. I think I have addressed most of Austin's and Jani's comments. I've tried to make a compromise between Jani's desire to not see any unneeded initialization and my desire to have clarity and to make defaults that define behavior explicit. I ended up only initializing the variables that define default user behavior. I hope that's ok. jamie. [0] id:"1337205359-2444-1-git-send-email-jrollins at finestructure.net"
[PATCH 4/6] cli: intialize crypto structure in show and reply
On Fri, May 18 2012, Daniel Kahn Gillmor wrote:
>
> The real tradeoff in this choice is whether we prefer:
>
> a) more compact code to facilitate quick reading by experts
>
>or
>
> b) more verbose code to facilitate comprehension by the non-expert.
>
> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P
>
> Overall, i think i'm still in the (b) camp. But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.
I also think it is good to see explicit initializations when those aren't
needed but clarifies the code. After all it doesn't generate any extra
code to the target module. Also the &(params->crypto) is good from
clarification point of view.
Austin's .crypto { ... } initialization looks good & clear; In case there
will be new version of this patch series I'd like to see that used...
> --dkg
>
> PS gcc's -pedantic argument provides the following warning:
>
> error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.
In order to do that id:"cover.1325977940.git.jani at nikula.org" needs to
be applied (and probably rebased).
> [0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
Tomi
[PATCH 4/6] cli: intialize crypto structure in show and reply
*sigh* I'm failing to detach myself from this conversation. :(
On Thu, 17 May 2012, Daniel Kahn Gillmor wrote:
> I don't think it's an assumption -- Jani is probably relying on the C
> standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:
Thanks for digging up the references.
> The real tradeoff in this choice is whether we prefer:
>
> a) more compact code to facilitate quick reading by experts
>
>or
>
> b) more verbose code to facilitate comprehension by the non-expert.
We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)
IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.
Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?
> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P
It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.
> Overall, i think i'm still in the (b) camp. But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.
Agreed.
> PS gcc's -pedantic argument provides the following warning:
>
> error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.
Unfortunately, the notmuch code base uses mixed standards, due to GCC
being so lax about it. Anything -pedantic produces warnings:
id:"cover.1325977940.git.jani at nikula.org". You may also want to try
clang to get better warnings.
BR,
Jani.
[PATCH 9/9] lib: Don't needlessly create directory docs in _notmuch_message_remove_filename
Previously, if passed a filename with a directory that did not exist in the database, _notmuch_message_remove_filename would needlessly create that directory document. Fix it so that doesn't happen. --- lib/message.cc |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/message.cc b/lib/message.cc index 8d552f1..6787506 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -541,8 +541,8 @@ _notmuch_message_remove_filename (notmuch_message_t *message, Xapian::TermIterator i, last; status = _notmuch_database_filename_to_direntry ( - local, message->notmuch, filename, NOTMUCH_FIND_CREATE, ); -if (status) + local, message->notmuch, filename, NOTMUCH_FIND_LOOKUP, ); +if (status || !direntry) return status; /* Unlink this file from its parent directory. */ -- 1.7.10
[PATCH 8/9] python: Remove find_message_by_filename workaround
Now that notmuch_database_find_message_by_filename works on read-only
databases, remove the workaround that disabled it on read-write
databases.
This also adds a regression test for find_message_by_filename.
---
bindings/python/notmuch/database.py |9 -
test/python |8
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/bindings/python/notmuch/database.py
b/bindings/python/notmuch/database.py
index ff89818..e5c74cf 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -526,19 +526,10 @@ class Database(object):
retry.
:raises: :exc:`NotInitializedError` if the database was not
intitialized.
-:raises: :exc:`ReadOnlyDatabaseError` if the database has not been
- opened in read-write mode
*Added in notmuch 0.9*"""
self._assert_db_is_initialized()
-# work around libnotmuch calling exit(3), see
-# id:20120221002921.8534.57091 at thinkbox.jade-hamburg.de
-# TODO: remove once this issue is resolved
-if self.mode != Database.MODE.READ_WRITE:
-raise ReadOnlyDatabaseError('The database has to be opened in '
-'read-write mode for get_directory')
-
msg_p = NotmuchMessageP()
status = Database._find_message_by_filename(self._db, _str(filename),
byref(msg_p))
diff --git a/test/python b/test/python
index 6018c2d..3f03a2e 100755
--- a/test/python
+++ b/test/python
@@ -28,4 +28,12 @@ EOF
notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id://
> EXPECTED
test_expect_equal_file OUTPUT EXPECTED
+test_begin_subtest "get non-existent file"
+test_python <
[PATCH 7/9] lib: Make notmuch_database_find_message_by_filename not crash on read-only databases
Previously, _notmuch_database_filename_to_direntry would abort with an
internal error when called on a read-only database. Now that creating
the directory document is optional,
notmuch_database_find_message_by_filename can disable directory
document creation (as it should) and, as a result, not abort on
read-only databases.
---
lib/database.cc |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/database.cc b/lib/database.cc
index e27a0e1..761dc1a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1895,8 +1895,8 @@ notmuch_database_find_message_by_filename
(notmuch_database_t *notmuch,
try {
status = _notmuch_database_filename_to_direntry (
- local, notmuch, filename, NOTMUCH_FIND_CREATE, );
- if (status)
+ local, notmuch, filename, NOTMUCH_FIND_LOOKUP, );
+ if (status || !direntry)
goto DONE;
term = talloc_asprintf (local, "%s%s", prefix, direntry);
--
1.7.10
[PATCH 6/9] python: Update Database.get_directory documentation
notmuch_database_get_directory no longer returns an error for read-only databases, so remove ReadOnlyDatabaseError from the list of get_directory exceptions. --- bindings/python/notmuch/database.py |3 --- 1 file changed, 3 deletions(-) diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py index 797554d..ff89818 100644 --- a/bindings/python/notmuch/database.py +++ b/bindings/python/notmuch/database.py @@ -346,7 +346,6 @@ class Database(object): def get_directory(self, path): """Returns a :class:`Directory` of path, -(creating it if it does not exist(?)) :param path: An unicode string containing the path relative to the path of database (see :meth:`get_path`), or else should be an absolute @@ -354,8 +353,6 @@ class Database(object): :returns: :class:`Directory` or raises an exception. :raises: :exc:`FileError` if path is not relative database or absolute with initial components same as database. -:raises: :exc:`ReadOnlyDatabaseError` if the database has not been - opened in read-write mode """ self._assert_db_is_initialized() -- 1.7.10
[PATCH 5/9] new: Remove workaround for detecting newly created directory objects
Previously, notmuch_database_get_directory did not indicate whether or
not the returned directory object was newly created, which required a
workaround to distinguish newly created directory objects with no
child messages from directory objects that had no mtime set but did
have child messages. Now that notmuch_database_get_directory
distinguishes whether or not the directory object exists in the
database, this workaround is no longer necessary.
---
notmuch-new.c | 36 +++-
1 file changed, 7 insertions(+), 29 deletions(-)
diff --git a/notmuch-new.c b/notmuch-new.c
index a3a8bec..72dd558 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -256,7 +256,7 @@ add_files_recursive (notmuch_database_t *notmuch,
notmuch_filenames_t *db_subdirs = NULL;
time_t stat_time;
struct stat st;
-notmuch_bool_t is_maildir, new_directory;
+notmuch_bool_t is_maildir;
const char **tag;
if (stat (path, )) {
@@ -281,33 +281,12 @@ add_files_recursive (notmuch_database_t *notmuch,
}
db_mtime = directory ? notmuch_directory_get_mtime (directory) : 0;
-new_directory = db_mtime ? FALSE : TRUE;
-
-/* XXX This is a temporary workaround. If we don't update the
- * database mtime until after processing messages in this
- * directory, then a 0 mtime is *not* sufficient to indicate that
- * this directory has no messages or subdirs in the database (for
- * example, if an earlier run skipped the mtime update because
- * fs_mtime == stat_time, or was interrupted before updating the
- * mtime at the end). To address this, we record a (bogus)
- * non-zero value before processing any child messages so that a
- * later run won't mistake this for a new directory (and, for
- * example, fail to detect removed files and subdirs).
- *
- * A better solution would be for notmuch_database_get_directory
- * to indicate if it really created a new directory or not, either
- * by a new out-argument, or by recording this information and
- * providing an accessor.
- */
-if (new_directory && directory)
- notmuch_directory_set_mtime (directory, -1);
-
/* If the database knows about this directory, then we sort based
* on strcmp to match the database sorting. Otherwise, we can do
* inode-based sorting for faster filesystem operation. */
num_fs_entries = scandir (path, _entries, 0,
- new_directory ?
- dirent_sort_inode : dirent_sort_strcmp_name);
+ directory ?
+ dirent_sort_strcmp_name : dirent_sort_inode);
if (num_fs_entries == -1) {
fprintf (stderr, "Error opening directory %s: %s\n",
@@ -376,13 +355,12 @@ add_files_recursive (notmuch_database_t *notmuch,
* being discovered until the clock catches up and the directory
* is modified again).
*/
-if (fs_mtime == db_mtime)
+if (directory && fs_mtime == db_mtime)
goto DONE;
-/* new_directory means a directory that the database has never
- * seen before. In that case, we can simply leave db_files and
- * db_subdirs NULL. */
-if (!new_directory) {
+/* If the database has never seen this directory before, we can
+ * simply leave db_files and db_subdirs NULL. */
+if (directory) {
db_files = notmuch_directory_get_child_files (directory);
db_subdirs = notmuch_directory_get_child_directories (directory);
}
--
1.7.10
[PATCH 4/9] lib: Make notmuch_database_get_directory return NULL if the directory is not found
Using the new support from _notmuch_directory_create, this makes
notmuch_database_get_directory a read-only operation that simply
returns the directory object if it exists or NULL otherwise. This
also means that notmuch_database_get_directory can work on read-only
databases.
This change breaks the directory mtime workaround in notmuch-new.c by
fixing the exact issue it was working around. This permits mtime
update races to prevent scans of changed directories, which
non-deterministically breaks a few tests. The next patch fixes this.
---
lib/database.cc |7 ++-
lib/notmuch-private.h |3 +++
lib/notmuch.h | 10 ++
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/lib/database.cc b/lib/database.cc
index b4c76b4..e27a0e1 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1328,12 +1328,9 @@ notmuch_database_get_directory (notmuch_database_t
*notmuch,
return NOTMUCH_STATUS_NULL_POINTER;
*directory = NULL;
-/* XXX Handle read-only databases properly */
-if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
- return NOTMUCH_STATUS_READ_ONLY_DATABASE;
-
try {
- *directory = _notmuch_directory_create (notmuch, path,
NOTMUCH_FIND_CREATE, );
+ *directory = _notmuch_directory_create (notmuch, path,
+ NOTMUCH_FIND_LOOKUP, );
} catch (const Xapian::Error ) {
fprintf (stderr, "A Xapian exception occurred getting directory: %s.\n",
error.get_msg().c_str());
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 34f7ac7..bfb4111 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -148,6 +148,9 @@ typedef enum _notmuch_private_status {
/* Flags shared by various lookup functions. */
typedef enum _notmuch_find_flags {
+/* Lookup without creating any documents. This is the default
+ * behavior. */
+NOTMUCH_FIND_LOOKUP = 0,
/* If set, create the necessary document (or documents) if they
* are missing. Requires a read/write database. */
NOTMUCH_FIND_CREATE = 1<<0,
diff --git a/lib/notmuch.h b/lib/notmuch.h
index bbb17e4..3633bed 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -300,10 +300,8 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch);
* (see notmuch_database_get_path), or else should be an absolute path
* with initial components that match the path of 'database'.
*
- * Note: Currently this will create the directory object if it doesn't
- * exist. In the future, when a directory object does not exist this
- * will return NOTMUCH_STATUS_SUCCESS and set *directory to NULL.
- * Callers should be prepared for this.
+ * If this directory object does not exist in the database, this
+ * returns NOTMUCH_STATUS_SUCCESS and sets *directory to NULL.
*
* Return value:
*
@@ -313,10 +311,6 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch);
*
* NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
* directory not retrieved.
- *
- * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
- * mode so the directory cannot be created (this case will be
- * removed in the future).
*/
notmuch_status_t
notmuch_database_get_directory (notmuch_database_t *database,
--
1.7.10
[PATCH 3/9] lib: Perform the same transformation to _notmuch_database_filename_to_direntry
Now _notmuch_database_filename_to_direntry takes a flags argument and
can indicate if the necessary directory documents do not exist.
Again, callers have been updated, but retain their original behavior.
---
lib/database.cc | 17 +++--
lib/message.cc|9 -
lib/notmuch-private.h |1 +
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/lib/database.cc b/lib/database.cc
index 716982d..b4c76b4 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1248,13 +1248,16 @@ _notmuch_database_get_directory_path (void *ctx,
* database path), return a new string (with 'ctx' as the talloc
* owner) suitable for use as a direntry term value.
*
- * The necessary directory documents will be created in the database
- * as needed.
+ * If (flags & NOTMUCH_FIND_CREATE), the necessary directory documents
+ * will be created in the database as needed. Otherwise, if the
+ * necessary directory documents do not exist, this sets
+ * *direntry to NULL and returns NOTMUCH_STATUS_SUCCESS.
*/
notmuch_status_t
_notmuch_database_filename_to_direntry (void *ctx,
notmuch_database_t *notmuch,
const char *filename,
+ notmuch_find_flags_t flags,
char **direntry)
{
const char *relative, *directory, *basename;
@@ -1268,10 +1271,12 @@ _notmuch_database_filename_to_direntry (void *ctx,
if (status)
return status;
-status = _notmuch_database_find_directory_id (notmuch, directory,
NOTMUCH_FIND_CREATE,
+status = _notmuch_database_find_directory_id (notmuch, directory, flags,
_id);
-if (status)
+if (status || directory_id == (unsigned int)-1) {
+ *direntry = NULL;
return status;
+}
*direntry = talloc_asprintf (ctx, "%u:%s", directory_id, basename);
@@ -1892,8 +1897,8 @@ notmuch_database_find_message_by_filename
(notmuch_database_t *notmuch,
local = talloc_new (notmuch);
try {
- status = _notmuch_database_filename_to_direntry (local, notmuch,
-filename, );
+ status = _notmuch_database_filename_to_direntry (
+ local, notmuch, filename, NOTMUCH_FIND_CREATE, );
if (status)
goto DONE;
diff --git a/lib/message.cc b/lib/message.cc
index 0075425..8d552f1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -495,9 +495,8 @@ _notmuch_message_add_filename (notmuch_message_t *message,
if (status)
return status;
-status = _notmuch_database_filename_to_direntry (local,
-message->notmuch,
-filename, );
+status = _notmuch_database_filename_to_direntry (
+ local, message->notmuch, filename, NOTMUCH_FIND_CREATE, );
if (status)
return status;
@@ -541,8 +540,8 @@ _notmuch_message_remove_filename (notmuch_message_t
*message,
notmuch_status_t status;
Xapian::TermIterator i, last;
-status = _notmuch_database_filename_to_direntry (local, message->notmuch,
-filename, );
+status = _notmuch_database_filename_to_direntry (
+ local, message->notmuch, filename, NOTMUCH_FIND_CREATE, );
if (status)
return status;
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index a36549d..34f7ac7 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -207,6 +207,7 @@ notmuch_status_t
_notmuch_database_filename_to_direntry (void *ctx,
notmuch_database_t *notmuch,
const char *filename,
+ notmuch_find_flags_t flags,
char **direntry);
/* directory.cc */
--
1.7.10
[PATCH 2/9] lib: Perform the same transformation to _notmuch_database_find_directory_id
Now _notmuch_database_find_directory_id takes a flags argument, which
it passes through to _notmuch_directory_create and can indicate if the
directory does not exist. Again, callers have been updated, but
retain their original behavior.
---
lib/database.cc | 14 +++---
lib/directory.cc |8 +++-
lib/notmuch-private.h |1 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/lib/database.cc b/lib/database.cc
index df996a9..716982d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1197,9 +1197,17 @@ _notmuch_database_split_path (void *ctx,
return NOTMUCH_STATUS_SUCCESS;
}
+/* Find the document ID of the specified directory.
+ *
+ * If (flags & NOTMUCH_FIND_CREATE), a new directory document will be
+ * created if one does not exist for 'path'. Otherwise, if the
+ * directory document does not exist, this sets *directory_id to
+ * ((unsigned int)-1) and returns NOTMUCH_STATUS_SUCCESS.
+ */
notmuch_status_t
_notmuch_database_find_directory_id (notmuch_database_t *notmuch,
const char *path,
+notmuch_find_flags_t flags,
unsigned int *directory_id)
{
notmuch_directory_t *directory;
@@ -1210,8 +1218,8 @@ _notmuch_database_find_directory_id (notmuch_database_t
*notmuch,
return NOTMUCH_STATUS_SUCCESS;
}
-directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE,
);
-if (status) {
+directory = _notmuch_directory_create (notmuch, path, flags, );
+if (status || !directory) {
*directory_id = -1;
return status;
}
@@ -1260,7 +1268,7 @@ _notmuch_database_filename_to_direntry (void *ctx,
if (status)
return status;
-status = _notmuch_database_find_directory_id (notmuch, directory,
+status = _notmuch_database_find_directory_id (notmuch, directory,
NOTMUCH_FIND_CREATE,
_id);
if (status)
return status;
diff --git a/lib/directory.cc b/lib/directory.cc
index 83bb19b..6a3ffed 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -153,7 +153,13 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
_notmuch_database_split_path (local, path, , );
- _notmuch_database_find_directory_id (notmuch, parent, _id);
+ *status_ret = _notmuch_database_find_directory_id (
+ notmuch, parent, NOTMUCH_FIND_CREATE, _id);
+ if (*status_ret) {
+ notmuch_directory_destroy (directory);
+ directory = NULL;
+ goto DONE;
+ }
if (basename) {
term = talloc_asprintf (local, "%s%u:%s",
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 538274f..a36549d 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -195,6 +195,7 @@ _notmuch_database_find_unique_doc_id (notmuch_database_t
*notmuch,
notmuch_status_t
_notmuch_database_find_directory_id (notmuch_database_t *database,
const char *path,
+notmuch_find_flags_t flags,
unsigned int *directory_id);
const char *
--
1.7.10
[PATCH 1/9] lib: Make directory document creation optional for _notmuch_directory_create
Previously this function would create directory documents if they
didn't exist. As a result, it could only be used on writable
databases. This adds an argument to make creation optional and to
make this function work on read-only databases. We use a flag
argument to avoid a bare boolean and to permit future expansion.
Both callers have been updated, but currently retain the old behavior.
We'll take advantage of the new argument in the following patches.
---
lib/database.cc |6 +++---
lib/directory.cc | 33 -
lib/notmuch-private.h |8
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/lib/database.cc b/lib/database.cc
index f8c4a7d..df996a9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -956,7 +956,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
document.get_value (NOTMUCH_VALUE_TIMESTAMP));
directory = _notmuch_directory_create (notmuch, term.c_str() +
10,
- );
+ NOTMUCH_FIND_CREATE,
);
notmuch_directory_set_mtime (directory, mtime);
notmuch_directory_destroy (directory);
}
@@ -1210,7 +1210,7 @@ _notmuch_database_find_directory_id (notmuch_database_t
*notmuch,
return NOTMUCH_STATUS_SUCCESS;
}
-directory = _notmuch_directory_create (notmuch, path, );
+directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE,
);
if (status) {
*directory_id = -1;
return status;
@@ -1320,7 +1320,7 @@ notmuch_database_get_directory (notmuch_database_t
*notmuch,
return NOTMUCH_STATUS_READ_ONLY_DATABASE;
try {
- *directory = _notmuch_directory_create (notmuch, path, );
+ *directory = _notmuch_directory_create (notmuch, path,
NOTMUCH_FIND_CREATE, );
} catch (const Xapian::Error ) {
fprintf (stderr, "A Xapian exception occurred getting directory: %s.\n",
error.get_msg().c_str());
diff --git a/lib/directory.cc b/lib/directory.cc
index 70e1693..83bb19b 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -82,28 +82,41 @@ find_directory_document (notmuch_database_t *notmuch,
return NOTMUCH_PRIVATE_STATUS_SUCCESS;
}
+/* Find or create a directory document.
+ *
+ * 'path' should be a path relative to the path of 'database', or else
+ * should be an absolute path with initial components that match the
+ * path of 'database'.
+ *
+ * If (flags & NOTMUCH_FIND_CREATE), then the directory document will
+ * be created if it does not exist. Otherwise, if the directory
+ * document does not exist, *status_ret is set to
+ * NOTMUCH_STATUS_SUCCESS and this returns NULL.
+ */
notmuch_directory_t *
_notmuch_directory_create (notmuch_database_t *notmuch,
const char *path,
+ notmuch_find_flags_t flags,
notmuch_status_t *status_ret)
{
Xapian::WritableDatabase *db;
notmuch_directory_t *directory;
notmuch_private_status_t private_status;
const char *db_path;
+notmuch_bool_t create = (flags & NOTMUCH_FIND_CREATE);
*status_ret = NOTMUCH_STATUS_SUCCESS;
path = _notmuch_database_relative_path (notmuch, path);
-if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+if (create && notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
INTERNAL_ERROR ("Failure to ensure database is writable");
-db = static_cast (notmuch->xapian_db);
-
directory = talloc (notmuch, notmuch_directory_t);
-if (unlikely (directory == NULL))
+if (unlikely (directory == NULL)) {
+ *status_ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
return NULL;
+}
directory->notmuch = notmuch;
@@ -122,6 +135,13 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
directory->document_id = directory->doc.get_docid ();
if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+ if (!create) {
+ notmuch_directory_destroy (directory);
+ directory = NULL;
+ *status_ret = NOTMUCH_STATUS_SUCCESS;
+ goto DONE;
+ }
+
void *local = talloc_new (directory);
const char *parent, *basename;
Xapian::docid parent_id;
@@ -145,6 +165,8 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
directory->doc.add_value (NOTMUCH_VALUE_TIMESTAMP,
Xapian::sortable_serialise (0));
+ db = static_cast (notmuch->xapian_db);
+
directory->document_id = _notmuch_database_generate_doc_id
(notmuch);
db->replace_document (directory->document_id, directory->doc);
talloc_free (local);
@@ -158,10 +180,11 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
error.get_msg().c_str());
[PATCH 0/9] Fix directory lookup on read-only databases
This fixes notmuch_database_get_directory and notmuch_database_find_message_by_filename so that they don't attempt to create missing directory documents. This makes them work on read-only databases (and prevents n_d_f_m_b_f from crashing unceremoniously on read-only databases). Unfortunately, there are several functions involved in directory document lookup, so the first three patches simply add a creation flag at each necessary layer. The remaining patches then fix up the two API functions and their uses. If we do a 0.13.1 bug fix release, these patches could go in to complement the API changes made in 0.13 to support these fixes. David can make that call. There are several patches, but they're all short and incremental. bindings/python/notmuch/database.py | 11 --- lib/database.cc | 40 +--- lib/directory.cc| 41 +++-- lib/message.cc | 11 +-- lib/notmuch-private.h | 13 + lib/notmuch.h | 10 ++ notmuch-new.c | 36 +++- test/python |8 8 files changed, 95 insertions(+), 75 deletions(-)
[PATCH 4/6] cli: intialize crypto structure in show and reply
On Thu, 17 May 2012, Jameson Graef Rollins wrote: > On Thu, May 17 2012, Jani Nikula wrote: >> The values are not undefined, they are properly initialized, and we can >> count on it. For sure, not maybe. If you want to explicitly set them for >> clarity, it's a matter of taste. Personally I find it too verbose, but then >> again notmuch code is generally fairly verbose. > > I want them explicitly set for clarity, as well as safety. Code is > meant to be read by humans, not computers. Brevity is not always a > virtue if it sacrifices clarity. It's much nicer to have the defaults > clearly stated in the initialization, than to force the reader to > understand how the initialization works and to interpret what that means > for the current case. I also don't think it's safe to assume that the > variables will be always be "properly" initialized in your favor in > perpetuity. It's much safer to explicitly set them to what you want > them to be rather than just assume they'll be set correctly. In short, when you read enough code, having everything explicitly stated becomes a burden. It's explicit, but you have to read it all, even when there really is no need to. >> If you insist on it, please at least drop the extra temp crypto >> variable, and initialize the struct in one initializer. > > I don't see why this matters either. Again, I think this is just a > matter of taste. I would rather the code be verbose where clarity > requires it, rather than always trying to make the code as terse as > possible. You introduce an extra variable that every reader of your code has to track down to realize that it's only ever used once to initialize another variable. You make code harder for other people to read. I have now offered my review and opinions on the matter; I will not pursue this discussion further. BR, Jani.
Re: [PATCH 4/6] cli: intialize crypto structure in show and reply
On Fri, May 18 2012, Daniel Kahn Gillmor d...@fifthhorseman.net wrote:
The real tradeoff in this choice is whether we prefer:
a) more compact code to facilitate quick reading by experts
or
b) more verbose code to facilitate comprehension by the non-expert.
I started this discussion leaning strongly toward the (b) perspective.
But now that i know the relevant bits of the standard, i can sympathize
with the (a) perspective as well :P
Overall, i think i'm still in the (b) camp. But i think it's more
important that we don't allow dithering over this issue to prevent the
inclusion of this patch series, which is a step in the right direction
for handling S/MIME messages as well as PGP/MIME.
I also think it is good to see explicit initializations when those aren't
needed but clarifies the code. After all it doesn't generate any extra
code to the target module. Also the (params-crypto) is good from
clarification point of view.
Austin's .crypto { ... } initialization looks good clear; In case there
will be new version of this patch series I'd like to see that used...
--dkg
PS gcc's -pedantic argument provides the following warning:
error: ISO C90 forbids specifying subobject to initialize
So we probably want to specify -std=c99 at least to ensure our choice of
subobject initialization is respected.
In order to do that id:cover.1325977940.git.j...@nikula.org needs to
be applied (and probably rebased).
[0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 4/6] cli: intialize crypto structure in show and reply
*sigh* I'm failing to detach myself from this conversation. :(
On Thu, 17 May 2012, Daniel Kahn Gillmor d...@fifthhorseman.net wrote:
I don't think it's an assumption -- Jani is probably relying on the C
standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:
Thanks for digging up the references.
The real tradeoff in this choice is whether we prefer:
a) more compact code to facilitate quick reading by experts
or
b) more verbose code to facilitate comprehension by the non-expert.
We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)
IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.
Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?
I started this discussion leaning strongly toward the (b) perspective.
But now that i know the relevant bits of the standard, i can sympathize
with the (a) perspective as well :P
It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.
Overall, i think i'm still in the (b) camp. But i think it's more
important that we don't allow dithering over this issue to prevent the
inclusion of this patch series, which is a step in the right direction
for handling S/MIME messages as well as PGP/MIME.
Agreed.
PS gcc's -pedantic argument provides the following warning:
error: ISO C90 forbids specifying subobject to initialize
So we probably want to specify -std=c99 at least to ensure our choice of
subobject initialization is respected.
Unfortunately, the notmuch code base uses mixed standards, due to GCC
being so lax about it. Anything -pedantic produces warnings:
id:cover.1325977940.git.j...@nikula.org. You may also want to try
clang to get better warnings.
BR,
Jani.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: Using procmail to set tags
Tomi Ollila tomi.oll...@iki.fi writes: On Tue, May 15 2012, Austin Clements amdra...@mit.edu wrote: * execute notmuch new * use the environment variables in post-new hook (notmuch tags $NEW_TAGS -new tag:new) Thanks to all, that gave me enough hints to make progress. It looks like adding a new tag in the config, needsProcessing, and then a post-new hook to a script that I write should do the job. Instead of dealing an even more complex and arcane procmail setup, I write a script that runs as the post-new hook. I'm finding the new flagging works correctly for simple mail drop from postfix, so the extra flag will work. I don't want to work off new because that's also useful for mail reading and other uses. The post-new hook can start by finding the emails that are tagged needsProcessing, parse and tag them appropriately, and remove the needsProcessing tag. It just needs to start with a query and be written to work on all the messages found, so it doesn't matter how many are new. At the moment it looks like a simple python script using the notmuch library binding may be easiest. If not, there are other tools to do the job. R Horn rjh...@alum.mit.edu ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 4/6] cli: intialize crypto structure in show and reply
On 05/18/2012 04:20 AM, Jani Nikula wrote:
We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)
i think we can agree that this is the right choice. We might even want
to discourage non-designated initializations entirely.
IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.
Agreed.
Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
I'm not sure it has to be this dramatic and all or nothing. For
example, it could be reasonable to explicitly initialize some subobjects
and not others. For example, the notmuch_crypto_t jamie is proposing
would effectively encode the default setting for the --verify and
--decrypt flags. I could see wanting to explicitly initialize those
default policy choices, even if they happen to be identical to the
implicit zeroing.
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?
i see your point here, which is why i'm not arguing that all subobjects
need to be explicitly initialized all the time.
It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.
yep, understood.
--dkg
signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2 0/5] cli: improve handling of crypto parameters and contexts
Here's a second version of this series [0]. I think I have addressed most of Austin's and Jani's comments. I've tried to make a compromise between Jani's desire to not see any unneeded initialization and my desire to have clarity and to make defaults that define behavior explicit. I ended up only initializing the variables that define default user behavior. I hope that's ok. jamie. [0] id:1337205359-2444-1-git-send-email-jroll...@finestructure.net ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument
This simplifies the interface considerably, getting rid of #ifdefs
along the way.
---
mime-node.c | 11 +++
notmuch-client.h | 14 +-
notmuch-reply.c |6 ++
notmuch-show.c |3 +--
4 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index a95bdab..183ff5d 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -61,12 +61,7 @@ _mime_node_context_free (mime_node_context_t *res)
notmuch_status_t
mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
- GMimeCryptoContext *cryptoctx,
-#else
- GMimeCipherContext *cryptoctx,
-#endif
- notmuch_bool_t decrypt, mime_node_t **root_out)
+ notmuch_crypto_t *crypto, mime_node_t **root_out)
{
const char *filename = notmuch_message_get_filename (message);
mime_node_context_t *mctx;
@@ -118,8 +113,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
-mctx-cryptoctx = cryptoctx;
-mctx-decrypt = decrypt;
+mctx-cryptoctx = crypto-gpgctx;
+mctx-decrypt = crypto-decrypt;
/* Create the root node */
root-part = GMIME_OBJECT (mctx-mime_message);
diff --git a/notmuch-client.h b/notmuch-client.h
index 2ad24cf..9892968 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -345,9 +345,10 @@ struct mime_node {
};
/* Construct a new MIME node pointing to the root message part of
- * message. If cryptoctx is non-NULL, it will be used to verify
- * signatures on any child parts. If decrypt is true, then cryptoctx
- * will additionally be used to decrypt any encrypted child parts.
+ * message. If crypto-gpgctx is non-NULL, it will be used to verify
+ * signatures on any child parts. If crypto-decrypt is true, then
+ * crypto.gpgctx will additionally be used to decrypt any encrypted
+ * child parts.
*
* Return value:
*
@@ -359,12 +360,7 @@ struct mime_node {
*/
notmuch_status_t
mime_node_open (const void *ctx, notmuch_message_t *message,
-#ifdef GMIME_ATLEAST_26
- GMimeCryptoContext *cryptoctx,
-#else
- GMimeCipherContext *cryptoctx,
-#endif
- notmuch_bool_t decrypt, mime_node_t **node_out);
+ notmuch_crypto_t *crypto, mime_node_t **node_out);
/* Return a new MIME node for the requested child part of parent.
* parent will be used as the talloc context for the returned child
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 8d608e1..34a906e 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -544,8 +544,7 @@ notmuch_reply_format_default(void *ctx,
g_object_unref (G_OBJECT (reply));
reply = NULL;
- if (mime_node_open (ctx, message, crypto-gpgctx, crypto-decrypt,
- root) == NOTMUCH_STATUS_SUCCESS) {
+ if (mime_node_open (ctx, message, crypto, root) ==
NOTMUCH_STATUS_SUCCESS) {
format_part_reply (root);
talloc_free (root);
}
@@ -574,8 +573,7 @@ notmuch_reply_format_json(void *ctx,
messages = notmuch_query_search_messages (query);
message = notmuch_messages_get (messages);
-if (mime_node_open (ctx, message, crypto-gpgctx, crypto-decrypt,
- node) != NOTMUCH_STATUS_SUCCESS)
+if (mime_node_open (ctx, message, crypto, node) != NOTMUCH_STATUS_SUCCESS)
return 1;
reply = create_reply_message (ctx, config, message, reply_all);
diff --git a/notmuch-show.c b/notmuch-show.c
index 7779c3e..66c74e2 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -810,8 +810,7 @@ show_message (void *ctx,
mime_node_t *root, *part;
notmuch_status_t status;
-status = mime_node_open (local, message, params-crypto.gpgctx,
-params-crypto.decrypt, root);
+status = mime_node_open (local, message, (params-crypto), root);
if (status)
goto DONE;
part = mime_node_seek_dfs (root, (params-part 0 ? 0 : params-part));
--
1.7.10
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters
The main point here is to keep track of the crypto stuff together in
one place. In notmuch-show the crypto struct is a sub structure of
the parameters struct. In notmuch-reply, which had been using a
notmuch_show_params_t to store the crypto parameters, we can now just
use the general crypto struct.
I slip in a name change of the crypto context itself to better reflect
what the context is specifically for: it's actually a GPG context,
which is a sub type of Crypto context. There are other types of
Crypto contexts (Pkcs7 in particular) so we want to be clear.
The following patches will use this to simplify some function
interfaces.
---
notmuch-client.h | 16 ++--
notmuch-reply.c | 36 +++-
notmuch-show.c | 30 ++
3 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/notmuch-client.h b/notmuch-client.h
index 19b7f01..2ad24cf 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -74,17 +74,21 @@ typedef struct notmuch_show_format {
const char *message_set_end;
} notmuch_show_format_t;
+typedef struct notmuch_crypto {
+#ifdef GMIME_ATLEAST_26
+GMimeCryptoContext* gpgctx;
+#else
+GMimeCipherContext* gpgctx;
+#endif
+notmuch_bool_t decrypt;
+} notmuch_crypto_t;
+
typedef struct notmuch_show_params {
notmuch_bool_t entire_thread;
notmuch_bool_t omit_excluded;
notmuch_bool_t raw;
int part;
-#ifdef GMIME_ATLEAST_26
-GMimeCryptoContext* cryptoctx;
-#else
-GMimeCipherContext* cryptoctx;
-#endif
-notmuch_bool_t decrypt;
+notmuch_crypto_t crypto;
} notmuch_show_params_t;
/* There's no point in continuing when we've detected that we've done
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7184a5d..8d608e1 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -515,7 +515,7 @@ static int
notmuch_reply_format_default(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
-notmuch_show_params_t *params,
+notmuch_crypto_t *crypto,
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -544,7 +544,7 @@ notmuch_reply_format_default(void *ctx,
g_object_unref (G_OBJECT (reply));
reply = NULL;
- if (mime_node_open (ctx, message, params-cryptoctx, params-decrypt,
+ if (mime_node_open (ctx, message, crypto-gpgctx, crypto-decrypt,
root) == NOTMUCH_STATUS_SUCCESS) {
format_part_reply (root);
talloc_free (root);
@@ -559,7 +559,7 @@ static int
notmuch_reply_format_json(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
- notmuch_show_params_t *params,
+ notmuch_crypto_t *crypto,
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -574,7 +574,7 @@ notmuch_reply_format_json(void *ctx,
messages = notmuch_query_search_messages (query);
message = notmuch_messages_get (messages);
-if (mime_node_open (ctx, message, params-cryptoctx, params-decrypt,
+if (mime_node_open (ctx, message, crypto-gpgctx, crypto-decrypt,
node) != NOTMUCH_STATUS_SUCCESS)
return 1;
@@ -605,7 +605,7 @@ static int
notmuch_reply_format_headers_only(void *ctx,
notmuch_config_t *config,
notmuch_query_t *query,
- unused (notmuch_show_params_t *params),
+ unused (notmuch_crypto_t *crypto),
notmuch_bool_t reply_all)
{
GMimeMessage *reply;
@@ -674,8 +674,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
notmuch_query_t *query;
char *query_string;
int opt_index, ret = 0;
-int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_show_params_t *params, notmuch_bool_t
reply_all);
-notmuch_show_params_t params = { .part = -1 };
+int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
+notmuch_crypto_t crypto = {
+ .decrypt = FALSE
+};
int format = FORMAT_DEFAULT;
int reply_all = TRUE;
@@ -689,7 +691,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
(notmuch_keyword_t []){ { all, TRUE },
{ sender, FALSE },
{ 0, 0 } } },
- { NOTMUCH_OPT_BOOLEAN, params.decrypt, decrypt, 'd', 0 },
+ { NOTMUCH_OPT_BOOLEAN, crypto.decrypt, decrypt, 'd', 0 },
{ 0, 0, 0, 0, 0 }
};
@@ -706,18 +708,18 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
else
reply_format_func = notmuch_reply_format_default;
-if
[PATCH v2 4/5] cli: new crypto verify flag to handle verification
Use this flag rather than depend on the existence of an initialized
gpgctx, to determine whether we should verify a multipart/signed. We
will be moving to create the ctx lazily, so we don't want to depend on
it being previously initialized if it's not needed.
---
mime-node.c |5 ++---
notmuch-client.h |8
notmuch-reply.c |1 +
notmuch-show.c | 14 +++---
4 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index 3dda900..3adbe5a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -183,8 +183,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
}
/* Handle PGP/MIME parts */
-if (GMIME_IS_MULTIPART_ENCRYPTED (part)
-node-ctx-crypto-gpgctx node-ctx-crypto-decrypt) {
+if (GMIME_IS_MULTIPART_ENCRYPTED (part) node-ctx-crypto-decrypt) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 4, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/encrypted
@@ -218,7 +217,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
(err ? err-message : no error explanation given));
}
}
-} else if (GMIME_IS_MULTIPART_SIGNED (part) node-ctx-crypto-gpgctx) {
+} else if (GMIME_IS_MULTIPART_SIGNED (part) node-ctx-crypto-verify) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 5, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/signed message
diff --git a/notmuch-client.h b/notmuch-client.h
index 9892968..c671c13 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -80,6 +80,7 @@ typedef struct notmuch_crypto {
#else
GMimeCipherContext* gpgctx;
#endif
+notmuch_bool_t verify;
notmuch_bool_t decrypt;
} notmuch_crypto_t;
@@ -345,10 +346,9 @@ struct mime_node {
};
/* Construct a new MIME node pointing to the root message part of
- * message. If crypto-gpgctx is non-NULL, it will be used to verify
- * signatures on any child parts. If crypto-decrypt is true, then
- * crypto.gpgctx will additionally be used to decrypt any encrypted
- * child parts.
+ * message. If crypto-verify is true, signed child parts will be
+ * verified. If crypto-decrypt is true, encrypted child parts will be
+ * decrypted.
*
* Return value:
*
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 34a906e..345be76 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -674,6 +674,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
int opt_index, ret = 0;
int (*reply_format_func)(void *ctx, notmuch_config_t *config,
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
notmuch_crypto_t crypto = {
+ .verify = FALSE,
.decrypt = FALSE
};
int format = FORMAT_DEFAULT;
diff --git a/notmuch-show.c b/notmuch-show.c
index 66c74e2..f4ee038 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -987,11 +987,11 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
.part = -1,
.omit_excluded = TRUE,
.crypto = {
+ .verify = FALSE,
.decrypt = FALSE
}
};
int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
-notmuch_bool_t verify = FALSE;
int exclude = EXCLUDE_TRUE;
notmuch_opt_desc_t options[] = {
@@ -1008,7 +1008,7 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
{ NOTMUCH_OPT_INT, params.part, part, 'p', 0 },
{ NOTMUCH_OPT_BOOLEAN, params.entire_thread, entire-thread, 't', 0 },
{ NOTMUCH_OPT_BOOLEAN, params.crypto.decrypt, decrypt, 'd', 0 },
- { NOTMUCH_OPT_BOOLEAN, verify, verify, 'v', 0 },
+ { NOTMUCH_OPT_BOOLEAN, params.crypto.verify, verify, 'v', 0 },
{ 0, 0, 0, 0, 0 }
};
@@ -1018,6 +1018,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
return 1;
}
+/* decryption implies verification */
+if (params.crypto.decrypt)
+ params.crypto.verify = TRUE;
+
if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {
/* if part was requested and format was not specified, use format=raw */
if (params.part = 0)
@@ -1052,7 +1056,7 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
break;
}
-if (params.crypto.decrypt || verify) {
+if (params.crypto.decrypt || params.crypto.verify) {
#ifdef GMIME_ATLEAST_26
/* TODO: GMimePasswordRequestFunc */
params.crypto.gpgctx = g_mime_gpg_context_new (NULL, gpg);
@@ -1063,6 +1067,10 @@ notmuch_show_command (void *ctx, unused (int argc),
unused (char *argv[]))
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
[PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
This simplifies some more interfaces and gets rid of another #ifdef.
---
mime-node.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index 183ff5d..3dda900 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -33,12 +33,7 @@ typedef struct mime_node_context {
GMimeMessage *mime_message;
/* Context provided by the caller. */
-#ifdef GMIME_ATLEAST_26
-GMimeCryptoContext *cryptoctx;
-#else
-GMimeCipherContext *cryptoctx;
-#endif
-notmuch_bool_t decrypt;
+notmuch_crypto_t *crypto;
} mime_node_context_t;
static int
@@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
goto DONE;
}
-mctx-cryptoctx = crypto-gpgctx;
-mctx-decrypt = crypto-decrypt;
+mctx-crypto = crypto;
/* Create the root node */
root-part = GMIME_OBJECT (mctx-mime_message);
@@ -190,7 +184,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
/* Handle PGP/MIME parts */
if (GMIME_IS_MULTIPART_ENCRYPTED (part)
-node-ctx-cryptoctx node-ctx-decrypt) {
+node-ctx-crypto-gpgctx node-ctx-crypto-decrypt) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 4, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/encrypted
@@ -203,10 +197,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
#ifdef GMIME_ATLEAST_26
GMimeDecryptResult *decrypt_result = NULL;
node-decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node-ctx-cryptoctx, decrypt_result, err);
+ (encrypteddata, node-ctx-crypto-gpgctx, decrypt_result,
err);
#else
node-decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node-ctx-cryptoctx, err);
+ (encrypteddata, node-ctx-crypto-gpgctx, err);
#endif
if (node-decrypted_child) {
node-decrypt_success = node-verify_attempted = TRUE;
@@ -224,7 +218,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
(err ? err-message : no error explanation given));
}
}
-} else if (GMIME_IS_MULTIPART_SIGNED (part) node-ctx-cryptoctx) {
+} else if (GMIME_IS_MULTIPART_SIGNED (part) node-ctx-crypto-gpgctx) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 5, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/signed message
@@ -233,7 +227,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
} else {
#ifdef GMIME_ATLEAST_26
node-sig_list = g_mime_multipart_signed_verify
- (GMIME_MULTIPART_SIGNED (part), node-ctx-cryptoctx, err);
+ (GMIME_MULTIPART_SIGNED (part), node-ctx-crypto-gpgctx,
err);
node-verify_attempted = TRUE;
if (!node-sig_list)
@@ -249,7 +243,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
* 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), node-ctx-cryptoctx, err);
+ (GMIME_MULTIPART_SIGNED (part), node-ctx-crypto.gpgctx, err);
node-verify_attempted = TRUE;
node-sig_validity = sig_validity;
if (sig_validity) {
--
1.7.10
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
Quoth Jameson Graef Rollins on May 18 at 10:32 am:
This simplifies some more interfaces and gets rid of another #ifdef.
---
mime-node.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/mime-node.c b/mime-node.c
index 183ff5d..3dda900 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -33,12 +33,7 @@ typedef struct mime_node_context {
GMimeMessage *mime_message;
/* Context provided by the caller. */
-#ifdef GMIME_ATLEAST_26
-GMimeCryptoContext *cryptoctx;
-#else
-GMimeCipherContext *cryptoctx;
-#endif
-notmuch_bool_t decrypt;
+notmuch_crypto_t *crypto;
} mime_node_context_t;
static int
@@ -113,8 +108,7 @@ mime_node_open (const void *ctx, notmuch_message_t
*message,
goto DONE;
}
-mctx-cryptoctx = crypto-gpgctx;
-mctx-decrypt = crypto-decrypt;
+mctx-crypto = crypto;
/* Create the root node */
root-part = GMIME_OBJECT (mctx-mime_message);
@@ -190,7 +184,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
/* Handle PGP/MIME parts */
if (GMIME_IS_MULTIPART_ENCRYPTED (part)
- node-ctx-cryptoctx node-ctx-decrypt) {
+ node-ctx-crypto-gpgctx node-ctx-crypto-decrypt) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 4, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/encrypted
@@ -203,10 +197,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject
*part)
#ifdef GMIME_ATLEAST_26
GMimeDecryptResult *decrypt_result = NULL;
node-decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node-ctx-cryptoctx, decrypt_result, err);
+ (encrypteddata, node-ctx-crypto-gpgctx, decrypt_result,
err);
#else
node-decrypted_child = g_mime_multipart_encrypted_decrypt
- (encrypteddata, node-ctx-cryptoctx, err);
+ (encrypteddata, node-ctx-crypto-gpgctx, err);
#endif
if (node-decrypted_child) {
node-decrypt_success = node-verify_attempted = TRUE;
@@ -224,7 +218,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
(err ? err-message : no error explanation given));
}
}
-} else if (GMIME_IS_MULTIPART_SIGNED (part) node-ctx-cryptoctx) {
+} else if (GMIME_IS_MULTIPART_SIGNED (part)
node-ctx-crypto-gpgctx) {
if (node-nchildren != 2) {
/* this violates RFC 3156 section 5, so we won't bother with it. */
fprintf (stderr, Error: %d part(s) for a multipart/signed message
@@ -233,7 +227,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
} else {
#ifdef GMIME_ATLEAST_26
node-sig_list = g_mime_multipart_signed_verify
- (GMIME_MULTIPART_SIGNED (part), node-ctx-cryptoctx, err);
+ (GMIME_MULTIPART_SIGNED (part), node-ctx-crypto-gpgctx,
err);
node-verify_attempted = TRUE;
if (!node-sig_list)
@@ -249,7 +243,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
* 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), node-ctx-cryptoctx, err);
+ (GMIME_MULTIPART_SIGNED (part), node-ctx-crypto.gpgctx, err);
This should be crypto-gpgctx.
node-verify_attempted = TRUE;
node-sig_validity = sig_validity;
if (sig_validity) {
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t
On Fri, May 18 2012, Austin Clements amdra...@mit.edu wrote: -(GMIME_MULTIPART_SIGNED (part), node-ctx-cryptoctx, err); +(GMIME_MULTIPART_SIGNED (part), node-ctx-crypto.gpgctx, err); This should be crypto-gpgctx. Weird. Good catch, Austin! But I must admit I'm thoroughly confused. I tested every stage of this series right before I sent, and sure enough this patch compiles cleanly and all tests pass. Can anyone explain why? I don't understand. I'll send a new version of this one patch now anyway. jamie. pgp7lJq11gtEJ.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed
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
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
