Web lists-archives.com

Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm




Jeff King <peff@xxxxxxxx> writes:

>> @@ -16,13 +16,18 @@ struct gpg_format_data {
>>  
>>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>> +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
>>  
>> -enum gpgformats { PGP_FMT };
>> +enum gpgformats { PGP_FMT, X509_FMT };
>>  struct gpg_format_data gpg_formats[] = {
>>  	{ .format = "openpgp", .program = "gpg",
>>  	  .extra_args_verify = { "--keyid-format=long" },
>>  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
>>  	},
>> +	{ .format = "x509", .program = "gpgsm",
>> +	  .extra_args_verify = { NULL },
>> +	  .sigs = { X509_SIGNATURE, NULL }
>> +	},
>
> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> outside of this array (as I hope there wouldn't be after this series),
> would it make more sense to just include the literals inline in the
> array definition? That's one less layer of indirection when somebody is
> reading the code.

It is good design-sense to shoot for fewer levels of indirection,
but I suspect that "'const char **' instead of maximally-sized fixed
array of strings" would require a named array and constants like
this:

	static const char *gpg_verify_args[] = {
		"--verify",
		"--status-fd=1",
		"--keyid-format=long",
		NULL
	};
	static const char *gpg_sigs[] = {
		"-----BEGIN PGP SIGNATURE-----",
		"-----BEGIN PGP MESSAGE-----",
		NULL
	};

	struct gpg_format {
		const char *name;
		const char *program;
		const char * const *verify_args;
		const char * const *sigs;
	} gpg_format[] = {
		{
			.name = "openpgp",
			.program = "gpg',
			.verify_args = gpg_verify_args,
			.sigs = gpg_sigs,
		},
		{
			...
		},
	};

so we may end up having the same number of levels of indirection
anyway in the long-term final form.

As readers may be able to read from the above, I also have a
suspicion that it is a mistake to pretend that "--verify" etc.,
which merely happen to be common across the variants the series
covers, will stay forever to be common across _all_ variants and
that is why the field no longer is called "extra" args but is meant
to contain the full args.