From: Henning Schild <henning.schild@siemens.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
"Junio C Hamano" <gitster@pobox.com>,
"Martin Ågren" <martin.agren@gmail.com>,
"Ben Toews" <mastahyeti@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>,
"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats
Date: Fri, 13 Jul 2018 10:41:26 +0200 [thread overview]
Message-ID: <20180713104126.06017661@md1pvb1c.ad001.siemens.net> (raw)
In-Reply-To: <20180710162332.GD23624@sigill.intra.peff.net>
Am Tue, 10 Jul 2018 12:23:32 -0400
schrieb Jeff King <peff@peff.net>:
> On Tue, Jul 10, 2018 at 10:52:27AM +0200, Henning Schild wrote:
>
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "openpgp". This commit
> > prepares for the introduction of more formats, that might use other
> > programs and match other signatures.
>
> Great, this looks like a good incremental step.
>
> > static char *configured_signing_key;
> > -static const char *gpg_format = "openpgp";
> > -static const char *gpg_program = "gpg";
> > +struct gpg_format_data {
> > + const char *format;
> > + const char *program;
> > + const char *extra_args_verify[1];
> > + const char *sigs[2];
> > +};
>
> These magic numbers are at a weird distance from where we fill them
> in:
>
> > +struct gpg_format_data gpg_formats[] = {
> > + { .format = "openpgp", .program = "gpg",
> > + .extra_args_verify = { "--keyid-format=long" },
> > + .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > + },
> > +};
>
> I'm not sure if we can easily do any better in C, though. Declaring
> the struct with an open-ended "[]" would make the compiler unhappy.
> We could do something like:
>
> struct gpg_format_data {
> ...
> const char **extra_args_verify;
> };
> ...
> static const char *openpgp_verify_args[] = {
> "--key-id-format=long"
> };
> ...
> static struct gpg_format_data gpg_formats[] = {
> { ...
> .extra_args_verify = openpgp_verify_args
> }
> };
>
> I'm not sure if that's more horrible or less. It's worse to write in
> the first place, but it's slightly easier to maintain going forward. I
> dunno.
I switched to that, looks good but i am also not sure which one is
better.
> > +enum gpgformats { PGP_FMT };
>
> Looks like we use this only for indexing the gpg_formats array. I know
> that C guarantees 0-indexing, but if we're depending on it, it might
> be worth writing out "PGP_FMT = 0" explicitly. And probably adding a
> comment that this needs to remain in sync with the array.
>
> The other alternative is that we could simply use
> get_format_data("openpgp"), though that does add a minor runtime cost.
Thanks, i got rid of the enum and use get_format_data with two fixed
strings for the two possible matches.
> > +struct gpg_format_data gpg_formats[] = {
> > + { .format = "openpgp", .program = "gpg",
> > + .extra_args_verify = { "--keyid-format=long" },
> > + .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > + },
> > +};
>
> This array should be marked static, I think.
Yes.
> > +static struct gpg_format_data *get_format_data(const char *str)
> > +{
> > + int i;
> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > + if (!strcasecmp(gpg_formats[i].format, str))
> > + return gpg_formats + i;
> > + return NULL;
> > +}
>
> This looks much nicer than the assert()-ing version from v1.
>
> > +static struct gpg_format_data *get_format_data_by_sig(const char
> > *sig) +{
> > + int i, j;
> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > + for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);
> > j++)
> > + if (gpg_formats[i].sigs[j] &&
> > + !strncmp(gpg_formats[i].sigs[j], sig,
> > +
> > strlen(gpg_formats[i].sigs[j])))
> > + return gpg_formats + i;
> > + return NULL;
> > +}
>
> This might be a little more readable with:
>
> starts_with(sig, gpg_formats[i].sigs[j])
>
> instead of the strncmp. It may also be more efficient, as we don't
> have to compute the strlen of the prefix for each non-matching line
> (the compiler _might_ be smart enough to realize these are all string
> literals, but it's pretty buried).
Thanks, will do.
> I also wondered if our prefix matching here is overly loose. We have
> to do a prefix match, since "sig" isn't terminated at the line
> buffer. So I think we'd match:
>
> --- BEGIN PGP MESSAGE --- AND SOME OTHER STUFF ---
>
> on a line. But I think that's no different than the current code. If
> we care, I guess we could look for '\n' or '\0' immediately after.
Let us not care, just like current code.
> > static int is_gpg_start(const char *line)
> > {
> > - return starts_with(line, PGP_SIGNATURE) ||
> > - starts_with(line, PGP_MESSAGE);
> > + return (get_format_data_by_sig(line) != NULL);
> > }
>
> I don't know if we've ever discussed this style explicitly, but we'd
> usually omit the unnecessary parentheses for the return here.
Will remove those braces.
> > @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const
> > char *value, void *cb) }
> >
> > if (!strcmp(var, "gpg.format")) {
> > - if (strcasecmp(value, "openpgp"))
> > + if (!get_format_data(value))
> > return error("malformed value for %s: %s",
> > var, value); return git_config_string(&gpg_format, var, value);
> > }
>
> Much nicer than v1.
>
> > @@ -165,12 +194,16 @@ const char *get_signing_key(void)
> > int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
> > const char *signing_key) {
> > struct child_process gpg = CHILD_PROCESS_INIT;
> > + struct gpg_format_data *fmt;
> > int ret;
> > size_t i, j, bottom;
> > struct strbuf gpg_status = STRBUF_INIT;
> >
> > + fmt = get_format_data(gpg_format);
> > + if (!fmt)
> > + BUG("bad gpg_format '%s'", gpg_format);
>
> This makes sense as a BUG, because we would already have validated it
> when parsing gpg.format earlier.
> That does make me wonder if we should
> simply be storing a "struct gpg_format_data" instead of a string,
> though. I.e., at the top-level:
>
> /* default to signing with openpgp */
> static struct gpg_format_data *gpg_format = &gpg_formats[0];
Good idea, implemented that.
> > @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload,
> > size_t payload_size, return -1;
> > }
> >
> > + fmt = get_format_data_by_sig(signature);
> > + assert(fmt);
>
> Is this assert() right? The signature data comes from the user. I
> guess to get here we'll already have matched their signature via
> is_gpg_start(), and this is just a cross-check? If so, then it's OK to
> assert, but a BUG() with a descriptive message would be better still.
Turned that into a BUG(). I knew there where 2 such asserts but must
have been looking for assert(0) when i fixed the first.
> I also wonder if whoever parses the signature should get back a
> gpg_format_data and just pass it in here, so we don't have to reparse.
> That's what my earlier series did. It requires tweaking the function
> signatures, but IMHO the result was a lot more obvious.
It would make sense to return the type of signature right with
parse_signature, and would probably safe a few lookups. I started the
refactoring and it turned out to become a pretty big change. That is
why i would prefer to do that on top of the series or leave it the way
it is now.
> > + argv_array_pushl(&gpg.args,
> > + fmt->program, NULL);
>
> If you're just pushing one thing, you don't need pushl(). You can
> just:
>
> argv_array_push(&gpg.args, fmt->program);
>
> > + for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> > + if (fmt->extra_args_verify[i])
> > + argv_array_pushl(&gpg.args,
> > +
> > fmt->extra_args_verify[i], NULL);
>
> Likewise here. Though if you made extra_args_verify a NULL-terminated
> list, this whole loop could become:
>
> argv_array_pushv(&gpg.args, fmt->extra_args_verify);
>
> It's not _that_ much code, but I think using NULL-terminated lists in
> a situation like this is more idiomatic for our code base.
Thanks, good point!
> -Peff
next prev parent reply other threads:[~2018-07-13 8:41 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-10 8:52 [PATCH v2 0/9] X509 (gpgsm) commit signing support Henning Schild
2018-07-10 8:52 ` [PATCH v2 1/9] builtin/receive-pack: use check_signature from gpg-interface Henning Schild
2018-07-10 8:52 ` [PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header Henning Schild
2018-07-10 16:47 ` Junio C Hamano
2018-07-11 8:41 ` Henning Schild
2018-07-10 8:52 ` [PATCH v2 3/9] gpg-interface: add new config to select how to sign a commit Henning Schild
2018-07-10 15:56 ` Jeff King
2018-07-10 8:52 ` [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format Henning Schild
2018-07-10 15:55 ` Jeff King
2018-07-11 8:02 ` Henning Schild
2018-07-10 16:54 ` Junio C Hamano
2018-07-11 8:47 ` Henning Schild
2018-07-10 8:52 ` [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats Henning Schild
2018-07-10 16:23 ` Jeff King
2018-07-13 8:41 ` Henning Schild [this message]
2018-07-10 17:16 ` Junio C Hamano
2018-07-13 8:41 ` Henning Schild
2018-07-10 8:52 ` [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore Henning Schild
2018-07-10 15:49 ` Jeff King
2018-07-11 8:54 ` Henning Schild
2018-07-11 12:34 ` Jeff King
2018-07-11 13:46 ` Henning Schild
2018-07-11 14:27 ` Jeff King
2018-07-11 16:15 ` Henning Schild
2018-07-11 16:38 ` Jeff King
2018-07-10 8:52 ` [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program Henning Schild
2018-07-10 16:54 ` Jeff King
2018-07-10 16:56 ` Jeff King
2018-07-14 18:13 ` brian m. carlson
2018-07-16 21:35 ` Jeff King
2018-07-16 21:56 ` Junio C Hamano
2018-07-16 22:23 ` Jeff King
2018-07-16 23:12 ` Junio C Hamano
2018-07-10 17:29 ` Junio C Hamano
2018-07-13 8:41 ` Henning Schild
2018-07-10 8:52 ` [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm Henning Schild
2018-07-10 17:01 ` Jeff King
2018-07-10 17:40 ` Junio C Hamano
2018-07-10 17:50 ` Jeff King
2018-07-11 9:18 ` Henning Schild
2018-07-10 8:52 ` [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM Henning Schild
2018-07-10 17:09 ` Jeff King
2018-07-10 17:16 ` Jeff King
2018-07-11 10:38 ` Henning Schild
2018-07-11 12:51 ` Jeff King
2018-07-11 13:40 ` Henning Schild
2018-07-11 14:35 ` Jeff King
2018-07-11 15:48 ` Henning Schild
2018-07-11 16:26 ` Junio C Hamano
2018-07-14 18:26 ` brian m. carlson
2018-07-10 21:12 ` Junio C Hamano
2018-07-11 10:38 ` Henning Schild
2018-07-11 14:33 ` Jeff King
2018-07-11 16:35 ` Henning Schild
2018-07-10 17:12 ` [PATCH v2 0/9] X509 (gpgsm) commit signing support Jeff King
2018-07-14 18:33 ` brian m. carlson
2018-07-16 21:32 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180713104126.06017661@md1pvb1c.ad001.siemens.net \
--to=henning.schild@siemens.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.agren@gmail.com \
--cc=mastahyeti@gmail.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
--cc=sandals@crustytoothpaste.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.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).