* [PATCH v2 0/5] cli: improve handling of crypto parameters and contexts @ 2012-05-18 17:32 Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters Jameson Graef Rollins 0 siblings, 1 reply; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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@finestructure.net" ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters 2012-05-18 17:32 [PATCH v2 0/5] cli: improve handling of crypto parameters and contexts Jameson Graef Rollins @ 2012-05-18 17:32 ` Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument Jameson Graef Rollins 0 siblings, 1 reply; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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, ¶ms.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 (params.decrypt) { + if (crypto.decrypt) { #ifdef GMIME_ATLEAST_26 /* TODO: GMimePasswordRequestFunc */ - params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg"); + crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg"); #else GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); - params.cryptoctx = g_mime_gpg_context_new (session, "gpg"); + crypto.gpgctx = g_mime_gpg_context_new (session, "gpg"); #endif - if (params.cryptoctx) { - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE); + if (crypto.gpgctx) { + g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE); } else { - params.decrypt = FALSE; + crypto.decrypt = FALSE; fprintf (stderr, "Failed to construct gpg context.\n"); } #ifndef GMIME_ATLEAST_26 @@ -750,14 +752,14 @@ notmuch_reply_command (void *ctx, int argc, char *argv[]) return 1; } - if (reply_format_func (ctx, config, query, ¶ms, reply_all) != 0) + if (reply_format_func (ctx, config, query, &crypto, reply_all) != 0) return 1; notmuch_query_destroy (query); notmuch_database_destroy (notmuch); - if (params.cryptoctx) - g_object_unref(params.cryptoctx); + if (crypto.gpgctx) + g_object_unref(crypto.gpgctx); return ret; } diff --git a/notmuch-show.c b/notmuch-show.c index 95427d4..7779c3e 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -810,8 +810,8 @@ show_message (void *ctx, mime_node_t *root, *part; notmuch_status_t status; - status = mime_node_open (local, message, params->cryptoctx, - params->decrypt, &root); + status = mime_node_open (local, message, params->crypto.gpgctx, + params->crypto.decrypt, &root); if (status) goto DONE; part = mime_node_seek_dfs (root, (params->part < 0 ? 0 : params->part)); @@ -984,7 +984,13 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) char *query_string; int opt_index, ret; const notmuch_show_format_t *format = &format_text; - notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE }; + notmuch_show_params_t params = { + .part = -1, + .omit_excluded = TRUE, + .crypto = { + .decrypt = FALSE + } + }; int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED; notmuch_bool_t verify = FALSE; int exclude = EXCLUDE_TRUE; @@ -1002,7 +1008,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) { 0, 0 } } }, { NOTMUCH_OPT_INT, ¶ms.part, "part", 'p', 0 }, { NOTMUCH_OPT_BOOLEAN, ¶ms.entire_thread, "entire-thread", 't', 0 }, - { NOTMUCH_OPT_BOOLEAN, ¶ms.decrypt, "decrypt", 'd', 0 }, + { NOTMUCH_OPT_BOOLEAN, ¶ms.crypto.decrypt, "decrypt", 'd', 0 }, { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 }, { 0, 0, 0, 0, 0 } }; @@ -1047,18 +1053,18 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) break; } - if (params.decrypt || verify) { + if (params.crypto.decrypt || verify) { #ifdef GMIME_ATLEAST_26 /* TODO: GMimePasswordRequestFunc */ - params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg"); + params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg"); #else GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); - params.cryptoctx = g_mime_gpg_context_new (session, "gpg"); + params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg"); #endif - if (params.cryptoctx) { - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE); + if (params.crypto.gpgctx) { + g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE); } else { - params.decrypt = FALSE; + params.crypto.decrypt = FALSE; fprintf (stderr, "Failed to construct gpg context.\n"); } #ifndef GMIME_ATLEAST_26 @@ -1118,8 +1124,8 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) notmuch_query_destroy (query); notmuch_database_destroy (notmuch); - if (params.cryptoctx) - g_object_unref(params.cryptoctx); + if (params.crypto.gpgctx) + g_object_unref(params.crypto.gpgctx); return ret; } -- 1.7.10 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument 2012-05-18 17:32 ` [PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters Jameson Graef Rollins @ 2012-05-18 17:32 ` Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Jameson Graef Rollins 0 siblings, 1 reply; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t 2012-05-18 17:32 ` [PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument Jameson Graef Rollins @ 2012-05-18 17:32 ` Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jameson Graef Rollins 2012-05-18 19:09 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Austin Clements 0 siblings, 2 replies; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] cli: new crypto verify flag to handle verification 2012-05-18 17:32 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Jameson Graef Rollins @ 2012-05-18 17:32 ` Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed Jameson Graef Rollins 2012-05-19 11:26 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jani Nikula 2012-05-18 19:09 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Austin Clements 1 sibling, 2 replies; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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, ¶ms.part, "part", 'p', 0 }, { NOTMUCH_OPT_BOOLEAN, ¶ms.entire_thread, "entire-thread", 't', 0 }, { NOTMUCH_OPT_BOOLEAN, ¶ms.crypto.decrypt, "decrypt", 'd', 0 }, - { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 }, + { NOTMUCH_OPT_BOOLEAN, ¶ms.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 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"); } -- 1.7.10 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed 2012-05-18 17:32 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jameson Graef Rollins @ 2012-05-18 17:32 ` Jameson Graef Rollins 2012-05-18 19:21 ` Austin Clements 2012-05-19 11:26 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jani Nikula 1 sibling, 1 reply; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 17:32 UTC (permalink / raw) To: Notmuch Mail 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed 2012-05-18 17:32 ` [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed Jameson Graef Rollins @ 2012-05-18 19:21 ` Austin Clements 2012-05-18 19:45 ` Jameson Graef Rollins 0 siblings, 1 reply; 14+ messages in thread From: Austin Clements @ 2012-05-18 19:21 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on May 18 at 10:32 am: > Move the creation of the crypto ctx into mime-node.c and create it > only when needed. This removes code duplication from notmuch-show and > notmuch-reply, and should speed up these functions considerably if the > crypto flags are provided but the messages don't have any > cryptographic parts. > --- > mime-node.c | 25 +++++++++++++++++++++++++ > notmuch-client.h | 3 ++- > notmuch-reply.c | 19 ------------------- > notmuch-show.c | 23 ----------------------- > 4 files changed, 27 insertions(+), 43 deletions(-) > > diff --git a/mime-node.c b/mime-node.c > index 3adbe5a..592e0b6 100644 > --- a/mime-node.c > +++ b/mime-node.c > @@ -182,6 +182,31 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part) > return NULL; > } > > + /* Lazily create the gpgctx if it's needed and hasn't been initialized yet */ > + if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part)) > + && (node->ctx->crypto->verify || node->ctx->crypto->decrypt)) { > + if (!node->ctx->crypto->gpgctx) { These if conditions could be combined, like if ((GMIME_IS_MULTIPART_ENCRYPTED (part) || GMIME_IS_MULTIPART_SIGNED (part)) && (node->ctx->crypto->verify || node->ctx->crypto->decrypt) && !node->ctx->crypto->gpgctx) { When I see two nested 'if's like this, I expect there to be an else part to the inner if or something after the inner if (why else would it be separate?) and then I wind up matching braces when I don't see anything. Also, one 'if' would save a level of indentation. > +#ifdef GMIME_ATLEAST_26 > + /* TODO: GMimePasswordRequestFunc */ > + node->ctx->crypto->gpgctx = g_mime_gpg_context_new (NULL, "gpg"); > +#else > + GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); > + node->ctx->crypto->gpgctx = g_mime_gpg_context_new (session, "gpg"); > + g_object_unref (session); > +#endif > + if (node->ctx->crypto->gpgctx) { > + g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) node->ctx->crypto->gpgctx, FALSE); > + } else { > + /* If we fail to create the gpgctx set the verify and > + * decrypt flags to FALSE so we don't try to do any > + * further verification or decryption */ > + node->ctx->crypto->verify = FALSE; > + node->ctx->crypto->decrypt = FALSE; > + fprintf (stderr, "Failed to construct gpg context.\n"); > + } > + } > + } > + > /* Handle PGP/MIME parts */ > if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto->decrypt) { > if (node->nchildren != 2) { > diff --git a/notmuch-client.h b/notmuch-client.h > index c671c13..c79eaa9 100644 > --- a/notmuch-client.h > +++ b/notmuch-client.h > @@ -348,7 +348,8 @@ struct mime_node { > /* Construct a new MIME node pointing to the root message part of > * message. If crypto->verify is true, signed child parts will be > * verified. If crypto->decrypt is true, encrypted child parts will be > - * decrypted. > + * decrypted. The GPG context crypto->gpgctx does not need to be > + * pre-initialized as it will be initialized lazily as needed. Perhaps "If crypto->gpgctx is NULL, it will be lazily initialized."? The variable does have to be "initialized", in the sense that it can't be uninitialized data. It's slightly awkward that it's the caller's responsibility to free this lazily constructed object. That should probably be documented. We could more carefully reference count it, but I think that would actually be worse because the reference count would probably bounce through zero frequently. > * > * Return value: > * > diff --git a/notmuch-reply.c b/notmuch-reply.c > index 345be76..1a61aa7 100644 > --- a/notmuch-reply.c > +++ b/notmuch-reply.c > @@ -707,25 +707,6 @@ notmuch_reply_command (void *ctx, int argc, char *argv[]) > else > reply_format_func = notmuch_reply_format_default; > > - if (crypto.decrypt) { > -#ifdef GMIME_ATLEAST_26 > - /* TODO: GMimePasswordRequestFunc */ > - crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg"); > -#else > - GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); > - crypto.gpgctx = g_mime_gpg_context_new (session, "gpg"); > -#endif > - if (crypto.gpgctx) { > - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto.gpgctx, FALSE); > - } else { > - crypto.decrypt = FALSE; > - fprintf (stderr, "Failed to construct gpg context.\n"); > - } > -#ifndef GMIME_ATLEAST_26 > - g_object_unref (session); > -#endif > - } > - > config = notmuch_config_open (ctx, NULL, NULL); > if (config == NULL) > return 1; > diff --git a/notmuch-show.c b/notmuch-show.c > index f4ee038..5f785d0 100644 > --- a/notmuch-show.c > +++ b/notmuch-show.c > @@ -1056,29 +1056,6 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[])) > break; > } > > - if (params.crypto.decrypt || params.crypto.verify) { > -#ifdef GMIME_ATLEAST_26 > - /* TODO: GMimePasswordRequestFunc */ > - params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg"); > -#else > - GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); > - params.crypto.gpgctx = g_mime_gpg_context_new (session, "gpg"); > -#endif > - if (params.crypto.gpgctx) { > - g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE); > - } else { > - /* If we fail to create the gpgctx set the verify and > - * decrypt flags to FALSE so we don't try to do any > - * further verification or decryption */ > - params.crypto.verify = FALSE; > - params.crypto.decrypt = FALSE; > - fprintf (stderr, "Failed to construct gpg context.\n"); > - } > -#ifndef GMIME_ATLEAST_26 > - g_object_unref (session); > -#endif > - } > - > config = notmuch_config_open (ctx, NULL, NULL); > if (config == NULL) > return 1; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed 2012-05-18 19:21 ` Austin Clements @ 2012-05-18 19:45 ` Jameson Graef Rollins 2012-05-18 20:37 ` Daniel Kahn Gillmor 0 siblings, 1 reply; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 19:45 UTC (permalink / raw) To: Austin Clements; +Cc: Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 2342 bytes --] On Fri, May 18 2012, Austin Clements <amdragon@MIT.EDU> 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. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed 2012-05-18 19:45 ` Jameson Graef Rollins @ 2012-05-18 20:37 ` Daniel Kahn Gillmor 2012-05-18 20:42 ` Jameson Graef Rollins 0 siblings, 1 reply; 14+ messages in thread From: Daniel Kahn Gillmor @ 2012-05-18 20:37 UTC (permalink / raw) To: Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 1754 bytes --] On 05/18/2012 03:45 PM, Jameson Graef Rollins wrote: > On Fri, May 18 2012, Austin Clements <amdragon@MIT.EDU> 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 1030 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed 2012-05-18 20:37 ` Daniel Kahn Gillmor @ 2012-05-18 20:42 ` Jameson Graef Rollins 0 siblings, 0 replies; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 20:42 UTC (permalink / raw) To: Daniel Kahn Gillmor, Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 1031 bytes --] On Fri, May 18 2012, Daniel Kahn Gillmor <dkg@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. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/5] cli: new crypto verify flag to handle verification 2012-05-18 17:32 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed Jameson Graef Rollins @ 2012-05-19 11:26 ` Jani Nikula 2012-05-19 16:28 ` Jameson Graef Rollins 1 sibling, 1 reply; 14+ messages in thread From: Jani Nikula @ 2012-05-19 11:26 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Fri, 18 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > 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, ¶ms.part, "part", 'p', 0 }, > { NOTMUCH_OPT_BOOLEAN, ¶ms.entire_thread, "entire-thread", 't', 0 }, > { NOTMUCH_OPT_BOOLEAN, ¶ms.crypto.decrypt, "decrypt", 'd', 0 }, > - { NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 }, > + { NOTMUCH_OPT_BOOLEAN, ¶ms.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; This does not change existing behaviour, only makes it more obvious (which is good), but this seems to be missing from the man page. I presume technically decryption doesn't have to imply verification, but it's probably a good thing. It should be documented, but does not have to be a part of this series. Thanks for working on this. The series looks good to me (apart from the comments already made by Austin), and the compromises after our debate reasonable. BR, Jani. > + > 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 > + * further verification or decryption */ > + params.crypto.verify = FALSE; > params.crypto.decrypt = FALSE; > fprintf (stderr, "Failed to construct gpg context.\n"); > } > -- > 1.7.10 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/5] cli: new crypto verify flag to handle verification 2012-05-19 11:26 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jani Nikula @ 2012-05-19 16:28 ` Jameson Graef Rollins 0 siblings, 0 replies; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-19 16:28 UTC (permalink / raw) To: Jani Nikula, Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 702 bytes --] On Sat, May 19 2012, Jani Nikula <jani@nikula.org> wrote: >> + /* decryption implies verification */ >> + if (params.crypto.decrypt) >> + params.crypto.verify = TRUE; > > This does not change existing behaviour, only makes it more obvious > (which is good), but this seems to be missing from the man page. I > presume technically decryption doesn't have to imply verification, but > it's probably a good thing. It should be documented, but does not have > to be a part of this series. Well that's very strange. I was sure I mentioned that in the man page back when I originally added decryption support. But apparently I didn't. I'll submit a separate patch to fix the documentation. jamie. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t 2012-05-18 17:32 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jameson Graef Rollins @ 2012-05-18 19:09 ` Austin Clements 2012-05-18 19:17 ` Jameson Graef Rollins 1 sibling, 1 reply; 14+ messages in thread From: Austin Clements @ 2012-05-18 19:09 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail 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) { ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t 2012-05-18 19:09 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Austin Clements @ 2012-05-18 19:17 ` Jameson Graef Rollins 0 siblings, 0 replies; 14+ messages in thread From: Jameson Graef Rollins @ 2012-05-18 19:17 UTC (permalink / raw) To: Austin Clements; +Cc: Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 537 bytes --] On Fri, May 18 2012, Austin Clements <amdragon@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. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-05-19 16:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-18 17:32 [PATCH v2 0/5] cli: improve handling of crypto parameters and contexts Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 1/5] cli: new crypto structure to store crypto contexts and parameters Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 2/5] cli: modify mime_node_open to take crypto struct as argument Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jameson Graef Rollins 2012-05-18 17:32 ` [PATCH v2 5/5] cli: lazily create the crypto gpg context only when needed Jameson Graef Rollins 2012-05-18 19:21 ` Austin Clements 2012-05-18 19:45 ` Jameson Graef Rollins 2012-05-18 20:37 ` Daniel Kahn Gillmor 2012-05-18 20:42 ` Jameson Graef Rollins 2012-05-19 11:26 ` [PATCH v2 4/5] cli: new crypto verify flag to handle verification Jani Nikula 2012-05-19 16:28 ` Jameson Graef Rollins 2012-05-18 19:09 ` [PATCH v2 3/5] cli: modify mime_node_context to use the new notmuch_crypto_t Austin Clements 2012-05-18 19:17 ` Jameson Graef Rollins
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).