From: Jani Nikula <jani@nikula.org>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 1/6] cli: new crypto structure to store crypto contexts
	and parameters
In-Reply-To: <1337205359-2444-2-git-send-email-jrollins@finestructure.net>
References: <1337205359-2444-1-git-send-email-jrollins@finestructure.net>
	<1337205359-2444-2-git-send-email-jrollins@finestructure.net>
User-Agent: Notmuch/0.13+13~gc259b9a (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Thu, 17 May 2012 10:36:08 +0300
Message-ID: <87ehqji7iv.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Thu, 17 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> 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.

Looks good. My only (potentially unwarranted) worry is dropping the use
of notmuch_show_params_t in reply. This diverges the show/reply code,
making any future unification of them slightly harder. And if reply ever
needs params, we'll need to bring it back. But perhaps I worry too
much. :)

BR,
Jani.


>
> 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  |   34 +++++++++++++++++-----------------
>  notmuch-show.c   |   22 +++++++++++-----------
>  3 files changed, 38 insertions(+), 34 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..ed87899 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,8 @@ 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 +689,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 +706,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 +750,14 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (reply_format_func (ctx, config, query, &params, 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..d254179 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));
> @@ -1002,7 +1002,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>                                    { 0, 0 } } },
>  	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
> -	{ NOTMUCH_OPT_BOOLEAN, &params.decrypt, "decrypt", 'd', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
> @@ -1047,18 +1047,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 +1118,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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch