Re: [PATCH v3 3/4] crypto: keytoc: Split env-provided full keyspec on spaces

[Marco Felsch <m.felsch@pengutronix.de>]

On 26-03-16, Jonas Rebmann wrote:
> keytoc/CONFIG_CRYPTO_PUBLIC_KEYS can work with a complete keyspec
> provided by an environment variable as opposed to providing single URIs.
> This would be a very useful feature if it could also provide any number
> of keys. Kconfig however provides keytoc with regular keyspecs already
> split at spaces so without furhter measures, the env variable can only
> be expanded into a single key.
> 
> If a complete argument is provided via __ENV, split it at any space
> character that is not escaped with a backslash in front of it.  An
> actual backslash in a path needs to be escape with another backslash.

I find your commit message a bit hard to read to be honest. I had to
read it twice to see what you actually want to add. So it could be
shorten to:

Add support to provide multiple keyspec entries via a single environemnt
(__ENV__) variable.

Furthermore please adapt the Kconfig help message to keep it in sync.

> 
> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
> ---
>  scripts/keytoc.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/scripts/keytoc.c b/scripts/keytoc.c
> index 1b99393fdc..c451ee081a 100644
> --- a/scripts/keytoc.c
> +++ b/scripts/keytoc.c
> @@ -10,6 +10,8 @@
>  
>  #pragma GCC diagnostic ignored "-Wdeprecated-declarations" /* ENGINE deprecated in OpenSSL 3.0 */
>  
> +#include "include/string_util.h"
> +
>  #include <stdio.h>
>  #include <string.h>
>  #include <time.h>
> @@ -787,6 +789,11 @@ static bool parse_info(char *p, struct keyinfo *out)
>  
>  static bool parse_keyspec(const char *keyspec, struct keyinfo *out)
>  {
> +	if (!strncmp(keyspec, "pkcs11:", 7)) { /* legacy format of pkcs11 URI */
> +		out->path = strdup(keyspec);
> +		return true;
> +	}
> +
>  	char *sep, *spec;
>  
>  	spec = strdup(keyspec);
> @@ -817,7 +824,7 @@ int main(int argc, char *argv[])
>  {
>  	int argi, opt, ret;
>  	char *outfile = NULL;
> -	int keycount;
> +	size_t keycount, num_positionals;

Why size_t?

>  	struct keyinfo *keylist;
>  
>  	outfilep = stdout;
> @@ -853,22 +860,44 @@ int main(int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	keycount = argc - optind;
> -	keylist = calloc(sizeof(struct keyinfo), keycount);
>  
> -	for (argi = 0; argi < keycount; argi++) {
> -		const char *keyspec = try_resolve_env(argv[optind + argi]);
> -		struct keyinfo *info = &keylist[argi];
> +	num_positionals = argc - optind;
> +	keycount = num_positionals;
>  
> -		if (!keyspec)
> -			exit(1);
> +	keylist = calloc(keycount, sizeof(*keylist));
> +

No newline required. Furthermore I would prefer a list now since this
would be much easier to read and to handle (without the below
reallocarray).

> +	if (!keylist)
> +		enomem_exit("push");
> +
> +	int listi = 0;
> +
> +	for (argi = 0; argi < num_positionals; argi++) {
> +		char *arg = strdup(argv[optind + argi]);
> +		char *resolved = try_resolve_env(arg);
		^
Why do you drop the 'const' here? Furthermore, why do you strdup()?

> -		if (!strncmp(keyspec, "pkcs11:", 7)) { // legacy format of pkcs11 URI
> -			info->path = strdup(keyspec);
> +		if (arg == resolved) {
> +			keylist[listi].path = arg;
> +			listi++;
>  		} else {
> -			if (!parse_keyspec(keyspec, info)) {
> -				fprintf(stderr, "invalid keyspec %i: %s\n", optind, keyspec);
> -				exit(1);
> +			char *keyspecs = strdup(resolved);
> +			char *keyspec;
> +
> +			/* Keyspec given as env Variable,
> +			 * remove it and add an arbitrary number of keyspecs from its contents
> +			 */
> +			keycount--;
> +			while ((keyspec = strsep_unescaped(&keyspecs, " ", NULL))) {
> +				keycount++;
> +				keylist = reallocarray(keylist, keycount, sizeof(*keylist));
> +				if (!keylist)
> +					enomem_exit("realloc keylist");
> +				bzero(keylist + (keycount - 1), sizeof(*keylist));
> +				if (!parse_keyspec(keyspec, &keylist[listi])) {
> +					fprintf(stderr, "invalid keyspec %i: %s\n", optind,
> +						keyspec);
> +					exit(1);
> +				}
> +				listi++;
>  			}

Did you tested that all the other usecases are not broken by this
change?

Regards,
  Marco

>  		}
>  	}
> 
> -- 
> 2.53.0.308.g50d063e335
> 
> 
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |